Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add toggle for policy validation #65

Closed
wants to merge 3 commits into from
Closed

Add toggle for policy validation #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69a12bb
Add toggle for policy validation
spassarop Jan 18, 2021
b39d3ee
Change message for fail() on new test
spassarop Jan 18, 2021
593c230
Refactor to issue console warnings when validating schema
spassarop Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 155 additions & 69 deletions src/main/java/org/owasp/validator/html/Policy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -120,6 +120,8 @@ public class Policy {
* XML Schema for policy validation
*/
private static Schema schema = null;
private static boolean validateSchema = true;
private static SAXException savedSchemaValidationException;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems strange to me to have this be a class variable. Can't it be passed as a parameter to the methods that need it from the caller?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would require an API change. All Policy.getPolicy() calls would need a new Boolean parameter to pass down and the method to enable/disable validation would be removed as it won’t be needed anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. I was referring to savedSchemaValidationException, not the boolean, which I think is fine. I'm testing/making changes and I'll address my concern if I easily can. Hold on for now.


/**
* Get the Tag specified by the provided tag name.
Expand Down Expand Up @@ -160,6 +162,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")
*
Expand Down Expand Up @@ -278,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<InputSource>() {
@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) {
Expand All @@ -290,40 +312,72 @@ 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);
}
}

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<InputSource>() {
@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<InputSource> getResetSource) throws PolicyException {
try {
getPolicySchema();
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);
}
}

DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
private static Element getDocumentElementFromSource(InputSource source, boolean schemaValidationEnabled)
throws ParserConfigurationException, SAXException, IOException {

/**
* 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);
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);

return dom.getDocumentElement();
} catch (SAXException | ParserConfigurationException | IOException | URISyntaxException e) {
throw new PolicyException(e);
}

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)
Expand All @@ -350,74 +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
}
}
}

getPolicySchema();
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);

/**
* 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 | URISyntaxException 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;
}
}

Expand Down Expand Up @@ -906,7 +992,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);
Expand Down
Loading