Return-Path: X-Original-To: apmail-cxf-commits-archive@www.apache.org Delivered-To: apmail-cxf-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id D7DD218ABE for ; Mon, 18 Jan 2016 14:18:13 +0000 (UTC) Received: (qmail 65133 invoked by uid 500); 18 Jan 2016 14:18:13 -0000 Delivered-To: apmail-cxf-commits-archive@cxf.apache.org Received: (qmail 65055 invoked by uid 500); 18 Jan 2016 14:18:13 -0000 Mailing-List: contact commits-help@cxf.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@cxf.apache.org Delivered-To: mailing list commits@cxf.apache.org Received: (qmail 64918 invoked by uid 99); 18 Jan 2016 14:18:13 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 18 Jan 2016 14:18:13 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 67A9BE0484; Mon, 18 Jan 2016 14:18:13 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: coheigea@apache.org To: commits@cxf.apache.org Date: Mon, 18 Jan 2016 14:18:14 -0000 Message-Id: <9a7160ab630649399c5eaac1ce7b7d28@git.apache.org> In-Reply-To: <068a94a07fe74e8da078a1d3f1100e2f@git.apache.org> References: <068a94a07fe74e8da078a1d3f1100e2f@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [2/6] cxf-fediz git commit: Fixing redirection attack issue Fixing redirection attack issue Project: http://git-wip-us.apache.org/repos/asf/cxf-fediz/repo Commit: http://git-wip-us.apache.org/repos/asf/cxf-fediz/commit/fd8dcda5 Tree: http://git-wip-us.apache.org/repos/asf/cxf-fediz/tree/fd8dcda5 Diff: http://git-wip-us.apache.org/repos/asf/cxf-fediz/diff/fd8dcda5 Branch: refs/heads/1.2.x-fixes Commit: fd8dcda574889e1d1e7021072f47c71f2e38e4db Parents: 8c99b2f Author: Colm O hEigeartaigh Authored: Fri Nov 27 17:10:51 2015 +0000 Committer: Colm O hEigeartaigh Committed: Mon Jan 18 13:43:40 2016 +0000 ---------------------------------------------------------------------- .../service/idp/beans/STSClientAction.java | 25 +++++++++++ .../fediz/service/idp/domain/Application.java | 23 +++++++++- .../idp/service/jpa/ApplicationDAOJPAImpl.java | 2 + .../idp/service/jpa/ApplicationEntity.java | 11 +++++ .../idp/src/main/resources/entities-realma.xml | 2 + .../src/test/resources/entities-realma.xml | 2 + .../test/resources/realma/entities-realma.xml | 2 + .../fediz/integrationtests/AbstractTests.java | 44 +++----------------- .../test/resources/realma/entities-realma.xml | 2 + 9 files changed, 74 insertions(+), 39 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/beans/STSClientAction.java ---------------------------------------------------------------------- diff --git a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/beans/STSClientAction.java b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/beans/STSClientAction.java index 948c557..1e316b1 100644 --- a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/beans/STSClientAction.java +++ b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/beans/STSClientAction.java @@ -25,6 +25,7 @@ import java.net.MalformedURLException; import java.net.URL; import java.security.cert.X509Certificate; import java.util.List; +import java.util.regex.Matcher; import javax.servlet.http.HttpServletRequest; import javax.xml.namespace.QName; @@ -199,6 +200,9 @@ public class STSClientAction { throw new ProcessingException(TYPE.BAD_REQUEST); } + // Check wreply parameter against passive requestor endpoint constraint + validateApplicationEndpoint(serviceConfig, context); + // Parse wreq parameter - we only support parsing TokenType and KeyType for now String wreq = (String)WebUtils.getAttributeFromFlowScope(context, FederationConstants.PARAM_REQUEST); String stsTokenType = null; @@ -299,6 +303,27 @@ public class STSClientAction { return StringEscapeUtils.escapeXml11(rpToken); } + // The wreply address must match the passive endpoint requestor constraint (if it is specified) + private void validateApplicationEndpoint(Application serviceConfig, RequestContext context) + throws ProcessingException { + if (serviceConfig.getCompiledPassiveRequestorEndpointConstraint() == null) { + LOG.info("No passive requestor endpoint constraint is configured for the application. " + + "This could lead to a malicious redirection attack"); + return; + } + + String wreply = + (String)WebUtils.getAttributeFromFlowScope(context, FederationConstants.PARAM_REPLY); + if (wreply != null) { + Matcher matcher = serviceConfig.getCompiledPassiveRequestorEndpointConstraint().matcher(wreply); + if (!matcher.matches()) { + LOG.error("The wreply value of {} does not match any of the passive requestor values", + wreply); + throw new ProcessingException(TYPE.BAD_REQUEST); + } + } + } + private String getIdFromToken(String token) throws XMLStreamException { InputStream is = new ByteArrayInputStream(token.getBytes()); Document doc = StaxUtils.read(is); http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/domain/Application.java ---------------------------------------------------------------------- diff --git a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/domain/Application.java b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/domain/Application.java index 5f14f5b..43c7e8a 100644 --- a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/domain/Application.java +++ b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/domain/Application.java @@ -22,6 +22,7 @@ import java.io.Serializable; import java.net.URI; import java.util.ArrayList; import java.util.List; +import java.util.regex.Pattern; import javax.xml.bind.annotation.XmlAttribute; import javax.xml.bind.annotation.XmlElementRef; @@ -32,7 +33,7 @@ import javax.xml.bind.annotation.XmlType; @XmlRootElement(name = "application", namespace = "http://org.apache.cxf.fediz/") @XmlType(propOrder = {"realm", "role", "serviceDisplayName", "serviceDescription", "protocol", "tokenType", "lifeTime", "encryptionCertificate", "requestedClaims", - "policyNamespace", "passiveRequestorEndpoint", "id" }) + "policyNamespace", "passiveRequestorEndpoint", "passiveRequestorEndpointConstraint", "id" }) public class Application implements Serializable { private static final long serialVersionUID = 5644327504861846964L; @@ -86,6 +87,10 @@ public class Application implements Serializable { //fed:ApplicationServiceType, fed:SecurityTokenServiceType private String passiveRequestorEndpoint; + // A regular expression constraint on the passiveRequestorEndpoint + private String passiveRequestorEndpointConstraint; + private Pattern compiledPassiveRequestorEndpointConstraint; + @XmlAttribute public int getId() { @@ -195,4 +200,20 @@ public class Application implements Serializable { this.passiveRequestorEndpoint = passiveRequestorEndpoint; } + public String getPassiveRequestorEndpointConstraint() { + return passiveRequestorEndpointConstraint; + } + + public void setPassiveRequestorEndpointConstraint(String passiveRequestorEndpointConstraint) { + this.passiveRequestorEndpointConstraint = passiveRequestorEndpointConstraint; + if (passiveRequestorEndpointConstraint != null) { + compiledPassiveRequestorEndpointConstraint = Pattern.compile(passiveRequestorEndpointConstraint); + } else { + compiledPassiveRequestorEndpointConstraint = null; + } + } + + public Pattern getCompiledPassiveRequestorEndpointConstraint() { + return compiledPassiveRequestorEndpointConstraint; + } } http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationDAOJPAImpl.java ---------------------------------------------------------------------- diff --git a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationDAOJPAImpl.java b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationDAOJPAImpl.java index 0cebe38..4829764 100644 --- a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationDAOJPAImpl.java +++ b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationDAOJPAImpl.java @@ -200,6 +200,7 @@ public class ApplicationDAOJPAImpl implements ApplicationDAO { entity.setTokenType(application.getTokenType()); entity.setPolicyNamespace(application.getPolicyNamespace()); entity.setPassiveRequestorEndpoint(application.getPassiveRequestorEndpoint()); + entity.setPassiveRequestorEndpointConstraint(application.getPassiveRequestorEndpointConstraint()); } public static Application entity2domain(ApplicationEntity entity, List expandList) { @@ -215,6 +216,7 @@ public class ApplicationDAOJPAImpl implements ApplicationDAO { application.setTokenType(entity.getTokenType()); application.setPolicyNamespace(entity.getPolicyNamespace()); application.setPassiveRequestorEndpoint(entity.getPassiveRequestorEndpoint()); + application.setPassiveRequestorEndpointConstraint(entity.getPassiveRequestorEndpointConstraint()); if (expandList != null && (expandList.contains("all") || expandList.contains("claims"))) { for (ApplicationClaimEntity item : entity.getRequestedClaims()) { http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationEntity.java ---------------------------------------------------------------------- diff --git a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationEntity.java b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationEntity.java index c6af8b2..e450132 100644 --- a/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationEntity.java +++ b/services/idp/src/main/java/org/apache/cxf/fediz/service/idp/service/jpa/ApplicationEntity.java @@ -81,6 +81,9 @@ public class ApplicationEntity { private String policyNamespace; private String passiveRequestorEndpoint; + + // A regular expression constraint on the passiveRequestorEndpoint + private String passiveRequestorEndpointConstraint; public int getId() { @@ -178,5 +181,13 @@ public class ApplicationEntity { public void setPassiveRequestorEndpoint(String passiveRequestorEndpoint) { this.passiveRequestorEndpoint = passiveRequestorEndpoint; } + + public String getPassiveRequestorEndpointConstraint() { + return passiveRequestorEndpointConstraint; + } + + public void setPassiveRequestorEndpointConstraint(String passiveRequestorEndpointConstraint) { + this.passiveRequestorEndpointConstraint = passiveRequestorEndpointConstraint; + } } http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/services/idp/src/main/resources/entities-realma.xml ---------------------------------------------------------------------- diff --git a/services/idp/src/main/resources/entities-realma.xml b/services/idp/src/main/resources/entities-realma.xml index e28aa52..b76bf66 100644 --- a/services/idp/src/main/resources/entities-realma.xml +++ b/services/idp/src/main/resources/entities-realma.xml @@ -104,6 +104,8 @@ + http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/systests/federation/samlsso/src/test/resources/entities-realma.xml ---------------------------------------------------------------------- diff --git a/systests/federation/samlsso/src/test/resources/entities-realma.xml b/systests/federation/samlsso/src/test/resources/entities-realma.xml index 04d9d52..86fd540 100644 --- a/systests/federation/samlsso/src/test/resources/entities-realma.xml +++ b/systests/federation/samlsso/src/test/resources/entities-realma.xml @@ -110,6 +110,8 @@ + http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/systests/federation/wsfed/src/test/resources/realma/entities-realma.xml ---------------------------------------------------------------------- diff --git a/systests/federation/wsfed/src/test/resources/realma/entities-realma.xml b/systests/federation/wsfed/src/test/resources/realma/entities-realma.xml index a3e1a36..4cca3ec 100644 --- a/systests/federation/wsfed/src/test/resources/realma/entities-realma.xml +++ b/systests/federation/wsfed/src/test/resources/realma/entities-realma.xml @@ -98,6 +98,8 @@ + http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/systests/tests/src/test/java/org/apache/cxf/fediz/integrationtests/AbstractTests.java ---------------------------------------------------------------------- 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 88ab429..de8efbf 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 @@ -19,21 +19,20 @@ package org.apache.cxf.fediz.integrationtests; +import java.net.URLEncoder; + import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.Node; import com.gargoylesoftware.htmlunit.CookieManager; import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException; -import com.gargoylesoftware.htmlunit.HttpMethod; import com.gargoylesoftware.htmlunit.WebClient; -import com.gargoylesoftware.htmlunit.WebRequest; import com.gargoylesoftware.htmlunit.html.DomElement; import com.gargoylesoftware.htmlunit.html.DomNodeList; import com.gargoylesoftware.htmlunit.html.HtmlForm; import com.gargoylesoftware.htmlunit.html.HtmlPage; import com.gargoylesoftware.htmlunit.html.HtmlSubmitInput; -import com.gargoylesoftware.htmlunit.util.NameValuePair; import com.gargoylesoftware.htmlunit.xml.XmlPage; import java.net.URL; @@ -620,43 +619,12 @@ public abstract class AbstractTests { new UsernamePasswordCredentials(user, password)); webClient2.getOptions().setJavaScriptEnabled(false); - final HtmlPage idpPage = webClient2.getPage(idpUrl); - webClient2.getOptions().setJavaScriptEnabled(true); - Assert.assertEquals("IDP SignIn Response Form", idpPage.getTitleText()); - - // Check that the form is to be posted to the malicious URL - DomNodeList formResults = idpPage.getElementsByTagName("form"); - Assert.assertTrue(formResults.size() == 1); - Assert.assertEquals(formResults.get(0).getAttributeNS(null, "action"), maliciousURL); - - // Parse the form to get the token (wresult) - DomNodeList results = idpPage.getElementsByTagName("input"); - - String wresult = null; - for (DomElement result : results) { - if ("wresult".equals(result.getAttributeNS(null, "name"))) { - wresult = result.getAttributeNS(null, "value"); - } - } - Assert.assertNotNull(wresult); - - // 4. Now the malicious user has the token. Try to invoke on the endpoint - final WebClient webClient3 = new WebClient(); - webClient3.getOptions().setUseInsecureSSL(true); - - WebRequest requestSettings = new WebRequest(new URL(url), HttpMethod.POST); - - requestSettings.setRequestParameters(new ArrayList()); - requestSettings.getRequestParameters().add(new NameValuePair("wa", "wsignin1.0")); - requestSettings.getRequestParameters().add(new NameValuePair("wresult", wresult)); - requestSettings.getRequestParameters().add(new NameValuePair("wtrealm", - "urn:org:apache:cxf:fediz:fedizhelloworld")); - try { - rpPage = webClient3.getPage(url); - Assert.fail("Exception expected"); + webClient2.getPage(idpUrl); + Assert.fail("Failure expected on a bad wreply address"); } catch (FailingHttpStatusCodeException ex) { - Assert.assertEquals(ex.getStatusCode(), 401); + Assert.assertEquals(ex.getStatusCode(), 400); } } + } http://git-wip-us.apache.org/repos/asf/cxf-fediz/blob/fd8dcda5/systests/tomcat7/src/test/resources/realma/entities-realma.xml ---------------------------------------------------------------------- diff --git a/systests/tomcat7/src/test/resources/realma/entities-realma.xml b/systests/tomcat7/src/test/resources/realma/entities-realma.xml index 88a3c67..f947274 100644 --- a/systests/tomcat7/src/test/resources/realma/entities-realma.xml +++ b/systests/tomcat7/src/test/resources/realma/entities-realma.xml @@ -104,6 +104,8 @@ +