From ccdb12b26ff89e0a998a333e84dd84bd713ac76c Mon Sep 17 00:00:00 2001 From: Colm O hEigeartaigh Date: Fri, 6 Oct 2017 16:16:19 +0100 Subject: [PATCH] Some improvements to the Spring plugins --- .../web/FederationAuthenticationFilter.java | 19 ++++--- .../web/FederationAuthenticationFilter.java | 19 ++++--- .../fediz/integrationtests/Spring3Test.java | 8 +++ .../fediz/integrationtests/SpringTest.java | 8 +++ .../fediz/integrationtests/AbstractTests.java | 53 +++++++++++++++++++ .../WEB-INF/applicationContext-security.xml | 1 + 6 files changed, 94 insertions(+), 14 deletions(-) diff --git a/plugins/spring/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java b/plugins/spring/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java index 485ca388d..49a059304 100644 --- a/plugins/spring/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java +++ b/plugins/spring/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java @@ -128,14 +128,19 @@ private String getState(ServletRequest request) { private void verifySavedState(HttpServletRequest request) { HttpSession session = request.getSession(false); - if (session != null) { - String savedContext = (String)session.getAttribute(FederationAuthenticationEntryPoint.SAVED_CONTEXT); - String state = getState(request); - if (savedContext != null && !savedContext.equals(state)) { - logger.warn("The received state does not match the state saved in the context"); - throw new BadCredentialsException("The received state does not match the state saved in the context"); - } + + if (session == null) { + logger.warn("The received state does not match the state saved in the context"); + throw new BadCredentialsException("The received state does not match the state saved in the context"); + } + + String savedContext = (String)session.getAttribute(FederationAuthenticationEntryPoint.SAVED_CONTEXT); + String state = getState(request); + if (savedContext == null || !savedContext.equals(state)) { + logger.warn("The received state does not match the state saved in the context"); + throw new BadCredentialsException("The received state does not match the state saved in the context"); } + session.removeAttribute(FederationAuthenticationEntryPoint.SAVED_CONTEXT); } /** diff --git a/plugins/spring3/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java b/plugins/spring3/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java index 485ca388d..49a059304 100644 --- a/plugins/spring3/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java +++ b/plugins/spring3/src/main/java/org/apache/cxf/fediz/spring/web/FederationAuthenticationFilter.java @@ -128,14 +128,19 @@ private String getState(ServletRequest request) { private void verifySavedState(HttpServletRequest request) { HttpSession session = request.getSession(false); - if (session != null) { - String savedContext = (String)session.getAttribute(FederationAuthenticationEntryPoint.SAVED_CONTEXT); - String state = getState(request); - if (savedContext != null && !savedContext.equals(state)) { - logger.warn("The received state does not match the state saved in the context"); - throw new BadCredentialsException("The received state does not match the state saved in the context"); - } + + if (session == null) { + logger.warn("The received state does not match the state saved in the context"); + throw new BadCredentialsException("The received state does not match the state saved in the context"); + } + + String savedContext = (String)session.getAttribute(FederationAuthenticationEntryPoint.SAVED_CONTEXT); + String state = getState(request); + if (savedContext == null || !savedContext.equals(state)) { + logger.warn("The received state does not match the state saved in the context"); + throw new BadCredentialsException("The received state does not match the state saved in the context"); } + session.removeAttribute(FederationAuthenticationEntryPoint.SAVED_CONTEXT); } /** diff --git a/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/Spring3Test.java b/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/Spring3Test.java index facc3e8b6..f59ee75e5 100644 --- a/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/Spring3Test.java +++ b/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/Spring3Test.java @@ -159,4 +159,12 @@ public void testCSRFAttack() throws Exception { csrfAttackTest(url); } + @Override + @org.junit.Test + public void testCSRFAttack2() throws Exception { + String url = "https://localhost:" + getRpHttpsPort() + "/" + getServletContextName() + + "/j_spring_fediz_security_check"; + csrfAttackTest2(url); + } + } diff --git a/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/SpringTest.java b/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/SpringTest.java index 86f2cbc95..6f8545aa3 100644 --- a/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/SpringTest.java +++ b/systests/spring/src/test/java/org/apache/cxf/fediz/integrationtests/SpringTest.java @@ -157,4 +157,12 @@ public void testCSRFAttack() throws Exception { + "/j_spring_fediz_security_check"; csrfAttackTest(url); } + + @Override + @org.junit.Test + public void testCSRFAttack2() throws Exception { + String url = "https://localhost:" + getRpHttpsPort() + "/" + getServletContextName() + + "/j_spring_fediz_security_check"; + csrfAttackTest2(url); + } } diff --git a/systests/tests/src/test/java/org/apache/cxf/fediz/integrationtests/AbstractTests.java b/systests/tests/src/test/java/org/apache/cxf/fediz/integrationtests/AbstractTests.java index 65dead1e4..a400174ae 100644 --- a/systests/tests/src/test/java/org/apache/cxf/fediz/integrationtests/AbstractTests.java +++ b/systests/tests/src/test/java/org/apache/cxf/fediz/integrationtests/AbstractTests.java @@ -799,4 +799,57 @@ protected void csrfAttackTest(String rpURL) throws Exception { } + @org.junit.Test + public void testCSRFAttack2() throws Exception { + String url = "https://localhost:" + getRpHttpsPort() + "/" + getServletContextName() + "/secure/fedservlet"; + csrfAttackTest2(url); + } + + protected void csrfAttackTest2(String rpURL) throws Exception { + String url = "https://localhost:" + getRpHttpsPort() + "/" + getServletContextName() + "/secure/fedservlet"; + + // 1. Log in as "bob" using another WebClient + WebClient webClient2 = new WebClient(); + webClient2.getOptions().setUseInsecureSSL(true); + webClient2.getCredentialsProvider().setCredentials( + new AuthScope("localhost", Integer.parseInt(getIdpHttpsPort())), + new UsernamePasswordCredentials("bob", "bob")); + + webClient2.getOptions().setJavaScriptEnabled(false); + final HtmlPage idpPage2 = webClient2.getPage(url); + webClient2.getOptions().setJavaScriptEnabled(true); + Assert.assertEquals("IDP SignIn Response Form", idpPage2.getTitleText()); + + // 2. Now instead of clicking on the form, send the form via alice's WebClient instead + + // Send with context... + WebRequest request = new WebRequest(new URL(rpURL), HttpMethod.POST); + request.setRequestParameters(new ArrayList()); + + DomNodeList results = idpPage2.getElementsByTagName("input"); + + for (DomElement result : results) { + if ("wresult".equals(result.getAttributeNS(null, "name")) + || "wa".equals(result.getAttributeNS(null, "name")) + || "wctx".equals(result.getAttributeNS(null, "name"))) { + String value = result.getAttributeNS(null, "value"); + request.getRequestParameters().add(new NameValuePair(result.getAttributeNS(null, "name"), value)); + } + } + + WebClient webClient = new WebClient(); + webClient.getOptions().setUseInsecureSSL(true); + + try { + webClient.getPage(request); + Assert.fail("Failure expected on a CSRF attack"); + } catch (FailingHttpStatusCodeException ex) { + // expected + } + + webClient.close(); + webClient2.close(); + + } + } diff --git a/systests/webapps/springWebapp/src/main/webapp/WEB-INF/applicationContext-security.xml b/systests/webapps/springWebapp/src/main/webapp/WEB-INF/applicationContext-security.xml index 68d1a5b93..c6ad4a3f4 100644 --- a/systests/webapps/springWebapp/src/main/webapp/WEB-INF/applicationContext-security.xml +++ b/systests/webapps/springWebapp/src/main/webapp/WEB-INF/applicationContext-security.xml @@ -37,6 +37,7 @@ http://www.springframework.org/schema/context http://www.springframework.org/sch +