From 69a12bb5d9c75c42d93952ef6add82e1b5205408 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Passaro?= Date: Mon, 18 Jan 2021 19:27:35 -0300 Subject: [PATCH 1/3] Add toggle for policy validation New static method on Policy to enable/disable schema validation. It's enabled by default. --- .../java/org/owasp/validator/html/Policy.java | 38 ++++++++++++++----- .../owasp/validator/html/test/PolicyTest.java | 28 +++++++++++++- 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/owasp/validator/html/Policy.java b/src/main/java/org/owasp/validator/html/Policy.java index faab2715..88de441c 100644 --- a/src/main/java/org/owasp/validator/html/Policy.java +++ b/src/main/java/org/owasp/validator/html/Policy.java @@ -120,6 +120,7 @@ public class Policy { * XML Schema for policy validation */ private static Schema schema = null; + private static boolean validateSchema = true; /** * Get the Tag specified by the provided tag name. @@ -160,6 +161,16 @@ public Property getPropertyByName(String propertyName) { return cssRules.get(propertyName.toLowerCase()); } + /** + * This can enable/disable the schema validation against AntiSamy XSD for the instantiated policies. + * It is enabled by default. + * + * @param enable Boolean value to specify if the schema validation should be performed. Use false to disable. + */ + public static void toggleSchemaValidation(boolean enable) { + validateSchema = enable; + } + /** * This retrieves a Policy based on a default location ("resources/antisamy.xml") * @@ -303,7 +314,6 @@ private static Element getTopLevelElement(InputStream is) throws PolicyException protected static Element getTopLevelElement(InputSource source) throws PolicyException { try { - getPolicySchema(); DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); @@ -314,14 +324,19 @@ protected static Element getTopLevelElement(InputSource source) throws PolicyExc dbf.setFeature(EXTERNAL_PARAM_ENTITIES, false); dbf.setFeature(DISALLOW_DOCTYPE_DECL, true); dbf.setFeature(LOAD_EXTERNAL_DTD, false); - dbf.setNamespaceAware(true); - dbf.setSchema(schema); + + if (validateSchema) { + getPolicySchema(); + dbf.setNamespaceAware(true); + dbf.setSchema(schema); + } + DocumentBuilder db = dbf.newDocumentBuilder(); db.setErrorHandler(new SAXErrorHandler()); Document dom = db.parse(source); return dom.getDocumentElement(); - } catch (SAXException | ParserConfigurationException | IOException | URISyntaxException e) { + } catch (SAXException | ParserConfigurationException | IOException e) { throw new PolicyException(e); } } @@ -385,8 +400,6 @@ private static Element getPolicy(String href, URL baseUrl) } } - getPolicySchema(); - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); /** @@ -396,8 +409,13 @@ private static Element getPolicy(String href, URL baseUrl) dbf.setFeature(EXTERNAL_PARAM_ENTITIES, false); dbf.setFeature(DISALLOW_DOCTYPE_DECL, true); dbf.setFeature(LOAD_EXTERNAL_DTD, false); - dbf.setNamespaceAware(true); - dbf.setSchema(schema); + + if (validateSchema) { + getPolicySchema(); + dbf.setNamespaceAware(true); + dbf.setSchema(schema); + } + DocumentBuilder db = dbf.newDocumentBuilder(); db.setErrorHandler(new SAXErrorHandler()); Document dom; @@ -416,7 +434,7 @@ private static Element getPolicy(String href, URL baseUrl) } return null; - } catch (SAXException | ParserConfigurationException | IOException | URISyntaxException e) { + } catch (SAXException | ParserConfigurationException | IOException e) { throw new PolicyException(e); } } @@ -906,7 +924,7 @@ public AntiSamyPattern getCommonRegularExpressions(String name) { return commonRegularExpressions.get(name); } - private static void getPolicySchema() throws URISyntaxException, SAXException { + private static void getPolicySchema() throws SAXException { if (schema == null) { InputStream schemaStream = Policy.class.getClassLoader().getResourceAsStream(POLICY_SCHEMA_URI); Source schemaSource = new StreamSource(schemaStream); diff --git a/src/test/java/org/owasp/validator/html/test/PolicyTest.java b/src/test/java/org/owasp/validator/html/test/PolicyTest.java index 5a0af270..06e20e82 100644 --- a/src/test/java/org/owasp/validator/html/test/PolicyTest.java +++ b/src/test/java/org/owasp/validator/html/test/PolicyTest.java @@ -115,7 +115,7 @@ public void testInvalidPolicies() { String policyFile = assembleFile(notSupportedTagsSection); try { policy = Policy.getInstance(new ByteArrayInputStream(policyFile.getBytes())); - fail("Not supported tag on policy, but not PolicyException occurred."); + fail("Not supported tag on policy, but no PolicyException occurred."); } catch (PolicyException e) { assertNotNull(e); } @@ -140,5 +140,31 @@ public void testInvalidPolicies() { assertNotNull(e); } } + + public void testSchemaValidationToggle() { + String notSupportedTagsSection = "\n" + + "\n"; + String policyFile = assembleFile(notSupportedTagsSection); + + // Disable validation + Policy.toggleSchemaValidation(false); + + try { + policy = Policy.getInstance(new ByteArrayInputStream(policyFile.getBytes())); + assertNotNull(policy); + } catch (PolicyException e) { + fail("Not supported tag on policy, but not PolicyException occurred."); + } + + // Enable validation again + Policy.toggleSchemaValidation(true); + + try { + policy = Policy.getInstance(new ByteArrayInputStream(policyFile.getBytes())); + fail("Not supported tag on policy, but no PolicyException occurred."); + } catch (PolicyException e) { + assertNotNull(e); + } + } } From b39d3eed7b024c7a90b73ab6d49d8493d0aaa224 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Passaro?= Date: Mon, 18 Jan 2021 19:30:30 -0300 Subject: [PATCH 2/3] Change message for fail() on new test --- src/test/java/org/owasp/validator/html/test/PolicyTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/owasp/validator/html/test/PolicyTest.java b/src/test/java/org/owasp/validator/html/test/PolicyTest.java index 06e20e82..8c200d1c 100644 --- a/src/test/java/org/owasp/validator/html/test/PolicyTest.java +++ b/src/test/java/org/owasp/validator/html/test/PolicyTest.java @@ -153,7 +153,7 @@ public void testSchemaValidationToggle() { policy = Policy.getInstance(new ByteArrayInputStream(policyFile.getBytes())); assertNotNull(policy); } catch (PolicyException e) { - fail("Not supported tag on policy, but not PolicyException occurred."); + fail("Schema validation is disabled, policy creation should not fail."); } // Enable validation again From 593c230af1484c28978892af85de6b9309e86189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebasti=C3=A1n=20Passaro?= Date: Fri, 22 Jan 2021 17:30:52 -0300 Subject: [PATCH 3/3] Refactor to issue console warnings when validating schema This implementation ntends to follow this steps: 1) Parsing with schema validation on and, if successful, check is validation is disabled. If it is disabled then issue a warning that it should not be. 2) If validation fails, remember the exception and then try to instantiate the parser again with validation off. If that also fails, throw that exception. If it succeeds, extract the error message from the saved exception and issue that as a warning. That procedure is just to try to help users with the policy change. Eventually, the changes on this commit should be reverted to always validate. --- .../java/org/owasp/validator/html/Policy.java | 224 ++++++++++++------ .../owasp/validator/html/test/PolicyTest.java | 44 +++- src/test/resources/invalidPolicy.xml | 11 + 3 files changed, 200 insertions(+), 79 deletions(-) create mode 100644 src/test/resources/invalidPolicy.xml diff --git a/src/main/java/org/owasp/validator/html/Policy.java b/src/main/java/org/owasp/validator/html/Policy.java index 88de441c..6c2587b9 100644 --- a/src/main/java/org/owasp/validator/html/Policy.java +++ b/src/main/java/org/owasp/validator/html/Policy.java @@ -31,7 +31,6 @@ import java.io.InputStream; import java.net.MalformedURLException; import java.net.URI; -import java.net.URISyntaxException; import java.net.URL; import java.util.ArrayList; import java.util.Collections; @@ -40,6 +39,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.Callable; import java.util.regex.Pattern; import javax.xml.XMLConstants; @@ -121,6 +121,7 @@ public class Policy { */ private static Schema schema = null; private static boolean validateSchema = true; + private static SAXException savedSchemaValidationException; /** * Get the Tag specified by the provided tag name. @@ -289,9 +290,19 @@ protected static ParseContext getParseContext(Element topLevelElement, URL baseU return parseContext; } + protected static Element getTopLevelElement(final URL baseUrl) throws PolicyException { + final InputSource source = getSourceFromUrl(baseUrl); + return getTopLevelElement(source, new Callable() { + @Override + public InputSource call() throws PolicyException { + return getSourceFromUrl(baseUrl); + } + }); + } + @SuppressFBWarnings(value = "SECURITY", justification="Opening a stream to the provided URL is not " - + "a vulnerability because it points to a local JAR file.") - protected static Element getTopLevelElement(URL baseUrl) throws PolicyException { + + "a vulnerability because it points to a local JAR file.") + protected static InputSource getSourceFromUrl(URL baseUrl) throws PolicyException { try { InputSource source = resolveEntity(baseUrl.toExternalForm(), baseUrl); if (source == null) { @@ -301,7 +312,7 @@ protected static Element getTopLevelElement(URL baseUrl) throws PolicyException source.setSystemId(baseUrl.toExternalForm()); } - return getTopLevelElement( source); + return source; } catch (SAXException | IOException e) { // SAXException can't actually happen. See JavaDoc for resolveEntity(String, URL) throw new PolicyException(e); @@ -309,36 +320,64 @@ protected static Element getTopLevelElement(URL baseUrl) throws PolicyException } private static Element getTopLevelElement(InputStream is) throws PolicyException { - return getTopLevelElement(new InputSource(is)); + final InputSource source = new InputSource(is); + source.getByteStream().mark(0); + return getTopLevelElement(source, new Callable() { + @Override + public InputSource call() throws IOException { + source.getByteStream().reset(); + return source; + } + }); } - protected static Element getTopLevelElement(InputSource source) throws PolicyException { + protected static Element getTopLevelElement(InputSource source, Callable getResetSource) throws PolicyException { try { - - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); - - /** - * Disable external entities, etc. - */ - dbf.setFeature(EXTERNAL_GENERAL_ENTITIES, false); - dbf.setFeature(EXTERNAL_PARAM_ENTITIES, false); - dbf.setFeature(DISALLOW_DOCTYPE_DECL, true); - dbf.setFeature(LOAD_EXTERNAL_DTD, false); - - if (validateSchema) { - getPolicySchema(); - dbf.setNamespaceAware(true); - dbf.setSchema(schema); + return getDocumentElementFromSource(source, true); + } catch (SAXException e) { + if (!validateSchema) { + try { + savedSchemaValidationException = e; + source = getResetSource.call(); + return getDocumentElementFromSource(source, false); + } catch (Exception e2) { + savedSchemaValidationException = null; + throw new PolicyException(e2); + } + } else { + throw new PolicyException(e); } + } catch (ParserConfigurationException | IOException e) { + throw new PolicyException(e); + } + } - DocumentBuilder db = dbf.newDocumentBuilder(); - db.setErrorHandler(new SAXErrorHandler()); - Document dom = db.parse(source); + private static Element getDocumentElementFromSource(InputSource source, boolean schemaValidationEnabled) + throws ParserConfigurationException, SAXException, IOException { - return dom.getDocumentElement(); - } catch (SAXException | ParserConfigurationException | IOException e) { - throw new PolicyException(e); + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + + /** + * Disable external entities, etc. + */ + dbf.setFeature(EXTERNAL_GENERAL_ENTITIES, false); + dbf.setFeature(EXTERNAL_PARAM_ENTITIES, false); + dbf.setFeature(DISALLOW_DOCTYPE_DECL, true); + dbf.setFeature(LOAD_EXTERNAL_DTD, false); + + if (schemaValidationEnabled) { + getPolicySchema(); + dbf.setNamespaceAware(true); + dbf.setSchema(schema); } + + DocumentBuilder db = dbf.newDocumentBuilder(); + db.setErrorHandler(new SAXErrorHandler()); + Document dom = db.parse(source); + + triggerSchemaValidationWarnings(); + + return dom.getDocumentElement(); } private static void parsePolicy(Element topLevelElement, ParseContext parseContext) @@ -365,77 +404,106 @@ private static void parsePolicy(Element topLevelElement, ParseContext parseConte */ @SuppressFBWarnings(value = "SECURITY", justification="Opening a stream to the provided URL is not " + "a vulnerability because only local file URLs are allowed.") - private static Element getPolicy(String href, URL baseUrl) - throws PolicyException { - + private static Element getPolicy(String href, URL baseUrl) throws PolicyException { try { - InputSource source = null; + return getDocumentElementByUrl(href, baseUrl, true); + } catch (SAXException e) { + if (!validateSchema) { + try { + savedSchemaValidationException = e; + return getDocumentElementByUrl(href, baseUrl, false); + } catch (SAXException | ParserConfigurationException | IOException e2) { + savedSchemaValidationException = null; + throw new PolicyException(e2); + } + } else { + throw new PolicyException(e); + } + } catch (ParserConfigurationException | IOException e) { + throw new PolicyException(e); + } + } - // Can't resolve public id, but might be able to resolve relative - // system id, since we have a base URI. - if (href != null && baseUrl != null) { + private static Element getDocumentElementByUrl(String href, URL baseUrl, boolean schemaValidationEnabled) + throws IOException, ParserConfigurationException, SAXException { - if (!"file".equals(baseUrl.getProtocol())) { - throw new MalformedURLException( - "Only local files can be accessed with the baseURL. Illegal value supplied was: " + baseUrl); - } + InputSource source = null; - URL url; + // Can't resolve public id, but might be able to resolve relative + // system id, since we have a base URI. + if (href != null && baseUrl != null) { + if (!"file".equals(baseUrl.getProtocol())) { + throw new MalformedURLException( + "Only local files can be accessed with the baseURL. Illegal value supplied was: " + baseUrl); + } + + URL url; + + try { + url = new URL(baseUrl, href); + source = new InputSource(url.openStream()); + source.setSystemId(href); + + } catch (MalformedURLException | java.io.FileNotFoundException e) { try { - url = new URL(baseUrl, href); + String absURL = URIUtils.resolveAsString(href, baseUrl.toString()); + url = new URL(absURL); source = new InputSource(url.openStream()); source.setSystemId(href); - } catch (MalformedURLException | java.io.FileNotFoundException e) { - try { - String absURL = URIUtils.resolveAsString(href, baseUrl.toString()); - url = new URL(absURL); - source = new InputSource(url.openStream()); - source.setSystemId(href); - - } catch (MalformedURLException ex2) { - // nothing to do - } + } catch (MalformedURLException ex2) { + // nothing to do } } + } - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); + DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); - /** - * Disable external entities, etc. - */ - dbf.setFeature(EXTERNAL_GENERAL_ENTITIES, false); - dbf.setFeature(EXTERNAL_PARAM_ENTITIES, false); - dbf.setFeature(DISALLOW_DOCTYPE_DECL, true); - dbf.setFeature(LOAD_EXTERNAL_DTD, false); - - if (validateSchema) { - getPolicySchema(); - dbf.setNamespaceAware(true); - dbf.setSchema(schema); - } + /** + * Disable external entities, etc. + */ + dbf.setFeature(EXTERNAL_GENERAL_ENTITIES, false); + dbf.setFeature(EXTERNAL_PARAM_ENTITIES, false); + dbf.setFeature(DISALLOW_DOCTYPE_DECL, true); + dbf.setFeature(LOAD_EXTERNAL_DTD, false); + + if (schemaValidationEnabled) { + getPolicySchema(); + dbf.setNamespaceAware(true); + dbf.setSchema(schema); + } - DocumentBuilder db = dbf.newDocumentBuilder(); - db.setErrorHandler(new SAXErrorHandler()); - Document dom; + DocumentBuilder db = dbf.newDocumentBuilder(); + db.setErrorHandler(new SAXErrorHandler()); + Document dom; + + /** + * Load and parse the file. + */ + if (source != null) { + dom = db.parse(source); + + triggerSchemaValidationWarnings(); /** - * Load and parse the file. + * Get the policy information out of it! */ - if (source != null) { - dom = db.parse(source); - /** - * Get the policy information out of it! - */ + return dom.getDocumentElement(); + } - return dom.getDocumentElement(); - } + return null; + } - return null; - } catch (SAXException | ParserConfigurationException | IOException e) { - throw new PolicyException(e); + private static void triggerSchemaValidationWarnings() { + if (!validateSchema) { + System.out.println("WARNING: XML schema validation for the policy is disabled, but it should not be."); + } + + if (savedSchemaValidationException != null) { + System.out.println("WARNING: " + savedSchemaValidationException.getMessage()); + savedSchemaValidationException = null; } } diff --git a/src/test/java/org/owasp/validator/html/test/PolicyTest.java b/src/test/java/org/owasp/validator/html/test/PolicyTest.java index 8c200d1c..839f02b6 100644 --- a/src/test/java/org/owasp/validator/html/test/PolicyTest.java +++ b/src/test/java/org/owasp/validator/html/test/PolicyTest.java @@ -35,6 +35,7 @@ import org.owasp.validator.html.scan.Constants; import java.io.ByteArrayInputStream; +import java.net.URL; /** * This class tests the Policy functionality to show that we can successfully parse the policy file. @@ -141,7 +142,7 @@ public void testInvalidPolicies() { } } - public void testSchemaValidationToggle() { + public void testSchemaValidationToggleWithSource() { String notSupportedTagsSection = "\n" + "\n"; String policyFile = assembleFile(notSupportedTagsSection); @@ -156,6 +157,14 @@ public void testSchemaValidationToggle() { fail("Schema validation is disabled, policy creation should not fail."); } + // This one should only print a warning on the console because validation is disabled + try { + policy = Policy.getInstance(new ByteArrayInputStream(assembleFile("").getBytes())); + assertNotNull(policy); + } catch (PolicyException e) { + fail("Schema validation is disabled, policy creation should not fail."); + } + // Enable validation again Policy.toggleSchemaValidation(true); @@ -166,5 +175,38 @@ public void testSchemaValidationToggle() { assertNotNull(e); } } + + public void testSchemaValidationToggleWithUrl() { + URL urlOfValidPolicy = getClass().getResource("/antisamy.xml"); + URL urlOfInvalidPolicy = getClass().getResource("/invalidPolicy.xml"); + + // Disable validation + Policy.toggleSchemaValidation(false); + + try { + policy = TestPolicy.getInstance(urlOfInvalidPolicy); + assertNotNull(policy); + } catch (PolicyException e) { + fail("Schema validation is disabled, policy creation should not fail."); + } + + // This one should only print a warning on the console because validation is disabled + try { + policy = TestPolicy.getInstance(urlOfValidPolicy); + assertNotNull(policy); + } catch (PolicyException e) { + fail("Schema validation is disabled, policy creation should not fail."); + } + + // Enable validation again + Policy.toggleSchemaValidation(true); + + try { + policy = TestPolicy.getInstance(urlOfInvalidPolicy); + fail("Not supported tag on policy, but no PolicyException occurred."); + } catch (PolicyException e) { + assertNotNull(e); + } + } } diff --git a/src/test/resources/invalidPolicy.xml b/src/test/resources/invalidPolicy.xml new file mode 100644 index 00000000..80756268 --- /dev/null +++ b/src/test/resources/invalidPolicy.xml @@ -0,0 +1,11 @@ + + + + + + + + + + + \ No newline at end of file