This is an automated email from the ASF dual-hosted git repository. radu pushed a commit to branch issue/SLING-7741 in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-xss.git
commit 634fdd2f75c7c331eab3d8143607f80dbc2db28b Author: Radu Cotescu <[email protected]> AuthorDate: Tue Jun 26 18:04:30 2018 +0200 SLING-7741 - org.apache.sling.xss.impl.XSSAPIImpl#getValidHref doesn't correctly handle the ":" character in URL fragments * modified mangleNamespaces function to only perform namespace mangling for paths * extended onsiteURL regex to accept a colon character in fragments * removed redundancy from offsiteURL regex --- .../java/org/apache/sling/xss/impl/XSSAPIImpl.java | 70 ++++++++++++---------- .../org/apache/sling/xss/impl/XSSFilterImpl.java | 6 +- src/main/resources/SLING-INF/content/config.xml | 4 +- .../org/apache/sling/xss/impl/XSSAPIImplTest.java | 28 ++++++++- 4 files changed, 69 insertions(+), 39 deletions(-) diff --git a/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java b/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java index fe6c299..746de22 100644 --- a/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java +++ b/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java @@ -18,6 +18,8 @@ package org.apache.sling.xss.impl; import java.io.StringReader; import java.io.StringWriter; +import java.net.URI; +import java.net.URISyntaxException; import java.util.HashMap; import java.util.Map; import java.util.regex.Matcher; @@ -181,29 +183,11 @@ public class XSSAPIImpl implements XSSAPI { private static final String MANGLE_NAMESPACE_IN_PREFIX = "/_"; - private static final String SCHEME_PATTERN = "://"; - - private String mangleNamespaces(String absPath) { - if (absPath != null) { - // check for absolute urls - final int schemeIndex = absPath.indexOf(SCHEME_PATTERN); - final String manglePath; - final String prefix; - if (schemeIndex != -1) { - final int pathIndex = absPath.indexOf("/", schemeIndex + 3); - if (pathIndex != -1) { - prefix = absPath.substring(0, pathIndex); - manglePath = absPath.substring(pathIndex); - } else { - prefix = absPath; - manglePath = ""; - } - } else { - prefix = ""; - manglePath = absPath; - } - if (manglePath.contains(MANGLE_NAMESPACE_OUT_SUFFIX)) { - final Matcher m = MANGLE_NAMESPACE_PATTERN.matcher(manglePath); + private URI mangleNamespaces(URI uri) { + String mangledPath = null; + if (uri.getPath() != null) { + if (uri.getRawPath().contains(MANGLE_NAMESPACE_OUT_SUFFIX)) { + final Matcher m = MANGLE_NAMESPACE_PATTERN.matcher(uri.getRawPath()); final StringBuffer buf = new StringBuffer(); while (m.find()) { @@ -212,13 +196,17 @@ public class XSSAPIImpl implements XSSAPI { } m.appendTail(buf); - - absPath = prefix + buf.toString(); - + mangledPath = buf.toString(); } } - - return absPath; + if (mangledPath != null) { + try { + return new URI(uri.getScheme(), uri.getUserInfo(), uri.getHost(), uri.getPort(), mangledPath, uri.getRawQuery(), uri.getRawFragment()); + } catch (URISyntaxException e) { + LOGGER.warn("Invalid URI.", e); + } + } + return uri; } /** @@ -237,14 +225,32 @@ public class XSSAPIImpl implements XSSAPI { .replaceAll("<", "%3C") .replaceAll("`", "%60") .replaceAll(" ", "%20"); - int qMarkIx = encodedUrl.indexOf('?'); - if (qMarkIx > 0) { - encodedUrl = encodedUrl.substring(0, qMarkIx) + encodedUrl.substring(qMarkIx).replaceAll(":", "%3A"); + URI mangledURI = null; + try { + mangledURI = mangleNamespaces(new URI(encodedUrl)); + } catch (URISyntaxException e) { + LOGGER.warn("Invalid URI.", e); + } + if (mangledURI != null) { + StringBuilder uriBuilder = new StringBuilder(); + if (StringUtils.isNotEmpty(mangledURI.getScheme()) && StringUtils.isNotEmpty(mangledURI.getAuthority())) { + uriBuilder.append(mangledURI.getScheme()).append("://").append(mangledURI.getRawAuthority()); + } + if (StringUtils.isNotEmpty(mangledURI.getPath())) { + uriBuilder.append(mangledURI.getRawPath()); + } + if (StringUtils.isNotEmpty(mangledURI.getQuery())) { + uriBuilder.append("?").append(mangledURI.getRawQuery().replaceAll(":", "%3A")); + } + if (StringUtils.isNotEmpty(mangledURI.getFragment())) { + uriBuilder.append("#").append(mangledURI.getRawFragment()); + } + encodedUrl = uriBuilder.toString(); } - encodedUrl = mangleNamespaces(encodedUrl); if (xssFilter.isValidHref(encodedUrl)) { return encodedUrl; } + } // fall through to empty string return ""; diff --git a/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java b/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java index b155d49..2df2ec4 100644 --- a/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java +++ b/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java @@ -72,8 +72,8 @@ public class XSSFilterImpl implements XSSFilter, ResourceChangeListener, Externa static final Attribute DEFAULT_HREF_ATTRIBUTE = new Attribute( "href", Arrays.asList( - Pattern.compile("([\\p{L}\\p{M}*+\\p{N}\\\\\\.\\#@\\$%\\+&;\\-_~,\\?=/!\\*\\(\\)]*|\\#(\\w)+)"), - Pattern.compile("(\\s)*((ht|f)tp(s?)://|mailto:)[\\p{L}\\p{M}*+\\p{N}]+[\\p{L}\\p{M}*+\\p{N}\\p{Zs}\\.\\#@\\$%\\+&;:\\-_~,\\?=/!\\*\\(\\)]*(\\s)*") + Pattern.compile("([\\p{L}\\p{M}\\p{N}#@$%+&;\\-_~,?=/!*().\\\\]*(#(\\w|:)+)?)"), + Pattern.compile("(\\s)*((ht|f)tp(s?)://|mailto:)[\\p{L}\\p{M}\\p{N}]+[\\p{L}\\p{M}\\p{N}\\p{Zs}.#@$%+&;:\\-_~,?=/!*()]*(\\s)*") ), Collections.<String>emptyList(), "removeAttribute", "" @@ -150,7 +150,7 @@ public class XSSFilterImpl implements XSSFilter, ResourceChangeListener, Externa // Same logic as in org.owasp.validator.html.scan.MagicSAXFilter.startElement() boolean isValid = hrefAttribute.containsAllowedValue(url.toLowerCase()); if (!isValid) { - isValid = hrefAttribute.matchesAllowedExpression(url); + isValid = hrefAttribute.matchesAllowedExpression(url.toLowerCase()); } return isValid; } diff --git a/src/main/resources/SLING-INF/content/config.xml b/src/main/resources/SLING-INF/content/config.xml index f71b704..2dbfabd 100644 --- a/src/main/resources/SLING-INF/content/config.xml +++ b/src/main/resources/SLING-INF/content/config.xml @@ -67,8 +67,8 @@ http://www.w3.org/TR/html401/struct/global.html <regexp name="htmlClass" value="[a-zA-Z0-9\s,\-_]+"/> <!-- Allow empty URL attributes with a '*'-quantifier instead of '+' for the first part of the regexp --> - <regexp name="onsiteURL" value="([\p{L}\p{M}*+\p{N}\\\.\#@\$%\+&;\-_~,\?=/!\*\(\)]*|\#(\w)+)"/> - <regexp name="offsiteURL" value="(\s)*((ht|f)tp(s?)://|mailto:)[\p{L}\p{M}*+\p{N}]+[\p{L}\p{M}*+\p{N}\p{Zs}\.\#@\$%\+&;:\-_~,\?=/!\*\(\)]*(\s)*"/> + <regexp name="onsiteURL" value="([\p{L}\p{M}\p{N}#@$%+&;\-_~,?=/!*().\\]*(#(\w|:)+)?)"/> + <regexp name="offsiteURL" value="(\s)*((ht|f)tp(s?)://|mailto:)[\p{L}\p{M}\p{N}]+[\p{L}\p{M}\p{N}\p{Zs}.#@$%+&;:\-_~,?=/!*()]*(\s)*"/> <regexp name="boolean" value="(true|false)"/> <regexp name="singlePrintable" value="[a-zA-Z0-9]{1}"/> diff --git a/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java b/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java index 889fc1c..ee7ae2f 100644 --- a/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java +++ b/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java @@ -19,10 +19,14 @@ package org.apache.sling.xss.impl; import java.io.FileInputStream; import java.io.InputStream; import java.lang.reflect.Field; +import java.util.regex.Pattern; +import org.apache.commons.lang.StringEscapeUtils; +import org.apache.commons.lang3.StringUtils; import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.xss.XSSAPI; +import org.apache.sling.xss.XSSFilter; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -35,6 +39,7 @@ import org.powermock.reflect.Whitebox; import junit.framework.TestCase; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Matchers.anyString; import static org.mockito.Mockito.mock; @@ -286,14 +291,33 @@ public class XSSAPIImplTest { { // ? in query string "/test/search.html?0_tag:id=test?ing&1_tag:id=abc", "/test/search.html?0_tag%3Aid=test?ing&1_tag%3Aid=abc", + }, + { + "/test/search.html?0_tag:id=test?ing&1_tag:id=abc#fragment:test", + "/test/search.html?0_tag%3Aid=test?ing&1_tag%3Aid=abc#fragment:test", + }, + { + "https://sling.apache.org/?a=1#fragment:test";, + "https://sling.apache.org/?a=1#fragment:test"; + }, + { + "https://sling.apache.org/#fragment:test";, + "https://sling.apache.org/#fragment:test"; } }; + StringBuilder errors = new StringBuilder(); for (String[] aTestData : testData) { String href = aTestData[0]; String expected = aTestData[1]; - - TestCase.assertEquals("Requested '" + href + "'", expected, xssAPI.getValidHref(href)); + String result = xssAPI.getValidHref(href); + if (!expected.equals(result)) { + errors.append("Requested '").append(href).append("'\nGot '").append(result).append("'\nExpected '").append(expected).append("'\n\n"); + } + } + if (errors.length() > 0) { + errors.insert(0, "\n"); + TestCase.fail(errors.toString()); } }
