diff --git a/server-spi-private/src/main/java/org/keycloak/models/ContentSecurityPolicyBuilder.java b/server-spi-private/src/main/java/org/keycloak/models/ContentSecurityPolicyBuilder.java index b220c1f50f..080854e15b 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/ContentSecurityPolicyBuilder.java +++ b/server-spi-private/src/main/java/org/keycloak/models/ContentSecurityPolicyBuilder.java @@ -1,48 +1,129 @@ +/* + * Copyright 2023 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ package org.keycloak.models; +import java.util.LinkedHashMap; +import java.util.Map; + public class ContentSecurityPolicyBuilder { - private String frameSrc = "'self'"; - private String frameAncestors = "'self'"; - private String objectSrc = "'none'"; + // constants for directive names used in the class + public static final String DIRECTIVE_NAME_FRAME_SRC = "frame-src"; + public static final String DIRECTIVE_NAME_FRAME_ANCESTORS = "frame-ancestors"; + public static final String DIRECTIVE_NAME_OBJECT_SRC = "object-src"; - private boolean first; - private StringBuilder sb; + // constants for specific directive value keywords + public static final String DIRECTIVE_VALUE_SELF = "'self'"; + public static final String DIRECTIVE_VALUE_NONE = "'none'"; + + private final Map directives = new LinkedHashMap<>(); public static ContentSecurityPolicyBuilder create() { - return new ContentSecurityPolicyBuilder(); + return new ContentSecurityPolicyBuilder() + .add(DIRECTIVE_NAME_FRAME_SRC, DIRECTIVE_VALUE_SELF) + .add(DIRECTIVE_NAME_FRAME_ANCESTORS, DIRECTIVE_VALUE_SELF) + .add(DIRECTIVE_NAME_OBJECT_SRC, DIRECTIVE_VALUE_NONE); + } + + public static ContentSecurityPolicyBuilder create(String directives) { + return new ContentSecurityPolicyBuilder().parse(directives); } public ContentSecurityPolicyBuilder frameSrc(String frameSrc) { - this.frameSrc = frameSrc; + if (frameSrc == null) { + directives.remove(DIRECTIVE_NAME_FRAME_SRC); + } else { + put(DIRECTIVE_NAME_FRAME_SRC, frameSrc); + } return this; } + public ContentSecurityPolicyBuilder addFrameSrc(String frameSrc) { + return add(DIRECTIVE_NAME_FRAME_SRC, frameSrc); + } + + public boolean isDefaultFrameAncestors() { + return DIRECTIVE_VALUE_SELF.equals(directives.get(DIRECTIVE_NAME_FRAME_ANCESTORS)); + } + public ContentSecurityPolicyBuilder frameAncestors(String frameancestors) { - this.frameAncestors = frameancestors; + if (frameancestors == null) { + directives.remove(DIRECTIVE_NAME_FRAME_ANCESTORS); + } else { + put(DIRECTIVE_NAME_FRAME_ANCESTORS, frameancestors); + } return this; } + public ContentSecurityPolicyBuilder addFrameAncestors(String frameancestors) { + return add(DIRECTIVE_NAME_FRAME_ANCESTORS, frameancestors); + } + public String build() { - sb = new StringBuilder(); - first = true; - - build("frame-src", frameSrc); - build("frame-ancestors", frameAncestors); - build("object-src", objectSrc); - + StringBuilder sb = new StringBuilder(); + if (!directives.isEmpty()) { + for (Map.Entry entry : directives.entrySet()) { + sb.append(entry.getKey()); + if (!entry.getValue().isEmpty()) { + sb.append(" ").append(entry.getValue()); + } + sb.append("; "); + } + sb.setLength(sb.length() - 1); + } return sb.toString(); } - private void build(String k, String v) { - if (v != null) { - if (!first) { - sb.append(" "); - } - first = false; - - sb.append(k).append(" ").append(v).append(";"); + private ContentSecurityPolicyBuilder put(String name, String value) { + if (name != null && value != null) { + directives.put(name, value); } + return this; } + private ContentSecurityPolicyBuilder add(String name, String value) { + if (name != null && value != null) { + String current = directives.get(name); + if (current != null && !current.isEmpty()) { + value = current + " " + value; + } + directives.put(name, value); + } + return this; + } + + // W3C Working Draft: https://www.w3.org/TR/CSP/ + // Only managing spaces not the other whitespaces defined in the spec + private ContentSecurityPolicyBuilder parse(String value) { + if (value == null) { + return this; + } + String[] values = value.split(";"); + if (values != null) { + for (String directive : values) { + directive = directive.trim(); + int idx = directive.indexOf(' '); + if (idx > 0) { + add(directive.substring(0, idx), directive.substring(idx + 1, directive.length()).trim()); + } else if (!directive.isEmpty()) { + add(directive, ""); + } + } + } + return this; + } } diff --git a/server-spi-private/src/test/java/org/keycloak/models/BrowserSecurityHeadersTest.java b/server-spi-private/src/test/java/org/keycloak/models/BrowserSecurityHeadersTest.java index e4b39afc4a..cb88f12d02 100644 --- a/server-spi-private/src/test/java/org/keycloak/models/BrowserSecurityHeadersTest.java +++ b/server-spi-private/src/test/java/org/keycloak/models/BrowserSecurityHeadersTest.java @@ -24,6 +24,24 @@ public class BrowserSecurityHeadersTest { assertEquals("frame-ancestors 'self'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameSrc(null).build()); assertEquals("frame-src 'self'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameAncestors(null).build()); assertEquals("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameSrc("'custom-frame-src'").frameAncestors("'custom-frame-ancestors'").build()); + assertEquals("frame-src localhost; frame-ancestors 'self'; object-src 'none';", ContentSecurityPolicyBuilder.create().frameSrc("localhost").build()); + assertEquals("frame-src 'self' localhost; frame-ancestors 'self'; object-src 'none';", + ContentSecurityPolicyBuilder.create().addFrameSrc("localhost").build()); + } + + private void assertParsedDirectives(String directives) { + assertEquals(directives, ContentSecurityPolicyBuilder.create(directives).build()); + } + + @Test + public void parseSecurityPolicyBuilderTest() { + assertParsedDirectives("frame-src 'self'; frame-ancestors 'self'; object-src 'none';"); + assertParsedDirectives("frame-ancestors 'self'; object-src 'none';"); + assertParsedDirectives("frame-src 'self'; object-src 'none';"); + assertParsedDirectives("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none';"); + assertParsedDirectives("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none'; style-src 'self';"); + assertParsedDirectives("frame-src 'custom-frame-src'; frame-ancestors 'custom-frame-ancestors'; object-src 'none'; sandbox;"); + assertEquals("frame-src 'custom-frame-src'; sandbox;", ContentSecurityPolicyBuilder.create("frame-src 'custom-frame-src' ; sandbox ; ").build()); } @Test diff --git a/services/src/main/java/org/keycloak/headers/DefaultSecurityHeadersProvider.java b/services/src/main/java/org/keycloak/headers/DefaultSecurityHeadersProvider.java index 58af8967d9..b10b2b28df 100644 --- a/services/src/main/java/org/keycloak/headers/DefaultSecurityHeadersProvider.java +++ b/services/src/main/java/org/keycloak/headers/DefaultSecurityHeadersProvider.java @@ -106,22 +106,24 @@ public class DefaultSecurityHeadersProvider implements SecurityHeadersProvider { // TODO This will be refactored as part of introducing a more strict CSP header if (options != null) { - ContentSecurityPolicyBuilder csp = ContentSecurityPolicyBuilder.create(); + ContentSecurityPolicyBuilder csp = ContentSecurityPolicyBuilder.create( + headers.getFirst(CONTENT_SECURITY_POLICY.getHeaderName()).toString()); if (options.isAllowAnyFrameAncestor()) { headers.remove(BrowserSecurityHeaders.X_FRAME_OPTIONS.getHeaderName()); - csp.frameAncestors(null); + if (csp.isDefaultFrameAncestors()) { + // only remove frame ancestors if defined to default 'self' + csp.frameAncestors(null); + } } String allowedFrameSrc = options.getAllowedFrameSrc(); if (allowedFrameSrc != null) { - csp.frameSrc(allowedFrameSrc); + csp.addFrameSrc(allowedFrameSrc); } - if (CONTENT_SECURITY_POLICY.getDefaultValue().equals(headers.getFirst(CONTENT_SECURITY_POLICY.getHeaderName()))) { - headers.putSingle(CONTENT_SECURITY_POLICY.getHeaderName(), csp.build()); - } + headers.putSingle(CONTENT_SECURITY_POLICY.getHeaderName(), csp.build()); } } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/FrontChannelLogoutHandler.java b/services/src/main/java/org/keycloak/protocol/oidc/FrontChannelLogoutHandler.java index bcac389cc6..f292aa89c0 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/FrontChannelLogoutHandler.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/FrontChannelLogoutHandler.java @@ -67,7 +67,6 @@ public class FrontChannelLogoutHandler { allowFrameSrc.append(client.frontChannelLogoutUrl.getAuthority()).append(' '); } - session.getProvider(SecurityHeadersProvider.class).options().allowAnyFrameAncestor(); session.getProvider(SecurityHeadersProvider.class).options().allowFrameSrc(allowFrameSrc.toString()); } diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java index 7e04bb8cb8..d3fd229b70 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/updaters/ClientAttributeUpdater.java @@ -68,6 +68,11 @@ public class ClientAttributeUpdater extends ServerResourceUpdater