From 4392205f63e49b9675b06e584f571a48b017d0b6 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Fri, 21 Nov 2014 21:32:56 -0600 Subject: [PATCH] SEC-2347: CSRF Enabled by default w/ XML Config --- .../config/http/CsrfBeanDefinitionParser.java | 11 ++++++-- .../config/http/HttpConfigurationBuilder.java | 18 +++++++------ .../security/config/spring-security-4.0.rnc | 3 +++ .../security/config/spring-security-4.0.xsd | 7 +++++ .../http/AbstractHttpConfigTests.groovy | 2 +- .../config/http/CsrfConfigTests.groovy | 27 ++++++++++++++++++- .../FormLoginBeanDefinitionParserTests.groovy | 4 +++ .../http/InterceptUrlConfigTests.groovy | 1 + .../config/http/LogoutConfigTests.groovy | 1 + .../config/http/MiscHttpConfigTests.groovy | 27 +++++++------------ .../http/MultiHttpBlockConfigTests.groovy | 2 ++ .../config/http/RememberMeConfigTests.groovy | 2 ++ ...ontextHolderAwareRequestConfigTests.groovy | 5 +++- .../http/SessionManagementConfigTests.groovy | 10 ++++++- docs/manual/src/asciidoc/index.adoc | 12 ++++----- 15 files changed, 93 insertions(+), 39 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/http/CsrfBeanDefinitionParser.java b/config/src/main/java/org/springframework/security/config/http/CsrfBeanDefinitionParser.java index 45fbf99a4e..4455cc1595 100644 --- a/config/src/main/java/org/springframework/security/config/http/CsrfBeanDefinitionParser.java +++ b/config/src/main/java/org/springframework/security/config/http/CsrfBeanDefinitionParser.java @@ -56,6 +56,10 @@ public class CsrfBeanDefinitionParser implements BeanDefinitionParser { private BeanDefinition csrfFilter; public BeanDefinition parse(Element element, ParserContext pc) { + boolean disabled = element != null && "true".equals(element.getAttribute("disabled")); + if(disabled) { + return null; + } boolean webmvcPresent = ClassUtils.isPresent(DISPATCHER_SERVLET_CLASS_NAME, getClass().getClassLoader()); if(webmvcPresent) { RootBeanDefinition beanDefinition = new RootBeanDefinition(CsrfRequestDataValueProcessor.class); @@ -64,8 +68,11 @@ public class CsrfBeanDefinitionParser implements BeanDefinitionParser { pc.registerBeanComponent(componentDefinition); } - csrfRepositoryRef = element.getAttribute(ATT_REPOSITORY); - String matcherRef = element.getAttribute(ATT_MATCHER); + String matcherRef = null; + if(element != null) { + csrfRepositoryRef = element.getAttribute(ATT_REPOSITORY); + matcherRef = element.getAttribute(ATT_MATCHER); + } if(!StringUtils.hasText(csrfRepositoryRef)) { RootBeanDefinition csrfTokenRepository = new RootBeanDefinition(HttpSessionCsrfTokenRepository.class); diff --git a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java index 57a8198c75..b158df688a 100644 --- a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java +++ b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java @@ -642,16 +642,18 @@ class HttpConfigurationBuilder { } - private CsrfBeanDefinitionParser createCsrfFilter() { + private void createCsrfFilter() { Element elmt = DomUtils.getChildElementByTagName(httpElt, Elements.CSRF); - if (elmt != null) { - csrfParser = new CsrfBeanDefinitionParser(); - csrfFilter = csrfParser.parse(elmt, pc); - this.csrfAuthStrategy = csrfParser.getCsrfAuthenticationStrategy(); - this.csrfLogoutHandler = csrfParser.getCsrfLogoutHandler(); - return csrfParser; + csrfParser = new CsrfBeanDefinitionParser(); + csrfFilter = csrfParser.parse(elmt, pc); + + if(csrfFilter == null) { + csrfParser = null; + return; } - return null; + + this.csrfAuthStrategy = csrfParser.getCsrfAuthenticationStrategy(); + this.csrfLogoutHandler = csrfParser.getCsrfLogoutHandler(); } BeanMetadataElement getCsrfLogoutHandler() { diff --git a/config/src/main/resources/org/springframework/security/config/spring-security-4.0.rnc b/config/src/main/resources/org/springframework/security/config/spring-security-4.0.rnc index 3fab552aef..e7e8c4b2f2 100644 --- a/config/src/main/resources/org/springframework/security/config/spring-security-4.0.rnc +++ b/config/src/main/resources/org/springframework/security/config/spring-security-4.0.rnc @@ -721,6 +721,9 @@ jdbc-user-service.attlist &= csrf = ## Element for configuration of the CsrfFilter for protection against CSRF. It also updates the default RequestCache to only replay "GET" requests. element csrf {csrf-options.attlist} +csrf-options.attlist &= + ## Specifies if csrf protection should be disabled. Default false (i.e. CSRF protection is enabled). + attribute disabled {xsd:boolean}? csrf-options.attlist &= ## The RequestMatcher instance to be used to determine if CSRF should be applied. Default is any HTTP method except "GET", "TRACE", "HEAD", "OPTIONS" attribute request-matcher-ref { xsd:token }? diff --git a/config/src/main/resources/org/springframework/security/config/spring-security-4.0.xsd b/config/src/main/resources/org/springframework/security/config/spring-security-4.0.xsd index 3380bbadcf..1a6549b713 100644 --- a/config/src/main/resources/org/springframework/security/config/spring-security-4.0.xsd +++ b/config/src/main/resources/org/springframework/security/config/spring-security-4.0.xsd @@ -2251,6 +2251,13 @@ + + + Specifies if csrf protection should be disabled. Default false (i.e. CSRF protection is + enabled). + + + The RequestMatcher instance to be used to determine if CSRF should be applied. Default is diff --git a/config/src/test/groovy/org/springframework/security/config/http/AbstractHttpConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/AbstractHttpConfigTests.groovy index 1cdc178346..5c82e4bb77 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/AbstractHttpConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/AbstractHttpConfigTests.groovy @@ -26,7 +26,7 @@ import org.springframework.security.web.FilterInvocation * */ abstract class AbstractHttpConfigTests extends AbstractXmlConfigTests { - final int AUTO_CONFIG_FILTERS = 13; + final int AUTO_CONFIG_FILTERS = 14; def httpAutoConfig(Closure c) { xml.http('auto-config': 'true', c) diff --git a/config/src/test/groovy/org/springframework/security/config/http/CsrfConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/CsrfConfigTests.groovy index 09902f05b0..d2081733ea 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/CsrfConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/CsrfConfigTests.groovy @@ -47,9 +47,34 @@ class CsrfConfigTests extends AbstractHttpConfigTests { MockHttpServletResponse response = new MockHttpServletResponse() MockFilterChain chain = new MockFilterChain() - def 'no http csrf filter by default'() { + @Unroll + def 'csrf is enabled by default'() { + setup: + httpAutoConfig { + } + createAppContext() + when: + request.method = httpMethod + springSecurityFilterChain.doFilter(request,response,chain) + then: + response.status == httpStatus + where: + httpMethod | httpStatus + 'POST' | HttpServletResponse.SC_FORBIDDEN + 'PUT' | HttpServletResponse.SC_FORBIDDEN + 'PATCH' | HttpServletResponse.SC_FORBIDDEN + 'DELETE' | HttpServletResponse.SC_FORBIDDEN + 'INVALID' | HttpServletResponse.SC_FORBIDDEN + 'GET' | HttpServletResponse.SC_OK + 'HEAD' | HttpServletResponse.SC_OK + 'TRACE' | HttpServletResponse.SC_OK + 'OPTIONS' | HttpServletResponse.SC_OK + } + + def 'csrf disabled'() { when: httpAutoConfig { + csrf(disabled:true) } createAppContext() then: diff --git a/config/src/test/groovy/org/springframework/security/config/http/FormLoginBeanDefinitionParserTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/FormLoginBeanDefinitionParserTests.groovy index e51d8d5ab9..37f23194d8 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/FormLoginBeanDefinitionParserTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/FormLoginBeanDefinitionParserTests.groovy @@ -16,6 +16,7 @@ class FormLoginBeanDefinitionParserTests extends AbstractHttpConfigTests { MockHttpServletResponse response = new MockHttpServletResponse() MockFilterChain chain = new MockFilterChain() httpAutoConfig { + csrf(disabled:true) } createAppContext() when: @@ -38,6 +39,7 @@ class FormLoginBeanDefinitionParserTests extends AbstractHttpConfigTests { MockFilterChain chain = new MockFilterChain() httpAutoConfig { 'form-login'('login-processing-url':'/login_custom','username-parameter':'custom_user','password-parameter':'custom_password') + csrf(disabled:true) } createAppContext() when: @@ -60,6 +62,7 @@ class FormLoginBeanDefinitionParserTests extends AbstractHttpConfigTests { MockFilterChain chain = new MockFilterChain() httpAutoConfig { 'openid-login'() + csrf(disabled:true) } createAppContext() when: @@ -87,6 +90,7 @@ class FormLoginBeanDefinitionParserTests extends AbstractHttpConfigTests { MockFilterChain chain = new MockFilterChain() httpAutoConfig { 'openid-login'('login-processing-url':'/login_custom') + csrf(disabled:true) } createAppContext() when: diff --git a/config/src/test/groovy/org/springframework/security/config/http/InterceptUrlConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/InterceptUrlConfigTests.groovy index 4a6e26dd2a..18df329acb 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/InterceptUrlConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/InterceptUrlConfigTests.groovy @@ -110,6 +110,7 @@ class InterceptUrlConfigTests extends AbstractHttpConfigTests { xml.http() { 'http-basic'() 'intercept-url'(pattern: '/**', 'method':'PATCH',access: 'ROLE_ADMIN') + csrf(disabled:true) } createAppContext() when: 'Method other than PATCH is used' diff --git a/config/src/test/groovy/org/springframework/security/config/http/LogoutConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/LogoutConfigTests.groovy index 49e13c425f..7820d171a3 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/LogoutConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/LogoutConfigTests.groovy @@ -13,6 +13,7 @@ class LogoutConfigTests extends AbstractHttpConfigTests { when: httpAutoConfig { 'logout'('logout-url':'/logout') + csrf(disabled:true) } createAppContext() diff --git a/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy index b50d1dd06c..795039bdf3 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/MiscHttpConfigTests.groovy @@ -15,6 +15,7 @@ */ package org.springframework.security.config.http +import org.springframework.security.web.csrf.CsrfFilter import org.springframework.security.web.header.HeaderWriterFilter import java.security.Principal @@ -107,6 +108,7 @@ class MiscHttpConfigTests extends AbstractHttpConfigTests { assert filters.next() instanceof SecurityContextPersistenceFilter assert filters.next() instanceof WebAsyncManagerIntegrationFilter assert filters.next() instanceof HeaderWriterFilter + assert filters.next() instanceof CsrfFilter assert filters.next() instanceof LogoutFilter Object authProcFilter = filters.next(); assert authProcFilter instanceof UsernamePasswordAuthenticationFilter @@ -187,7 +189,7 @@ class MiscHttpConfigTests extends AbstractHttpConfigTests { createAppContext() expect: - getFilters("/anything")[7] instanceof AnonymousAuthenticationFilter + getFilters("/anything")[8] instanceof AnonymousAuthenticationFilter } def anonymousFilterIsRemovedIfDisabledFlagSet() { @@ -360,7 +362,7 @@ class MiscHttpConfigTests extends AbstractHttpConfigTests { AUTO_CONFIG_FILTERS + 3 == filters.size(); filters[0] instanceof SecurityContextHolderAwareRequestFilter filters[1] instanceof SecurityContextPersistenceFilter - filters[6] instanceof SecurityContextHolderAwareRequestFilter + filters[7] instanceof SecurityContextHolderAwareRequestFilter filters[1] instanceof SecurityContextPersistenceFilter } @@ -383,7 +385,7 @@ class MiscHttpConfigTests extends AbstractHttpConfigTests { createAppContext() expect: - getFilters("/someurl")[4] instanceof X509AuthenticationFilter + getFilters("/someurl")[5] instanceof X509AuthenticationFilter } def x509SubjectPrincipalRegexCanBeSetUsingPropertyPlaceholder() { @@ -420,21 +422,9 @@ class MiscHttpConfigTests extends AbstractHttpConfigTests { def handlers = getFilter(LogoutFilter).handlers expect: - handlers[1] instanceof CookieClearingLogoutHandler - handlers[1].cookiesToClear[0] == 'JSESSIONID' - handlers[1].cookiesToClear[1] == 'mycookie' - } - - def invalidLogoutUrlIsDetected() { - when: - xml.http { - 'logout'('logout-url': 'noLeadingSlash') - 'form-login'() - } - createAppContext() - - then: - BeanCreationException e = thrown(); + handlers[2] instanceof CookieClearingLogoutHandler + handlers[2].cookiesToClear[0] == 'JSESSIONID' + handlers[2].cookiesToClear[1] == 'mycookie' } def logoutSuccessHandlerIsSetCorrectly() { @@ -615,6 +605,7 @@ class MiscHttpConfigTests extends AbstractHttpConfigTests { xml.debug() xml.http() { 'form-login'() + csrf(disabled:true) anonymous(enabled: 'false') } createAppContext() diff --git a/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy index 9aecde85d3..c5829ec425 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/MultiHttpBlockConfigTests.groovy @@ -74,9 +74,11 @@ class MultiHttpBlockConfigTests extends AbstractHttpConfigTests { setup: xml.http('authentication-manager-ref' : 'authManager', 'pattern' : '/first/**') { 'form-login'('login-processing-url': '/first/login') + csrf(disabled:true) } xml.http('authentication-manager-ref' : 'authManager2') { 'form-login'() + csrf(disabled:true) } mockBean(UserDetailsService,'uds') mockBean(UserDetailsService,'uds2') diff --git a/config/src/test/groovy/org/springframework/security/config/http/RememberMeConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/RememberMeConfigTests.groovy index 67a276ed4d..9c5c38f5f4 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/RememberMeConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/RememberMeConfigTests.groovy @@ -91,6 +91,7 @@ class RememberMeConfigTests extends AbstractHttpConfigTests { def rememberMeServiceWorksWithExternalServicesImpl() { httpAutoConfig () { 'remember-me'('key': "#{'our' + 'key'}", 'services-ref': 'rms') + csrf(disabled:true) } xml.'b:bean'(id: 'rms', 'class': TokenBasedRememberMeServices.class.name) { 'b:constructor-arg'(value: 'ourKey') @@ -118,6 +119,7 @@ class RememberMeConfigTests extends AbstractHttpConfigTests { def rememberMeAddsLogoutHandlerToLogoutFilter() { httpAutoConfig () { 'remember-me'() + csrf(disabled:true) } createAppContext(AUTH_PROVIDER_XML) diff --git a/config/src/test/groovy/org/springframework/security/config/http/SecurityContextHolderAwareRequestConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/SecurityContextHolderAwareRequestConfigTests.groovy index 35c26ed69c..b9aa986262 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/SecurityContextHolderAwareRequestConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/SecurityContextHolderAwareRequestConfigTests.groovy @@ -47,7 +47,7 @@ class SecurityContextHolderAwareRequestConfigTests extends AbstractHttpConfigTes def withAutoConfig() { httpAutoConfig () { - + csrf(disabled:true) } createAppContext(AUTH_PROVIDER_XML) @@ -93,10 +93,12 @@ class SecurityContextHolderAwareRequestConfigTests extends AbstractHttpConfigTes xml.http('authentication-manager-ref' : 'authManager', 'pattern' : '/first/**') { 'form-login'('login-page' : '/login') 'logout'('invalidate-session' : 'true') + csrf(disabled:true) } xml.http('authentication-manager-ref' : 'authManager2') { 'form-login'('login-page' : '/login2') 'logout'('invalidate-session' : 'false') + csrf(disabled:true) } String secondAuthManager = AUTH_PROVIDER_XML.replace("alias='authManager'", "id='authManager2'") @@ -125,6 +127,7 @@ class SecurityContextHolderAwareRequestConfigTests extends AbstractHttpConfigTes xml.http() { 'form-login'('login-page' : '/login') 'logout'('invalidate-session' : 'false', 'logout-success-url' : '/login?logout', 'delete-cookies' : 'JSESSIONID') + csrf(disabled:true) } createAppContext(AUTH_PROVIDER_XML) diff --git a/config/src/test/groovy/org/springframework/security/config/http/SessionManagementConfigTests.groovy b/config/src/test/groovy/org/springframework/security/config/http/SessionManagementConfigTests.groovy index 53be4e3373..61b0d99b05 100644 --- a/config/src/test/groovy/org/springframework/security/config/http/SessionManagementConfigTests.groovy +++ b/config/src/test/groovy/org/springframework/security/config/http/SessionManagementConfigTests.groovy @@ -111,6 +111,7 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { 'session-management'() { 'concurrency-control'('max-sessions':1,'error-if-maximum-exceeded':'true') } + csrf(disabled:true) } createAppContext() SessionRegistry registry = appContext.getBean(SessionRegistry) @@ -155,6 +156,7 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { 'session-management'() { 'concurrency-control'('session-registry-alias':'sr', 'expired-url': '/expired') } + csrf(disabled:true) } createAppContext(); List filters = getFilters("/someurl"); @@ -182,8 +184,9 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { } 'logout'('invalidate-session': false, 'delete-cookies': 'testCookie') 'remember-me'() + csrf(disabled:true) } - createAppContext(); + createAppContext() List filters = getFilters("/someurl") ConcurrentSessionFilter concurrentSessionFilter = filters.get(1) @@ -206,6 +209,7 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { 'concurrency-control'() } 'remember-me'() + csrf(disabled:true) }) bean('entryPoint', 'org.springframework.security.web.authentication.Http403ForbiddenEntryPoint') createAppContext() @@ -274,6 +278,7 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { setup: httpAutoConfig { 'session-management'('session-authentication-strategy-ref':'ss') + csrf(disabled:true) } mockBean(SessionAuthenticationStrategy,'ss') createAppContext() @@ -298,6 +303,7 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { 'session-management'() { 'concurrency-control'('session-registry-ref':'sr') } + csrf(disabled:true) } bean('sr', SessionRegistryImpl.class.name) createAppContext(); @@ -354,6 +360,7 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { def disablingSessionProtectionRemovesSessionManagementFilterIfNoInvalidSessionUrlSet() { httpAutoConfig { 'session-management'('session-fixation-protection': 'none') + csrf(disabled:true) } createAppContext() @@ -364,6 +371,7 @@ class SessionManagementConfigTests extends AbstractHttpConfigTests { def disablingSessionProtectionRetainsSessionManagementFilterInvalidSessionUrlSet() { httpAutoConfig { 'session-management'('session-fixation-protection': 'none', 'invalid-session-url': '/timeoutUrl') + csrf(disabled:true) } createAppContext() def filter = getFilters("/someurl")[10] diff --git a/docs/manual/src/asciidoc/index.adoc b/docs/manual/src/asciidoc/index.adoc index 116d79d28d..824f923253 100644 --- a/docs/manual/src/asciidoc/index.adoc +++ b/docs/manual/src/asciidoc/index.adoc @@ -3087,18 +3087,13 @@ This is not a limitation of Spring Security's support, but instead a general req ==== Configure CSRF Protection The next step is to include Spring Security's CSRF protection within your application. Some frameworks handle invalid CSRF tokens by invaliding the user's session, but this causes <>. Instead by default Spring Security's CSRF protection will produce an HTTP 403 access denied. This can be customized by configuring the <> to process `InvalidCsrfTokenException` differently. -For passivity reasons, if you are using the XML configuration, CSRF protection must be explicitly enabled using the <>> element. Refer to the <>> element's documentation for additional customizations. - -[NOTE] -==== -https://jira.springsource.org/browse/SEC-2347[SEC-2347] is logged to ensure Spring Security 4.x's XML namespace configuration will enable CSRF protection by default. -==== +As of Spring Security 4.0, CSRF protection is enabled by default with XML configuration. If you would like to disable CSRF protection, the corresponding XML configuration can be seen below. [source,xml] ---- - + ---- @@ -6895,6 +6890,9 @@ This element will add http://en.wikipedia.org/wiki/Cross-site_request_forgery[Cr [[nsa-csrf-attributes]] ===== Attributes +[[nsa-csrf-disabled]] +* **disabled** +Optional attribute that specifies to disable Spring Security's CSRF protection. The default is false (CSRF protection is enabled). It is highly recommended to leave CSRF protection enabled. [[nsa-csrf-token-repository-ref]] * **token-repository-ref**