Author: radu Date: Mon Jun 15 08:04:53 2015 New Revision: 1685504 URL: http://svn.apache.org/r1685504 Log: SLING-4584 - Performance: XSSAPI.getValidHref should not be based on HTML filtering
* Instead of parsing a constructed temporary anchor tag, skip the HTML parsing and validate the URL directly for better performance. (patch contributed by Joel Richard <joelr...@adobe.com>) Added: sling/trunk/bundles/extensions/xss/src/test/resources/ sling/trunk/bundles/extensions/xss/src/test/resources/configWithoutHref.xml Modified: sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/XSSFilter.java sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/package-info.java sling/trunk/bundles/extensions/xss/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java Modified: sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/XSSFilter.java URL: http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/XSSFilter.java?rev=1685504&r1=1685503&r2=1685504&view=diff ============================================================================== --- sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/XSSFilter.java (original) +++ sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/XSSFilter.java Mon Jun 15 08:04:53 2015 @@ -59,4 +59,15 @@ public interface XSSFilter { * @throws NullPointerException if context is <code>null</code> */ String filter(ProtectionContext context, String src); + + /** + * Checks if the given URL is valid to be used for the <code>href</code> attribute in a <code>a</code> tag. + * <p/> + * The default protection context is used for checking. + * + * @param url the URL that should be validated + * @return true if the URL is violation-free + */ + boolean isValidHref(String url); + } Modified: sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java URL: http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java?rev=1685504&r1=1685503&r2=1685504&view=diff ============================================================================== --- sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java (original) +++ sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSAPIImpl.java Mon Jun 15 08:04:53 2015 @@ -207,14 +207,10 @@ public class XSSAPIImpl implements XSSAP if (qMarkIx > 0) { encodedUrl = encodedUrl.substring(0, qMarkIx) + encodedUrl.substring(qMarkIx).replaceAll(":", "%3A"); } - String testHtml = LINK_PREFIX + mangleNamespaces(encodedUrl) + LINK_SUFFIX; - // replace all & with & because filterHTML will also apply this encoding - testHtml = testHtml.replaceAll("&(?!amp)", "&"); - final String safeHtml = xssFilter.filter(ProtectionContext.HTML_HTML_CONTENT, testHtml); - // if the xssFilter didn't like the input string we just return "" - // otherwise we return the mangled url without encoding - if (safeHtml.equals(testHtml)) { - return mangleNamespaces(encodedUrl); + + encodedUrl = mangleNamespaces(encodedUrl); + if (xssFilter.isValidHref(encodedUrl)) { + return encodedUrl; } } Modified: sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java URL: http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java?rev=1685504&r1=1685503&r2=1685504&view=diff ============================================================================== --- sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java (original) +++ sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/impl/XSSFilterImpl.java Mon Jun 15 08:04:53 2015 @@ -17,8 +17,11 @@ package org.apache.sling.xss.impl; import java.io.InputStream; +import java.util.Arrays; +import java.util.Collections; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.regex.Pattern; import org.apache.felix.scr.annotations.Activate; import org.apache.felix.scr.annotations.Component; @@ -35,6 +38,8 @@ import org.apache.sling.xss.XSSFilter; import org.osgi.service.event.Event; import org.osgi.service.event.EventConstants; import org.osgi.service.event.EventHandler; +import org.owasp.validator.html.model.Attribute; +import org.owasp.validator.html.model.Tag; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -49,9 +54,21 @@ public class XSSFilterImpl implements XS private static final Logger LOGGER = LoggerFactory.getLogger(XSSFilterImpl.class); + // Default href configuration copied from the config.xml supplied with AntiSamy + static final Attribute DEFAULT_HREF_ATTRIBUTE = new Attribute( + "href", + Arrays.asList( + Pattern.compile("([\\p{L}\\p{N}\\\\\\.\\#@\\$%\\+&;\\-_~,\\?=/!\\*\\(\\)]*|\\#(\\w)+)"), + Pattern.compile("(\\s)*((ht|f)tp(s?)://|mailto:)[\\p{L}\\p{N}]+[\\p{L}\\p{N}\\p{Zs}\\.\\#@\\$%\\+&;:\\-_~,\\?=/!\\*\\(\\)]*(\\s)*") + ), + Collections.<String>emptyList(), + "removeAttribute", "" + ); + private static final String DEFAULT_POLICY_PATH = "sling/xss/config.xml"; private static final int DEFAULT_POLICY_CACHE_SIZE = 128; private PolicyHandler defaultHandler; + private Attribute hrefAttribute; // available contexts private final XSSFilterRule htmlHtmlContext = new HtmlToHtmlContentContext(); @@ -104,7 +121,7 @@ public class XSSFilterImpl implements XS if (policyStream != null) { try { if (defaultHandler == null) { - defaultHandler = new PolicyHandler(policyStream); + setDefaultHandler(new PolicyHandler(policyStream)); policyStream.close(); } } catch (Exception e) { @@ -119,7 +136,7 @@ public class XSSFilterImpl implements XS if (policyStream != null) { try { if (defaultHandler == null) { - defaultHandler = new PolicyHandler(policyStream); + setDefaultHandler(new PolicyHandler(policyStream)); policyStream.close(); } } catch (Exception e) { @@ -185,7 +202,19 @@ public class XSSFilterImpl implements XS @SuppressWarnings("unused") public void setDefaultPolicy(InputStream policyStream) throws Exception { - defaultHandler = new PolicyHandler(policyStream); + setDefaultHandler(new PolicyHandler(policyStream)); + } + + private void setDefaultHandler(PolicyHandler defaultHandler) { + Tag linkTag = defaultHandler.getPolicy().getTagByLowercaseName("a"); + Attribute hrefAttribute = (linkTag != null) ? linkTag.getAttributeByName("href") : null; + if (hrefAttribute == null) { + // Fallback to default configuration + hrefAttribute = DEFAULT_HREF_ATTRIBUTE; + } + + this.defaultHandler = defaultHandler; + this.hrefAttribute = hrefAttribute; } @SuppressWarnings("unused") @@ -210,4 +239,15 @@ public class XSSFilterImpl implements XS public boolean hasPolicy(String policyName) { return policies.containsKey(policyName); } + + @Override + public boolean isValidHref(String url) { + // Same logic as in org.owasp.validator.html.scan.MagicSAXFilter.startElement() + boolean isValid = hrefAttribute.containsAllowedValue(url.toLowerCase()); + if (!isValid) { + isValid = hrefAttribute.matchesAllowedExpression(url); + } + return isValid; + } + } Modified: sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/package-info.java URL: http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/package-info.java?rev=1685504&r1=1685503&r2=1685504&view=diff ============================================================================== --- sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/package-info.java (original) +++ sling/trunk/bundles/extensions/xss/src/main/java/org/apache/sling/xss/package-info.java Mon Jun 15 08:04:53 2015 @@ -17,9 +17,9 @@ /** * XSS Protection Service * - * @version 1.1.0 + * @version 1.2.0 */ -@Version("1.1.0") +@Version("1.2.0") package org.apache.sling.xss; import aQute.bnd.annotation.Version; Modified: sling/trunk/bundles/extensions/xss/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java URL: http://svn.apache.org/viewvc/sling/trunk/bundles/extensions/xss/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java?rev=1685504&r1=1685503&r2=1685504&view=diff ============================================================================== --- sling/trunk/bundles/extensions/xss/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java (original) +++ sling/trunk/bundles/extensions/xss/src/test/java/org/apache/sling/xss/impl/XSSAPIImplTest.java Mon Jun 15 08:04:53 2015 @@ -23,12 +23,14 @@ import java.lang.reflect.Field; import org.apache.sling.api.SlingHttpServletRequest; import org.apache.sling.api.resource.ResourceResolver; import org.apache.sling.xss.XSSAPI; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; import org.mockito.stubbing.Answer; import org.owasp.validator.html.AntiSamy; import org.owasp.validator.html.Policy; +import org.owasp.validator.html.model.Attribute; import org.powermock.reflect.Whitebox; import junit.framework.TestCase; @@ -49,16 +51,8 @@ public class XSSAPIImplTest { @Before public void setup() { try { - InputStream policyStream = new FileInputStream("./src/main/resources/SLING-INF/content/config.xml"); - Policy policy = Policy.getInstance(policyStream); - AntiSamy antiSamy = new AntiSamy(policy); - - PolicyHandler mockPolicyHandler = mock(PolicyHandler.class); - when(mockPolicyHandler.getPolicy()).thenReturn(policy); - when(mockPolicyHandler.getAntiSamy()).thenReturn(antiSamy); - XSSFilterImpl xssFilter = new XSSFilterImpl(); - Whitebox.setInternalState(xssFilter, "defaultHandler", mockPolicyHandler); + setDefaultHandler(xssFilter, "./src/main/resources/SLING-INF/content/config.xml"); xssAPI = new XSSAPIImpl(); Whitebox.invokeMethod(xssAPI, "activate"); @@ -84,6 +78,18 @@ public class XSSAPIImplTest { } } + private static void setDefaultHandler(XSSFilterImpl xssFilter, String filename) throws Exception { + InputStream policyStream = new FileInputStream(filename); + Policy policy = Policy.getInstance(policyStream); + AntiSamy antiSamy = new AntiSamy(policy); + + PolicyHandler mockPolicyHandler = mock(PolicyHandler.class); + when(mockPolicyHandler.getPolicy()).thenReturn(policy); + when(mockPolicyHandler.getAntiSamy()).thenReturn(antiSamy); + + Whitebox.invokeMethod(xssFilter, "setDefaultHandler", mockPolicyHandler); + } + @Test public void testEncodeForHTML() { String[][] testData = { @@ -276,6 +282,19 @@ public class XSSAPIImplTest { } @Test + public void testGetValidHrefWithoutHrefConfig() throws Exception { + // Load AntiSamy configuration without href filter + XSSFilterImpl xssFilter = Whitebox.getInternalState(xssAPI, "xssFilter"); + setDefaultHandler(xssFilter, "./src/test/resources/configWithoutHref.xml"); + + Attribute hrefAttribute = Whitebox.getInternalState(xssFilter, "hrefAttribute"); + Assert.assertEquals(hrefAttribute, XSSFilterImpl.DEFAULT_HREF_ATTRIBUTE); + + // Run same tests again to check default configuration + testGetValidHref(); + } + + @Test public void testGetValidInteger() { String[][] testData = { // Source Expected Result