From 488d4e33239becd0368c78173fb62dd71ae918ec Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Sat, 23 Jan 2016 22:02:25 +0100 Subject: [PATCH 1/4] implemented xsrf protection, see issue #793 --- .../main/java/sonia/scm/ScmServletModule.java | 4 + .../resources/AuthenticationResource.java | 6 + .../java/sonia/scm/security/XsrfCookies.java | 87 +++++++++ .../scm/security/XsrfProtectionFilter.java | 112 +++++++++++ .../main/webapp/resources/js/sonia.global.js | 13 ++ .../sonia/scm/security/XsrfCookiesTest.java | 108 +++++++++++ .../security/XsrfProtectionFilterTest.java | 181 ++++++++++++++++++ 7 files changed, 511 insertions(+) create mode 100644 scm-webapp/src/main/java/sonia/scm/security/XsrfCookies.java create mode 100644 scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java create mode 100644 scm-webapp/src/test/java/sonia/scm/security/XsrfCookiesTest.java create mode 100644 scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java diff --git a/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java b/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java index 9087755f93..acc58fed15 100644 --- a/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java +++ b/scm-webapp/src/main/java/sonia/scm/ScmServletModule.java @@ -162,6 +162,7 @@ import sonia.scm.net.ahc.ContentTransformer; import sonia.scm.net.ahc.DefaultAdvancedHttpClient; import sonia.scm.net.ahc.JsonContentTransformer; import sonia.scm.net.ahc.XmlContentTransformer; +import sonia.scm.security.XsrfProtectionFilter; import sonia.scm.web.UserAgentParser; /** @@ -365,6 +366,9 @@ public class ScmServletModule extends ServletModule filter(PATTERN_ALL).through(LoggingFilter.class); } + // protect api agains xsrf attacks + filter(PATTERN_RESTAPI).through(XsrfProtectionFilter.class); + /* * filter(PATTERN_PAGE, * PATTERN_STATIC_RESOURCES).through(StaticResourceFilter.class); diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AuthenticationResource.java b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AuthenticationResource.java index b7f02187cb..36d296e1c5 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AuthenticationResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AuthenticationResource.java @@ -79,9 +79,11 @@ import sonia.scm.util.HttpUtil; import java.util.Collection; import java.util.Collections; import java.util.List; +import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; import javax.ws.rs.DefaultValue; import javax.ws.rs.FormParam; @@ -96,6 +98,7 @@ import javax.ws.rs.core.Response; import javax.xml.bind.annotation.XmlAccessType; import javax.xml.bind.annotation.XmlAccessorType; import javax.xml.bind.annotation.XmlRootElement; +import sonia.scm.security.XsrfCookies; /** * @@ -261,6 +264,9 @@ public class AuthenticationResource public Response logout(@Context HttpServletRequest request, @Context HttpServletResponse response) { + // remove xsrf token + XsrfCookies.remove(request, response); + Subject subject = SecurityUtils.getSubject(); subject.logout(); diff --git a/scm-webapp/src/main/java/sonia/scm/security/XsrfCookies.java b/scm-webapp/src/main/java/sonia/scm/security/XsrfCookies.java new file mode 100644 index 0000000000..de3c26995a --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/security/XsrfCookies.java @@ -0,0 +1,87 @@ +/** + * Copyright (c) 2014, Sebastian Sdorra + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * 3. Neither the name of SCM-Manager; nor the names of its + * contributors may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * http://bitbucket.org/sdorra/scm-manager + * + */ +package sonia.scm.security; + +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +/** + * Util methods to handle XsrfCookies. + * + * @author Sebastian Sdorra + * @version 1.47 + */ +public final class XsrfCookies +{ + + private XsrfCookies() + { + } + + /** + * Creates a new xsrf protection cookie and add it to the response. + * + * @param request http servlet request + * @param response http servlet response + * @param token xsrf token + */ + public static void create(HttpServletRequest request, HttpServletResponse response, String token){ + applyCookie(request, response, new Cookie(XsrfProtectionFilter.KEY, token)); + + } + + /** + * Removes the current xsrf protection cookie from response. + * + * @param request http servlet request + * @param response http servlet response + */ + public static void remove(HttpServletRequest request, HttpServletResponse response) + { + Cookie[] cookies = request.getCookies(); + if ( cookies != null ){ + for ( Cookie c : cookies ){ + if ( XsrfProtectionFilter.KEY.equals(c.getName()) ){ + c.setMaxAge(0); + c.setValue(null); + applyCookie(request, response, c); + } + } + } + } + + private static void applyCookie(HttpServletRequest request, HttpServletResponse response, Cookie cookie){ + cookie.setPath(request.getContextPath()); + response.addCookie(cookie); + } + +} diff --git a/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java b/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java new file mode 100644 index 0000000000..7e7391190c --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java @@ -0,0 +1,112 @@ +/** + * Copyright (c) 2014, Sebastian Sdorra + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * 3. Neither the name of SCM-Manager; nor the names of its + * contributors may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * http://bitbucket.org/sdorra/scm-manager + * + */ +package sonia.scm.security; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; +import java.io.IOException; +import java.util.UUID; +import javax.inject.Singleton; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import sonia.scm.util.HttpUtil; +import sonia.scm.web.filter.HttpFilter; + +/** + * Xsrf protection http filter. The filter will issue an cookie with an xsrf protection token on the first ajax request + * of the scm web interface and marks the http session as xsrf protected. On every other request within a protected + * session, the web interface has to send the token from the cookie as http header on every request. If the filter + * receives an request to a protected session, without proper xsrf header the filter will abort the request and send an + * http error code back to the client. If the filter receives an request to a non protected session, from a non web + * interface client the filter will call the chain. + * + * TODO for scm-manager 2 we have to store the csrf token as part of the jwt token instead of session. + * + * @see https://bitbucket.org/sdorra/scm-manager/issues/793/json-hijacking-vulnerability-cwe-116-cwe + * @author Sebastian Sdorra + * @version 1.47 + */ +@Singleton +public final class XsrfProtectionFilter extends HttpFilter +{ + + /** + * the logger for XsrfProtectionFilter + */ + private static final Logger logger = LoggerFactory.getLogger(XsrfProtectionFilter.class); + + /** + * Key used for session, header and cookie. + */ + static final String KEY = "X-XSRF-Token"; + + @Override + protected void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws + IOException, ServletException + { + HttpSession session = request.getSession(true); + String storedToken = (String) session.getAttribute(KEY); + if ( ! Strings.isNullOrEmpty(storedToken) ){ + String headerToken = request.getHeader(KEY); + if ( storedToken.equals(headerToken) ){ + logger.trace("received valid xsrf protected request"); + chain.doFilter(request, response); + } else { + // is forbidden the correct status code? + logger.warn("received request to a xsrf protected session without proper xsrf token"); + response.sendError(HttpServletResponse.SC_FORBIDDEN); + } + } else if (HttpUtil.isWUIRequest(request)) { + logger.debug("received wui request, mark session as xsrf protected and issue a new token"); + String token = createToken(); + session.setAttribute(KEY, token); + XsrfCookies.create(request, response, token); + chain.doFilter(request, response); + } else { + // handle non webinterface clients, which does not need xsrf protection + logger.trace("received request to a non xsrf protected session"); + chain.doFilter(request, response); + } + } + + private String createToken() + { + // TODO create interface and use a better method + return UUID.randomUUID().toString(); + } + +} diff --git a/scm-webapp/src/main/webapp/resources/js/sonia.global.js b/scm-webapp/src/main/webapp/resources/js/sonia.global.js index 32dec370a9..4513a05e6e 100644 --- a/scm-webapp/src/main/webapp/resources/js/sonia.global.js +++ b/scm-webapp/src/main/webapp/resources/js/sonia.global.js @@ -37,6 +37,19 @@ Ext.Ajax.defaultHeaders = { 'X-SCM-Client': 'WUI' }; +// XSRF protection +Ext.Ajax.on('beforerequest', function(conn, options){ + var token = Ext.util.Cookies.get('X-XSRF-Token'); + console.log(token); + console.log(options); + if (token){ + if (!options.headers){ + options.headers = {}; + } + options.headers['X-XSRF-Token'] = token; + } +}); + var state = null; var admin = false; diff --git a/scm-webapp/src/test/java/sonia/scm/security/XsrfCookiesTest.java b/scm-webapp/src/test/java/sonia/scm/security/XsrfCookiesTest.java new file mode 100644 index 0000000000..64dd27bc3b --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/security/XsrfCookiesTest.java @@ -0,0 +1,108 @@ +/** + * Copyright (c) 2014, Sebastian Sdorra + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * 3. Neither the name of SCM-Manager; nor the names of its + * contributors may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * http://bitbucket.org/sdorra/scm-manager + * + */ + +package sonia.scm.security; + +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.junit.Test; +import static org.junit.Assert.*; +import org.junit.Before; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import static org.mockito.Mockito.*; +import org.mockito.runners.MockitoJUnitRunner; + +/** + * Tests for the {@link XsrfCookies} util class. + * + * @author Sebastian Sdorra + */ +@RunWith(MockitoJUnitRunner.class) +public class XsrfCookiesTest { + + @Mock + private HttpServletRequest request; + + @Mock + private HttpServletResponse response; + + /** + * Prepare mocks for testing. + */ + @Before + public void prepareMocks(){ + when(request.getContextPath()).thenReturn("/scm"); + } + + /** + * Tests create method. + */ + @Test + public void testCreate() + { + XsrfCookies.create(request, response, "mytoken"); + + // capture cookie + ArgumentCaptor captor = ArgumentCaptor.forClass(Cookie.class); + verify(response).addCookie(captor.capture()); + + // check for cookie + Cookie cookie = captor.getValue(); + assertEquals(XsrfProtectionFilter.KEY, cookie.getName()); + assertEquals("/scm", cookie.getPath()); + assertEquals("mytoken", cookie.getValue()); + } + + /** + * Tests remove method. + */ + @Test + public void testRemove(){ + Cookie cookie = new Cookie(XsrfProtectionFilter.KEY, "mytoken"); + cookie.setMaxAge(15); + when(request.getCookies()).thenReturn(new Cookie[]{cookie}); + XsrfCookies.remove(request, response); + + // capture cookie + ArgumentCaptor captor = ArgumentCaptor.forClass(Cookie.class); + verify(response).addCookie(captor.capture()); + + // check the captured cookie + Cookie c = captor.getValue(); + assertEquals("cookie max age should be set to 0", 0, c.getMaxAge()); + assertEquals("cookie path should be equals", cookie.getPath(), c.getPath()); + assertNull("cookie value shuld be null", c.getValue()); + } + +} \ No newline at end of file diff --git a/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java b/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java new file mode 100644 index 0000000000..2cb187c24d --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java @@ -0,0 +1,181 @@ +/** + * Copyright (c) 2014, Sebastian Sdorra + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * 3. Neither the name of SCM-Manager; nor the names of its + * contributors may be used to endorse or promote products derived from this + * software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + * http://bitbucket.org/sdorra/scm-manager + * + */ + +package sonia.scm.security; + +import java.io.IOException; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; +import org.junit.Test; +import static org.junit.Assert.*; +import org.junit.Before; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import static org.mockito.Mockito.*; +import org.mockito.runners.MockitoJUnitRunner; +import sonia.scm.util.HttpUtil; + +/** + * Unit tests for {@link XsrfProtectionFilter}. + * + * @author Sebastian Sdorra + */ +@RunWith(MockitoJUnitRunner.class) +public class XsrfProtectionFilterTest { + + @Mock + private HttpServletRequest request; + + @Mock + private HttpServletResponse response; + + @Mock + private HttpSession session; + + @Mock + private FilterChain chain; + + private final XsrfProtectionFilter filter = new XsrfProtectionFilter(); + + /** + * Prepare mocks for testing. + */ + @Before + public void setUp(){ + when(request.getSession(true)).thenReturn(session); + when(request.getContextPath()).thenReturn("/scm"); + } + + /** + * Test filter method for non web interface clients. + * + * @throws IOException + * @throws ServletException + */ + @Test + public void testDoFilterFromNonWuiClient() throws IOException, ServletException + { + filter.doFilter(request, response, chain); + verify(chain).doFilter(request, response); + } + + /** + * Test filter method for first web interface request. + * + * @throws IOException + * @throws ServletException + */ + @Test + public void testDoFilterIssuesTokenOnFirstWuiRequest() throws IOException, ServletException + { + when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + + // call the filter + filter.doFilter(request, response, chain); + + // capture cookie + ArgumentCaptor captor = ArgumentCaptor.forClass(Cookie.class); + verify(response).addCookie(captor.capture()); + + // check for cookie + Cookie cookie = captor.getValue(); + assertEquals(XsrfProtectionFilter.KEY, cookie.getName()); + assertEquals("/scm", cookie.getPath()); + assertNotNull(cookie.getValue()); + + // ensure filter chain is called + verify(chain).doFilter(request, response); + } + + /** + * Test filter method on protected session with an invalid xsrf token. + * + * @throws IOException + * @throws ServletException + */ + @Test + public void testDoFilterWithInvalidToken() throws IOException, ServletException { + when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + when(request.getHeader(XsrfProtectionFilter.KEY)).thenReturn("invalidtoken"); + when(session.getAttribute(XsrfProtectionFilter.KEY)).thenReturn("mytoken"); + + // call the filter + filter.doFilter(request, response, chain); + + // ensure response send forbidden and the chain was never called + verify(response).sendError(HttpServletResponse.SC_FORBIDDEN); + verify(chain, never()).doFilter(request, response); + } + + /** + * Test filter method on protected session without xsrf token. + * + * @throws IOException + * @throws ServletException + */ + @Test + public void testDoFilterOnProtectedSessionWithoutToken() throws IOException, ServletException { + when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + when(session.getAttribute(XsrfProtectionFilter.KEY)).thenReturn("mytoken"); + + // call the filter + filter.doFilter(request, response, chain); + + // ensure response send forbidden and the chain was never called + verify(response).sendError(HttpServletResponse.SC_FORBIDDEN); + verify(chain, never()).doFilter(request, response); + } + + /** + * Test filter method on protected session with valid xsrf token. + * + * @throws IOException + * @throws ServletException + */ + @Test + public void testDoFilterOnProtectedSessionWithValidToken() throws IOException, ServletException { + when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + when(request.getHeader(XsrfProtectionFilter.KEY)).thenReturn("mytoken"); + when(session.getAttribute(XsrfProtectionFilter.KEY)).thenReturn("mytoken"); + + // call the filter + filter.doFilter(request, response, chain); + + // ensure chain was called + verify(chain).doFilter(request, response); + } + +} \ No newline at end of file From 652b98f53c1eee2f7bd6f66d23da8425ea7fab53 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 24 May 2016 21:12:09 +0200 Subject: [PATCH 2/4] #793 added configuration parameter to enable/disable xsrf protection. The protection is disabled by default until it is battle tested. --- .../sonia/scm/config/ScmConfiguration.java | 37 +++++++++++++++++++ .../scm/security/XsrfProtectionFilter.java | 26 ++++++++++++- .../security/XsrfProtectionFilterTest.java | 32 +++++++++++++++- 3 files changed, 93 insertions(+), 2 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java b/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java index eebefe4e1c..9fea54289e 100644 --- a/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java +++ b/scm-core/src/main/java/sonia/scm/config/ScmConfiguration.java @@ -177,6 +177,7 @@ public class ScmConfiguration this.skipFailedAuthenticators = other.skipFailedAuthenticators; this.loginAttemptLimit = other.loginAttemptLimit; this.loginAttemptLimitTimeout = other.loginAttemptLimitTimeout; + this.enabledXsrfProtection = other.enabledXsrfProtection; // deprecated fields this.servername = other.servername; @@ -424,6 +425,19 @@ public class ScmConfiguration return disableGroupingGrid; } + /** + * Returns {@code true} if the cookie xsrf protection is enabled. + * + * @see Issue 793 + * @return {@code true} if the cookie xsrf protection is enabled + * + * @since 1.47 + */ + public boolean isEnabledXsrfProtection() + { + return enabledXsrfProtection; + } + /** * Returns true if port forwarding is enabled. * @@ -800,6 +814,21 @@ public class ScmConfiguration this.sslPort = sslPort; } + /** + * Set {@code true} to enable xsrf cookie protection. + * + * @param enabledXsrfProtection {@code true} to enable xsrf protection + * @see Issue 793 + * + * @since 1.47 + */ + public void setEnabledXsrfProtection(boolean enabledXsrfProtection) + { + this.enabledXsrfProtection = enabledXsrfProtection; + } + + + //~--- fields --------------------------------------------------------------- /** Field description */ @@ -913,4 +942,12 @@ public class ScmConfiguration /** Field description */ private boolean anonymousAccessEnabled = false; + + /** + * Enables xsrf cookie protection. + * + * @since 1.47 + */ + @XmlElement(name = "xsrf-protection") + private boolean enabledXsrfProtection = false; } diff --git a/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java b/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java index 7e7391190c..333c71066f 100644 --- a/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java +++ b/scm-webapp/src/main/java/sonia/scm/security/XsrfProtectionFilter.java @@ -32,6 +32,7 @@ package sonia.scm.security; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; +import com.google.inject.Inject; import java.io.IOException; import java.util.UUID; import javax.inject.Singleton; @@ -43,6 +44,7 @@ import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import sonia.scm.config.ScmConfiguration; import sonia.scm.util.HttpUtil; import sonia.scm.web.filter.HttpFilter; @@ -52,7 +54,8 @@ import sonia.scm.web.filter.HttpFilter; * session, the web interface has to send the token from the cookie as http header on every request. If the filter * receives an request to a protected session, without proper xsrf header the filter will abort the request and send an * http error code back to the client. If the filter receives an request to a non protected session, from a non web - * interface client the filter will call the chain. + * interface client the filter will call the chain. The {@link XsrfProtectionFilter} is disabled by default and can be + * enabled with {@link ScmConfiguration#setEnabledXsrfProtection(boolean)}. * * TODO for scm-manager 2 we have to store the csrf token as part of the jwt token instead of session. * @@ -73,10 +76,30 @@ public final class XsrfProtectionFilter extends HttpFilter * Key used for session, header and cookie. */ static final String KEY = "X-XSRF-Token"; + + @Inject + public XsrfProtectionFilter(ScmConfiguration configuration) + { + this.configuration = configuration; + } @Override protected void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException + { + if (configuration.isEnabledXsrfProtection()) + { + doXsrfProtection(request, response, chain); + } + else + { + logger.trace("xsrf protection is disabled, skipping check"); + chain.doFilter(request, response); + } + } + + private void doXsrfProtection(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws + IOException, ServletException { HttpSession session = request.getSession(true); String storedToken = (String) session.getAttribute(KEY); @@ -109,4 +132,5 @@ public final class XsrfProtectionFilter extends HttpFilter return UUID.randomUUID().toString(); } + private ScmConfiguration configuration; } diff --git a/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java b/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java index 2cb187c24d..0a2145de04 100644 --- a/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java +++ b/scm-webapp/src/test/java/sonia/scm/security/XsrfProtectionFilterTest.java @@ -44,8 +44,10 @@ import org.junit.Before; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; +import org.mockito.Mockito; import static org.mockito.Mockito.*; import org.mockito.runners.MockitoJUnitRunner; +import sonia.scm.config.ScmConfiguration; import sonia.scm.util.HttpUtil; /** @@ -68,7 +70,9 @@ public class XsrfProtectionFilterTest { @Mock private FilterChain chain; - private final XsrfProtectionFilter filter = new XsrfProtectionFilter(); + private final ScmConfiguration configuration = new ScmConfiguration(); + + private final XsrfProtectionFilter filter = new XsrfProtectionFilter(configuration); /** * Prepare mocks for testing. @@ -77,6 +81,7 @@ public class XsrfProtectionFilterTest { public void setUp(){ when(request.getSession(true)).thenReturn(session); when(request.getContextPath()).thenReturn("/scm"); + configuration.setEnabledXsrfProtection(true); } /** @@ -91,6 +96,31 @@ public class XsrfProtectionFilterTest { filter.doFilter(request, response, chain); verify(chain).doFilter(request, response); } + + /** + * Test filter method with disabled xsrf protection. + * + * @throws IOException + * @throws ServletException + */ + @Test + public void testDoFilterWithDisabledXsrfProtection() throws IOException, ServletException + { + // disable xsrf protection + configuration.setEnabledXsrfProtection(false); + + // set webui user-agent + when(request.getHeader(HttpUtil.HEADER_SCM_CLIENT)).thenReturn(HttpUtil.SCM_CLIENT_WUI); + + // call the filter + filter.doFilter(request, response, chain); + + // verify that no xsrf other any other cookie was set + verify(response, never()).addCookie(Mockito.any(Cookie.class)); + + // ensure filter chain is called + verify(chain).doFilter(request, response); + } /** * Test filter method for first web interface request. From 9dce816fb634d7444d379468801c60eaaf602dfd Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 24 May 2016 21:25:14 +0200 Subject: [PATCH 3/4] added checkbox to enable xsrf protection --- .../resources/js/config/sonia.config.scmconfigpanel.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scm-webapp/src/main/webapp/resources/js/config/sonia.config.scmconfigpanel.js b/scm-webapp/src/main/webapp/resources/js/config/sonia.config.scmconfigpanel.js index 29efa3fa00..a985688a3e 100644 --- a/scm-webapp/src/main/webapp/resources/js/config/sonia.config.scmconfigpanel.js +++ b/scm-webapp/src/main/webapp/resources/js/config/sonia.config.scmconfigpanel.js @@ -102,6 +102,10 @@ Sonia.config.ScmConfigPanel = Ext.extend(Sonia.config.ConfigPanel,{ forceBaseUrlHelpText: 'Redirects to the base url if the request comes from a other url', disableGroupingGridHelpText: 'Disable repository Groups. A complete page reload is required after a change of this value.', enableRepositoryArchiveHelpText: 'Enable repository archives. A complete page reload is required after a change of this value.', + + // TODO i18n + enableXsrfProtectionText: 'Enable Xsrf Protection', + enableXsrfProtectionHelpText: 'Enable Xsrf Cookie Protection. Note: This feature is still experimental.', initComponent: function(){ @@ -166,6 +170,12 @@ Sonia.config.ScmConfigPanel = Ext.extend(Sonia.config.ConfigPanel,{ name: 'skip-failed-authenticators', inputValue: 'true', helpText: this.skipFailedAuthenticatorsHelpText + },{ + xtype: 'checkbox', + fieldLabel: this.enableXsrfProtectionText, + name: 'xsrf-protection', + inputValue: 'true', + helpText: this.enableXsrfProtectionHelpText },{ xtype: 'numberfield', fieldLabel: this.loginAttemptLimitText, From 4a7fe345ecddfe3c1b81d260f8c4f8b368a7c344 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 24 May 2016 21:26:01 +0200 Subject: [PATCH 4/4] close branch issue-793