diff --git a/pom.xml b/pom.xml index 8f2c69b3..7429c3a7 100644 --- a/pom.xml +++ b/pom.xml @@ -3,7 +3,7 @@ org.owasp.antisamy antisamy jar - 1.5.10 + 1.5.11 @@ -15,7 +15,7 @@ https://oss.sonatype.org/service/local/staging/deploy/maven2/ - + OWASP AntiSamy A library for performing fast, configurable cleansing of HTML coming from untrusted sources. https://github.com/nahsra/antisamy @@ -43,6 +43,7 @@ UTF-8 + 4.1.4 @@ -79,7 +80,7 @@ org.apache.httpcomponents httpclient - 4.5.12 + 4.5.13 @@ -88,6 +89,12 @@ + xerces xercesImpl @@ -96,7 +103,21 @@ commons-codec commons-codec - 1.14 + 1.15 + + + + + com.github.spotbugs + spotbugs-annotations + ${version.spotbugs} + true + + + net.jcip + jcip-annotations + 1.0 + true @@ -110,15 +131,15 @@ - - + + org.apache.maven.plugins maven-dependency-plugin 3.1.2 - - + + org.apache.maven.plugins @@ -145,7 +166,7 @@ org.codehaus.mojo extra-enforcer-rules - 1.2 + 1.3 @@ -156,6 +177,7 @@ 1.7 + true test Dependencies shouldn't require Java 8+. @@ -212,7 +234,7 @@ org.apache.maven.plugins maven-site-plugin - 3.9.0 + 3.9.1 org.apache.maven.plugins @@ -234,14 +256,15 @@ com.github.spotbugs spotbugs-maven-plugin - 4.0.4 + ${version.spotbugs} - com.github.spotbugs spotbugs - 4.0.4 - + ${version.spotbugs} + @@ -251,7 +274,7 @@ org.codehaus.mojo versions-maven-plugin - 2.7 + 2.8.1 @@ -264,10 +287,11 @@ org.apache.maven.plugins maven-project-info-reports-plugin - 3.1.0 + 3.1.1 + index dependency-convergence @@ -287,6 +311,8 @@ 1.10.1 + Max + false diff --git a/src/main/java/org/owasp/validator/css/CssScanner.java b/src/main/java/org/owasp/validator/css/CssScanner.java index 61680a32..7b9e352c 100644 --- a/src/main/java/org/owasp/validator/css/CssScanner.java +++ b/src/main/java/org/owasp/validator/css/CssScanner.java @@ -138,14 +138,11 @@ public CleanResults scanStyleSheet(String taintedCss, int sizeLimit) throws Scan // should already have been counted by the caller since it was // embedded in the HTML parser.parseStyleSheet(new InputSource(new StringReader(taintedCss))); - } catch (IOException ioe) { - throw new ScanException(ioe); - + } catch (IOException | ParseException e) { /* - * ParseExceptions, from batik, is unfortunately a RuntimeException. + * ParseException, from batik, is unfortunately a RuntimeException. */ - } catch (ParseException pe) { - throw new ScanException(pe); + throw new ScanException(e); } parseImportedStylesheets(stylesheets, handler, errorMessages, sizeLimit); diff --git a/src/main/java/org/owasp/validator/html/Policy.java b/src/main/java/org/owasp/validator/html/Policy.java index b08c8b40..292ca4be 100644 --- a/src/main/java/org/owasp/validator/html/Policy.java +++ b/src/main/java/org/owasp/validator/html/Policy.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007-2019, Arshan Dabirsiaghi, Jason Li, Kristian Rosenvold + * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li, Kristian Rosenvold * * All rights reserved. * @@ -24,6 +24,8 @@ package org.owasp.validator.html; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -128,7 +130,6 @@ public void resetParamsWhereLastConfigWins() { } } - /** * Retrieves a CSS Property from the Policy. * @@ -173,7 +174,6 @@ public static Policy getInstance(InputStream inputStream) throws PolicyException } - /** * This retrieves a Policy based on the File object passed in * @@ -190,7 +190,6 @@ public static Policy getInstance(File file) throws PolicyException { } } - /** * This retrieves a Policy based on the URL object passed in. *

@@ -230,7 +229,7 @@ protected Policy(Policy old, Map directives, Map ta protected static ParseContext getSimpleParseContext(Element topLevelElement) throws PolicyException { ParseContext parseContext = new ParseContext(); - if (getByTagName(topLevelElement, "include").iterator().hasNext()){ + if (getByTagName(topLevelElement, "include").iterator().hasNext()) { throw new IllegalArgumentException("A policy file loaded with an InputStream cannot contain include references"); } @@ -259,7 +258,8 @@ protected static ParseContext getParseContext(Element topLevelElement, URL baseU return parseContext; } - + @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 { try { InputSource source = resolveEntity(baseUrl.toExternalForm(), baseUrl); @@ -271,10 +271,8 @@ protected static Element getTopLevelElement(URL baseUrl) throws PolicyException } return getTopLevelElement( source); - } catch (SAXException e) { - // This can't actually happen. See JavaDoc for resolveEntity(String, URL) - throw new PolicyException(e); - } catch (IOException e) { + } catch (SAXException | IOException e) { + // SAXException can't actually happen. See JavaDoc for resolveEntity(String, URL) throw new PolicyException(e); } } @@ -297,17 +295,11 @@ protected static Element getTopLevelElement(InputSource source) throws PolicyExc DocumentBuilder db = dbf.newDocumentBuilder(); Document dom = db.parse(source); return dom.getDocumentElement(); - } catch (SAXException e) { - throw new PolicyException(e); - } catch (ParserConfigurationException e) { - throw new PolicyException(e); - } catch (IOException e) { + } catch (SAXException | ParserConfigurationException | IOException e) { throw new PolicyException(e); } } - - private static void parsePolicy(Element topLevelElement, ParseContext parseContext) throws PolicyException { @@ -327,10 +319,11 @@ private static void parsePolicy(Element topLevelElement, ParseContext parseConte parseRequiresClosingTags(getFirstChild(topLevelElement, "require-closing-tags"), parseContext.requireClosingTags); } - /** * Returns the top level element of a loaded policy Document */ + @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 { @@ -340,6 +333,12 @@ private static Element getPolicy(String href, URL baseUrl) // 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 { @@ -347,18 +346,7 @@ private static Element getPolicy(String href, URL baseUrl) source = new InputSource(url.openStream()); source.setSystemId(href); - } catch (MalformedURLException except) { - 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 (java.io.FileNotFoundException fnfe) { + } catch (MalformedURLException | java.io.FileNotFoundException e) { try { String absURL = URIUtils.resolveAsString(href, baseUrl.toString()); url = new URL(absURL); @@ -389,7 +377,6 @@ private static Element getPolicy(String href, URL baseUrl) if (source != null) { dom = db.parse(source); - /** * Get the policy information out of it! */ @@ -398,11 +385,7 @@ private static Element getPolicy(String href, URL baseUrl) } return null; - } catch (SAXException e) { - throw new PolicyException(e); - } catch (ParserConfigurationException e) { - throw new PolicyException(e); - } catch (IOException e) { + } catch (SAXException | ParserConfigurationException | IOException e) { throw new PolicyException(e); } } @@ -419,7 +402,6 @@ public Policy cloneWithDirective(String name, String value) { return new InternalPolicy(this, Collections.unmodifiableMap(directives), tagRules); } - /** * Go through section of the policy file. * @@ -434,7 +416,6 @@ private static void parseDirectives(Element root, Map directives } } - /** * Go through section of the policy file. * @@ -489,7 +470,6 @@ private static void parseGlobalAttributes(Element root, Map g for (Element ele : getByTagName(root, "attribute")) { String name = getAttributeValue(ele, "name"); - Attribute toAdd = commonAttributes.get(name.toLowerCase()); if (toAdd != null) { @@ -512,7 +492,6 @@ private static void parseDynamicAttributes(Element root, Map for (Element ele : getByTagName(root, "attribute")) { String name = getAttributeValue(ele, "name"); - Attribute toAdd = commonAttributes.get(name.toLowerCase()); if (toAdd != null) { @@ -540,7 +519,6 @@ private static void parseCommonRegExps(Element root, Map commonAttributes1, Map commonRegularExpressions1) { for (Element ele : getByTagName(root, "attribute")) { @@ -559,7 +537,6 @@ private static void parseCommonAttributes(Element root, Map c String description = getAttributeValue(ele, "description"); Attribute attribute = new Attribute(getAttributeValue(ele, "name"), allowedRegexps, allowedValues, onInvalidStr, description); - commonAttributes1.put(name.toLowerCase(), attribute); } } @@ -600,7 +577,7 @@ private static List getAllowedRegexps2(Map com String regExpName = getAttributeValue(regExpNode, "name"); String value = getAttributeValue(regExpNode, "value"); - /* + /* * Look up common regular expression specified * by the "name" field. They can put a common * name in the "name" field or provide a custom @@ -612,9 +589,9 @@ private static List getAllowedRegexps2(Map com AntiSamyPattern pattern = commonRegularExpressions1.get(regExpName); if (pattern != null) { allowedRegexps.add(pattern.getPattern()); - } else { - throw new PolicyException("Regular expression '" + regExpName + "' was referenced as a common regexp in definition of '" + tagName + "', but does not exist in "); - } + } else throw new PolicyException("Regular expression '" + regExpName + + "' was referenced as a common regexp in definition of '" + tagName + + "', but does not exist in "); } else if (value != null && value.length() > 0) { allowedRegexps.add(Pattern.compile(value)); @@ -623,7 +600,9 @@ private static List getAllowedRegexps2(Map com return allowedRegexps; } - private static List getAllowedRegexp3(Map commonRegularExpressions1, Element ele, String name) throws PolicyException { + private static List getAllowedRegexp3(Map commonRegularExpressions1, + Element ele, String name) throws PolicyException { + List allowedRegExp = new ArrayList(); for (Element regExpNode : getGrandChildrenByTagName(ele, "regexp-list", "regexp")) { String regExpName = getAttributeValue(regExpNode, "name"); @@ -635,14 +614,15 @@ private static List getAllowedRegexp3(Map comm allowedRegExp.add(pattern.getPattern()); } else if (value != null) { allowedRegExp.add(Pattern.compile(value)); - } else { - throw new PolicyException("Regular expression '" + regExpName + "' was referenced as a common regexp in definition of '" + name + "', but does not exist in "); - } + } else throw new PolicyException("Regular expression '" + regExpName + + "' was referenced as a common regexp in definition of '" + name + + "', but does not exist in "); } return allowedRegExp; } - - private static void parseTagRules(Element root, Map commonAttributes1, Map commonRegularExpressions1, Map tagRules1) throws PolicyException { + + private static void parseTagRules(Element root, Map commonAttributes1, Map commonRegularExpressions1, Map tagRules1) throws PolicyException { if (root == null) return; @@ -671,29 +651,24 @@ private static Map getTagAttributes(Map co Attribute attribute = commonAttributes1.get(attrName); /* - * All they provided was the name, so they must want a common - * attribute. - */ + * All they provided was the name, so they must want a common attribute. + */ if (attribute != null) { - /* - * If they provide onInvalid/description values here they will - * override the common values. - */ + * If they provide onInvalid/description values here they will + * override the common values. + */ String onInvalid = getAttributeValue(attributeNode, "onInvalid"); String description = getAttributeValue(attributeNode, "description"); - Attribute changed = attribute.mutate(onInvalid, description); - commonAttributes1.put(attrName, changed); - tagAttributes.put(attrName, changed); } else { - - throw new PolicyException("Attribute '" + getAttributeValue(attributeNode, "name") + "' was referenced as a common attribute in definition of '" + tagName + "', but does not exist in "); - + throw new PolicyException("Attribute '" + getAttributeValue(attributeNode, "name") + + "' was referenced as a common attribute in definition of '" + tagName + + "', but does not exist in "); } } else { @@ -704,8 +679,8 @@ private static Map getTagAttributes(Map co Attribute attribute = new Attribute(getAttributeValue(attributeNode, "name"), allowedRegexps2, allowedValues2, onInvalid, description); /* - * Add fully built attribute. - */ + * Add fully built attribute. + */ tagAttributes.put(attrName, attribute); } @@ -713,7 +688,6 @@ private static Map getTagAttributes(Map co return tagAttributes; } - private static void parseCSSRules(Element root, Map cssRules1, Map commonRegularExpressions1) throws PolicyException { for (Element ele : getByTagName(root, "property")){ @@ -721,7 +695,6 @@ private static void parseCSSRules(Element root, Map cssRules1, String name = getAttributeValue(ele, "name"); String description = getAttributeValue(ele, "description"); - List allowedRegexp3 = getAllowedRegexp3(commonRegularExpressions1, ele, name); List allowedValue = new ArrayList(); @@ -743,14 +716,10 @@ private static void parseCSSRules(Element root, Map cssRules1, } Property property = new Property(name,allowedRegexp3, allowedValue, shortHandRefs, description, onInvalidStr ); - - cssRules1.put(name.toLowerCase(), property); - } } - /** * A simple method for returning on of the <global-attribute> entries by * name. @@ -760,7 +729,6 @@ private static void parseCSSRules(Element root, Map cssRules1, */ public Attribute getGlobalAttributeByName(String name) { return globalAttributes.get(name.toLowerCase()); - } /** @@ -773,11 +741,12 @@ public Attribute getGlobalAttributeByName(String name) { */ public Attribute getDynamicAttributeByName(String name) { Attribute dynamicAttribute = null; - for (String d : dynamicAttributes.keySet()) { - if (name.startsWith(d)) { - dynamicAttribute = dynamicAttributes.get(d); - break; - } + Set> entries = dynamicAttributes.entrySet(); + for (Map.Entry entry : entries) { + if (name.startsWith(entry.getKey())) { + dynamicAttribute = entry.getValue(); + break; + } } return dynamicAttribute; } @@ -810,7 +779,6 @@ public String getDirective(String name) { return directives.get(name); } - /** * Resolves public and system IDs to files stored within the JAR. * @@ -821,12 +789,21 @@ public String getDirective(String name) { * @throws SAXException This exception can't actually be thrown, but left in the method signature for * API compatibility reasons. */ + @SuppressFBWarnings(value = "SECURITY", justification="Opening a stream to the provided URL is not " + + "a vulnerability because only local file URLs are allowed.") public static InputSource resolveEntity(final String systemId, URL baseUrl) throws IOException, SAXException { + InputSource source; // Can't resolve public id, but might be able to resolve relative // system id, since we have a base URI. if (systemId != 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 { @@ -834,17 +811,7 @@ public static InputSource resolveEntity(final String systemId, URL baseUrl) thro source = new InputSource(url.openStream()); source.setSystemId(systemId); return source; - } catch (MalformedURLException except) { - try { - String absURL = URIUtils.resolveAsString(systemId, baseUrl.toString()); - url = new URL(absURL); - source = new InputSource(url.openStream()); - source.setSystemId(systemId); - return source; - } catch (MalformedURLException ex2) { - // nothing to do - } - } catch (java.io.FileNotFoundException fnfe) { + } catch (MalformedURLException | java.io.FileNotFoundException e) { try { String absURL = URIUtils.resolveAsString(systemId, baseUrl.toString()); url = new URL(absURL); @@ -867,8 +834,7 @@ private static Element getFirstChild(Element element, String tagName) { NodeList elementsByTagName = element.getElementsByTagName(tagName); if (elementsByTagName != null && elementsByTagName.getLength() > 0) return (Element) elementsByTagName.item(0); - else - return null; + else return null; } private static Iterable getGrandChildrenByTagName(Element parent, String immediateChildName, String subChild){ diff --git a/src/main/java/org/owasp/validator/html/scan/AbstractAntiSamyScanner.java b/src/main/java/org/owasp/validator/html/scan/AbstractAntiSamyScanner.java index 295fbb20..371557fe 100644 --- a/src/main/java/org/owasp/validator/html/scan/AbstractAntiSamyScanner.java +++ b/src/main/java/org/owasp/validator/html/scan/AbstractAntiSamyScanner.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007-2019, Arshan Dabirsiaghi, Jason Li + * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li * * All rights reserved. * @@ -44,7 +44,6 @@ public abstract class AbstractAntiSamyScanner { public abstract CleanResults scan(String html) throws ScanException; - /* UnusedDeclaration TODO: Investigate */ public abstract CleanResults getResults(); public AbstractAntiSamyScanner(Policy policy) { diff --git a/src/main/java/org/owasp/validator/html/scan/AntiSamyDOMScanner.java b/src/main/java/org/owasp/validator/html/scan/AntiSamyDOMScanner.java index c8abee7b..bc54dab2 100644 --- a/src/main/java/org/owasp/validator/html/scan/AntiSamyDOMScanner.java +++ b/src/main/java/org/owasp/validator/html/scan/AntiSamyDOMScanner.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007-2019, Arshan Dabirsiaghi, Jason Li + * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li * * All rights reserved. * @@ -23,6 +23,8 @@ */ package org.owasp.validator.html.scan; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + import org.apache.batik.css.parser.ParseException; import org.apache.xerces.dom.DocumentImpl; import org.cyberneko.html.parsers.DOMFragmentParser; @@ -54,10 +56,11 @@ /** * This is where the magic lives. All the scanning/filtration logic resides * here, but it should not be called directly. All scanning should be done - * through a AntiSamy.scan() method. + * through an AntiSamy.scan() method. * * @author Arshan Dabirsiaghi */ +@SuppressFBWarnings(value = "REDOS", justification="Tested the Regex against saferegex and safe-regex and not vulnerable") public class AntiSamyDOMScanner extends AbstractAntiSamyScanner { private Document document = new DocumentImpl(); @@ -66,8 +69,7 @@ public class AntiSamyDOMScanner extends AbstractAntiSamyScanner { private static final int maxDepth = 250; private static final Pattern invalidXmlCharacters = Pattern.compile("[\\u0000-\\u001F\\uD800-\\uDFFF\\uFFFE-\\uFFFF&&[^\\u0009\\u000A\\u000D]]"); - private static final Pattern conditionalDirectives = - Pattern.compile("?"); + private static final Pattern conditionalDirectives = Pattern.compile("?"); private static final Queue cachedItems = new ConcurrentLinkedQueue(); @@ -97,18 +99,18 @@ public AntiSamyDOMScanner() throws PolicyException { /** * This is where the magic lives. * - * @param html - * A String whose contents we want to scan. + * @param html A String whose contents we want to scan. * @return A CleanResults object with an * XMLDocumentFragment object and its String * representation, as well as some scan statistics. * @throws ScanException When there is a problem encountered * while scanning the HTML. */ + @Override public CleanResults scan(String html) throws ScanException { if (html == null) { - throw new ScanException(new NullPointerException("Null input")); + throw new ScanException(new NullPointerException("Null html input")); } errorMessages.clear(); @@ -146,7 +148,6 @@ public CleanResults scan(String html) throws ScanException { * W3C. */ - DOMFragmentParser parser = cachedItem.getDomFragmentParser(); try { @@ -162,7 +163,6 @@ public CleanResults scan(String html) throws ScanException { * its string representation. */ - final String trimmedHtml = html; StringWriter out = new StringWriter(); @@ -174,10 +174,10 @@ public CleanResults scan(String html) throws ScanException { org.apache.xml.serialize.HTMLSerializer serializer = getHTMLSerializer(out, format); serializer.serialize(dom); - /* - * Get the String out of the StringWriter and rip out the XML - * declaration if the Policy says we should. - */ + /* + * Get the String out of the StringWriter and rip out the XML + * declaration if the Policy says we should. + */ final String trimmed = trim( trimmedHtml, out.getBuffer().toString() ); Callable cleanHtml = new Callable() { @@ -186,7 +186,7 @@ public String call() throws Exception { } }; - /** + /* * Return the DOM object as well as string HTML. */ results = new CleanResults(startOfScan, cleanHtml, dom, errorMessages); @@ -194,11 +194,7 @@ public String call() throws Exception { cachedItems.add( cachedItem); return results; - - } catch (SAXException e) { - throw new ScanException(e); - } - catch ( IOException e ) { + } catch (SAXException | IOException e) { throw new ScanException(e); } @@ -226,8 +222,7 @@ static DOMFragmentParser getDomParser() * according to the policy. This should be called implicitly through the * AntiSamy.scan() method. * - * @param node - * The node to validate. + * @param node The node to validate. */ private void recursiveValidateTag(final Node node, int currentStackDepth) throws ScanException { @@ -334,7 +329,7 @@ private void actionFilter(int currentStackDepth, Element ele, String tagName, Ta } private void actionValidate(int currentStackDepth, Element ele, Node parentNode, String tagName, String tagNameLowerCase, Tag tag, boolean masqueradingParam, Tag embedTag, NodeList eleChildNodes) throws ScanException { - /* + /* * If doing as , now is the time to convert it. */ String nameValue = null; @@ -393,21 +388,18 @@ private boolean processStyleTag(Element ele, Node parentNode) { */ CssScanner styleScanner; - if(policy.isEmbedStyleSheets()) { + if (policy.isEmbedStyleSheets()) { styleScanner = new ExternalCssScanner(policy, messages); - }else{ + } else { styleScanner = new CssScanner(policy, messages); } try { - Node firstChild = ele.getFirstChild(); if (firstChild != null) { String toScan = firstChild.getNodeValue(); - CleanResults cr = styleScanner.scanStyleSheet(toScan, policy.getMaxInputSize()); - errorMessages.addAll(cr.getErrorMessages()); /* @@ -421,43 +413,19 @@ private boolean processStyleTag(Element ele, Node parentNode) { final String cleanHTML = cr.getCleanHTML(); if (cleanHTML == null || cleanHTML.equals("")) { - firstChild.setNodeValue("/* */"); - } else { - firstChild.setNodeValue(cleanHTML); - } - } - } catch (DOMException e) { - - addError(ErrorMessageUtil.ERROR_CSS_TAG_MALFORMED, new Object[]{HTMLEntityEncoder.htmlEntityEncode(ele.getFirstChild().getNodeValue())}); - parentNode.removeChild(ele); - return true; - - } catch (ScanException e) { - - addError(ErrorMessageUtil.ERROR_CSS_TAG_MALFORMED, new Object[]{HTMLEntityEncoder.htmlEntityEncode(ele.getFirstChild().getNodeValue())}); - parentNode.removeChild(ele); - return true; - - /* - * This shouldn't be reachable anymore, but we'll leave it - * here because I'm hilariously dumb sometimes. - */ - } catch (ParseException e) { - - addError(ErrorMessageUtil.ERROR_CSS_TAG_MALFORMED, new Object[]{HTMLEntityEncoder.htmlEntityEncode(ele.getFirstChild().getNodeValue())}); - parentNode.removeChild(ele); - return true; + } catch (DOMException | ScanException | ParseException | NumberFormatException e) { /* + * ParseException shouldn't be possible anymore, but we'll leave it + * here because I (Arshan) am hilariously dumb sometimes. * Batik can throw NumberFormatExceptions (see bug #48). */ - } catch (NumberFormatException e) { addError(ErrorMessageUtil.ERROR_CSS_TAG_MALFORMED, new Object[]{HTMLEntityEncoder.htmlEntityEncode(ele.getFirstChild().getNodeValue())}); parentNode.removeChild(ele); @@ -474,13 +442,10 @@ private void actionTruncate(Element ele, String tagName, NodeList eleChildNodes) */ NamedNodeMap nnmap = ele.getAttributes(); - while (nnmap.getLength() > 0) { - - addError(ErrorMessageUtil.ERROR_ATTRIBUTE_NOT_IN_POLICY, new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(nnmap.item(0).getNodeName())}); - + addError(ErrorMessageUtil.ERROR_ATTRIBUTE_NOT_IN_POLICY, + new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(nnmap.item(0).getNodeName())}); ele.removeAttribute(nnmap.item(0).getNodeName()); - } int i = 0; @@ -488,15 +453,12 @@ private void actionTruncate(Element ele, String tagName, NodeList eleChildNodes) int length = eleChildNodes.getLength(); while (i < length) { - Node nodeToRemove = eleChildNodes.item(j); - if (nodeToRemove.getNodeType() != Node.TEXT_NODE) { ele.removeChild(nodeToRemove); } else { j++; } - i++; } } @@ -523,8 +485,6 @@ private boolean processAttributes(Element ele, String tagName, Tag tag, int curr attr = policy.getGlobalAttributeByName(name); } - boolean isAttributeValid = false; - /* * We have to special case the "style" attribute since it's * validated quite differently. @@ -537,26 +497,15 @@ private boolean processAttributes(Element ele, String tagName, Tag tag, int curr CssScanner styleScanner = new CssScanner(policy, messages); try { - CleanResults cr = styleScanner.scanInlineStyle(value, tagName, policy.getMaxInputSize()); - attribute.setNodeValue(cr.getCleanHTML()); - List cssScanErrorMessages = cr.getErrorMessages(); - errorMessages.addAll(cssScanErrorMessages); - } catch (DOMException e) { - - addError(ErrorMessageUtil.ERROR_CSS_ATTRIBUTE_MALFORMED, new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(ele.getNodeValue())}); - - ele.removeAttribute(attribute.getNodeName()); - currentAttributeIndex--; - - } catch (ScanException e) { - - addError(ErrorMessageUtil.ERROR_CSS_ATTRIBUTE_MALFORMED, new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(ele.getNodeValue())}); + } catch (DOMException | ScanException e) { + addError(ErrorMessageUtil.ERROR_CSS_ATTRIBUTE_MALFORMED, + new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(ele.getNodeValue())}); ele.removeAttribute(attribute.getNodeName()); currentAttributeIndex--; } @@ -565,18 +514,12 @@ private boolean processAttributes(Element ele, String tagName, Tag tag, int curr if (attr != null) { - if (attr.containsAllowedValue( value.toLowerCase())) { - isAttributeValid = true; - } - - if (attr.matchesAllowedExpression(value)){ - isAttributeValid = true; - } - - if (!isAttributeValid) { + // See if attribute is invalid + if (!(attr.containsAllowedValue( value.toLowerCase()) || + (attr.matchesAllowedExpression( value ))) ) { /* - * Document transgression and perform the + * Attribute is NOT valid, so: Document transgression and perform the * "onInvalid" action. The default action is to * strip the attribute and leave the rest intact. */ @@ -592,34 +535,30 @@ private boolean processAttributes(Element ele, String tagName, Tag tag, int curr removeNode(ele); addError(ErrorMessageUtil.ERROR_ATTRIBUTE_INVALID_REMOVED, - new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); + new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); return true; } else if ("filterTag".equals(onInvalidAction)) { /* - * Remove the attribute and keep the rest of the - * tag. + * Remove the attribute and keep the rest of the tag. */ processChildren(ele, currentStackDepth); - promoteChildren(ele); - - addError(ErrorMessageUtil.ERROR_ATTRIBUTE_CAUSE_FILTER, new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); + addError(ErrorMessageUtil.ERROR_ATTRIBUTE_CAUSE_FILTER, + new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); } else if ("encodeTag".equals(onInvalidAction)) { /* - * Remove the attribute and keep the rest of the - * tag. + * Remove the attribute and keep the rest of the tag. */ processChildren(ele, currentStackDepth); - encodeAndPromoteChildren(ele); - - addError(ErrorMessageUtil.ERROR_ATTRIBUTE_CAUSE_ENCODE, new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); + addError(ErrorMessageUtil.ERROR_ATTRIBUTE_CAUSE_ENCODE, + new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); } else { @@ -628,18 +567,15 @@ private boolean processAttributes(Element ele, String tagName, Tag tag, int curr */ ele.removeAttribute(attribute.getNodeName()); - currentAttributeIndex--; - - addError(ErrorMessageUtil.ERROR_ATTRIBUTE_INVALID, new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); + addError(ErrorMessageUtil.ERROR_ATTRIBUTE_INVALID, + new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); if ("removeTag".equals(onInvalidAction) || "filterTag".equals(onInvalidAction)) { return true; // remove/filter the tag } - } - } } else { @@ -648,13 +584,12 @@ private boolean processAttributes(Element ele, String tagName, Tag tag, int curr * - remove it (whitelisting!) */ - addError(ErrorMessageUtil.ERROR_ATTRIBUTE_NOT_IN_POLICY, new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); - + addError(ErrorMessageUtil.ERROR_ATTRIBUTE_NOT_IN_POLICY, + new Object[]{tagName, HTMLEntityEncoder.htmlEntityEncode(name), HTMLEntityEncoder.htmlEntityEncode(value)}); ele.removeAttribute(attribute.getNodeName()); - currentAttributeIndex--; - } // end if attribute is or is not found in policy file + } // end if attribute is found in policy file } // end while loop through attributes @@ -671,7 +606,6 @@ private void processChildren(NodeList childNodes, int currentStackDepth ) throws for (int i = 0; i < childNodes.getLength(); i++) { tmp = childNodes.item(i); - recursiveValidateTag(tmp, currentStackDepth); /* @@ -760,7 +694,7 @@ private void promoteChildren(Element ele, NodeList eleChildNodes) { parent.insertBefore(node, ele); } - if (parent != null){ + if (parent != null) { removeNode(ele); } } @@ -838,6 +772,7 @@ private String toString(Element ele) { return eleAsString.toString(); } + @Override public CleanResults getResults() { return results; } diff --git a/src/main/java/org/owasp/validator/html/scan/AntiSamySAXScanner.java b/src/main/java/org/owasp/validator/html/scan/AntiSamySAXScanner.java index 01b41600..85ef9f47 100644 --- a/src/main/java/org/owasp/validator/html/scan/AntiSamySAXScanner.java +++ b/src/main/java/org/owasp/validator/html/scan/AntiSamySAXScanner.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007-2011, Arshan Dabirsiaghi, Jason Li + * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li * * All rights reserved. * @@ -32,6 +32,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentLinkedQueue; +import javax.xml.XMLConstants; import javax.xml.transform.OutputKeys; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerConfigurationException; @@ -56,6 +57,12 @@ public class AntiSamySAXScanner extends AbstractAntiSamyScanner { private static final TransformerFactory sTransformerFactory = TransformerFactory.newInstance(); + static { + // Disable external entities, etc. + sTransformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + sTransformerFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); + } + static class CachedItem { private final Transformer transformer; private final SAXParser saxParser; @@ -68,12 +75,9 @@ static class CachedItem { XMLDocumentFilter[] filters = { magicSAXFilter }; try { saxParser.setProperty("http://cyberneko.org/html/properties/filters", filters); - } catch (SAXNotRecognizedException e) { - throw new RuntimeException(e); - } catch (SAXNotSupportedException e) { + } catch (SAXNotRecognizedException | SAXNotSupportedException e) { throw new RuntimeException(e); } - } } @@ -81,18 +85,20 @@ public AntiSamySAXScanner(Policy policy) { super(policy); } + @Override public CleanResults getResults() { return null; } + @Override public CleanResults scan(String html) throws ScanException { return scan(html, this.policy); } public CleanResults scan(String html, Policy policy) throws ScanException { if (html == null) { - throw new ScanException(new NullPointerException("Null input")); - } + throw new ScanException(new NullPointerException("Null html input")); + } int maxInputSize = this.policy.getMaxInputSize(); @@ -192,9 +198,7 @@ private static SAXParser getParser() { parser.setProperty("http://cyberneko.org/html/properties/names/elems", "lower"); return parser; - } catch (SAXNotRecognizedException e) { - throw new RuntimeException(e); - } catch (SAXNotSupportedException e) { + } catch (SAXNotRecognizedException | SAXNotSupportedException e) { throw new RuntimeException(e); } } diff --git a/src/main/java/org/owasp/validator/html/scan/Constants.java b/src/main/java/org/owasp/validator/html/scan/Constants.java index 082b65e1..5487a349 100644 --- a/src/main/java/org/owasp/validator/html/scan/Constants.java +++ b/src/main/java/org/owasp/validator/html/scan/Constants.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007-2011, Arshan Dabirsiaghi, Jason Li + * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li * * All rights reserved. * @@ -33,8 +33,8 @@ public class Constants { public static final String DEFAULT_ENCODING_ALGORITHM = "UTF-8"; public static final Tag BASIC_PARAM_TAG_RULE; - public static List defaultAllowedEmptyTags; - public static List defaultRequiresClosingTags; + public static final List defaultAllowedEmptyTags; + public static final List defaultRequiresClosingTags; private static final String[] allowedEmptyTags = { "br", "hr", "a", "img", "link", "iframe", "script", "object", "applet", @@ -55,11 +55,13 @@ public class Constants { attrs.put(paramValueAttr.getName().toLowerCase(), paramValueAttr); BASIC_PARAM_TAG_RULE = new Tag("param", attrs, Policy.ACTION_VALIDATE); - defaultAllowedEmptyTags = new ArrayList(); - defaultAllowedEmptyTags.addAll(Arrays.asList(allowedEmptyTags)); - - defaultRequiresClosingTags = new ArrayList(); - defaultRequiresClosingTags.addAll(Arrays.asList(requiresClosingTags)); + List allowedEmptyTagsList = new ArrayList(); + allowedEmptyTagsList.addAll(Arrays.asList(allowedEmptyTags)); + defaultAllowedEmptyTags = Collections.unmodifiableList(allowedEmptyTagsList); + + List requiresClosingTagsList = new ArrayList(); + requiresClosingTagsList.addAll(Arrays.asList(requiresClosingTags)); + defaultRequiresClosingTags = Collections.unmodifiableList(requiresClosingTagsList); } public static final String DEFAULT_LOCALE_LANG = "en"; diff --git a/src/main/java/org/owasp/validator/html/scan/MagicSAXFilter.java b/src/main/java/org/owasp/validator/html/scan/MagicSAXFilter.java index d4f80db8..23cd0869 100644 --- a/src/main/java/org/owasp/validator/html/scan/MagicSAXFilter.java +++ b/src/main/java/org/owasp/validator/html/scan/MagicSAXFilter.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007-2011, Arshan Dabirsiaghi, Jason Li + * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li * * All rights reserved. * @@ -24,6 +24,8 @@ package org.owasp.validator.html.scan; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + import java.util.*; import java.util.regex.Pattern; @@ -53,13 +55,14 @@ * filter is SAX-based which means it is much more memory-efficient and also a * bit faster than the DOM-based implementation. */ +@SuppressFBWarnings(value = "REDOS", justification="Tested the Regex against saferegex and safe-regex and not vulnerable") public class MagicSAXFilter extends DefaultFilter implements XMLDocumentFilter { private static enum Ops { CSS, FILTER, REMOVE, TRUNCATE, KEEP, ENCODE } private final Stack operations = new Stack(); - private List errorMessages = new ArrayList(); + private final List errorMessages = new ArrayList(); private StringBuffer cssContent = null; private XMLAttributes cssAttributes = null; private CssScanner cssScanner = null; @@ -91,7 +94,6 @@ public void reset(InternalPolicy instance){ cssAttributes = null; cssScanner = null; inCdata = false; - } public void characters(XMLString text, Augmentations augs) throws XNIException { diff --git a/src/main/java/org/owasp/validator/html/util/URIUtils.java b/src/main/java/org/owasp/validator/html/util/URIUtils.java index d3149617..9f00b582 100644 --- a/src/main/java/org/owasp/validator/html/util/URIUtils.java +++ b/src/main/java/org/owasp/validator/html/util/URIUtils.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2007-2019, Arshan Dabirsiaghi, Jason Li + * Copyright (c) 2007-2020, Arshan Dabirsiaghi, Jason Li * * All rights reserved. * @@ -24,6 +24,8 @@ package org.owasp.validator.html.util; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + import java.io.File; import java.net.MalformedURLException; import java.net.URL; @@ -58,6 +60,8 @@ public class URIUtils { */ private static final String PARENT_DIR_OP = ".."; + @SuppressFBWarnings(value = "SECURITY",justification="The 2x Path Traversal warnings related to the use of" + + " new File(href) are not vulnerabilities as no data is read or written.") public static String resolveAsString(String href, String documentBase) { try { @@ -68,7 +72,6 @@ public static String resolveAsString(String href, String documentBase) { } catch (MalformedURLException muex) { } - //-- join document base + href String absolute = null; if ((documentBase != null) && (documentBase.length() > 0)) { @@ -79,12 +82,10 @@ public static String resolveAsString(String href, String documentBase) { absolute = documentBase + HREF_PATH_SEP + href; } - } else { absolute = href; } - try { //-- try to create a new URL and see if MalformedURLExcetion is //-- ever thrown @@ -106,10 +107,8 @@ public static String resolveAsString(String href, String documentBase) { return absolute; } } - } - // Try local files String fileURL = absolute; File iFile = new File(href); @@ -124,14 +123,14 @@ public static String resolveAsString(String href, String documentBase) { //-- one last sanity check try { - //-- try to create a new URL and see if MalformedURLExcetion is + //-- try to create a new URL and see if MalformedURLException is //-- ever thrown new URL(fileURL); return fileURL; } catch (MalformedURLException muex) { } - //-- At this point we we're unsucessful at trying to resolve + //-- At this point we were unsuccessful at trying to resolve //-- the href + documentbase, this could be due to a custom //-- protocol or typo in the URI, just return documentBase + //-- href @@ -174,10 +173,8 @@ public static String normalize(String absoluteURL) throw new MalformedURLException("invalid absolute URL: " + absoluteURL); } tokens.pop(); - } else { - if (!CURRENT_DIR_OP.equals(token)) { - tokens.push(token); - } + } else if (!CURRENT_DIR_OP.equals(token)) { + tokens.push(token); } last = token; } @@ -211,13 +208,12 @@ private static String createFileURL(String filename) { for (int i = 0; i < chars.length; i++) { char ch = chars[i]; switch (ch) { - case '\\': - sb.append(HREF_PATH_SEP); - break; - default: - sb.append(ch); - break; - + case '\\': + sb.append(HREF_PATH_SEP); + break; + default: + sb.append(ch); + break; } } return sb.toString(); diff --git a/src/main/resources/antisamy-anythinggoes.xml b/src/main/resources/antisamy-anythinggoes.xml index 8a98c284..7382b5c1 100644 --- a/src/main/resources/antisamy-anythinggoes.xml +++ b/src/main/resources/antisamy-anythinggoes.xml @@ -50,7 +50,7 @@ http://www.w3.org/TR/html401/struct/global.html - + @@ -738,8 +738,6 @@ http://www.w3.org/TR/html401/struct/global.html - - @@ -2474,18 +2472,6 @@ http://www.w3.org/TR/html401/struct/global.html - - - - - - - - - - - - diff --git a/src/main/resources/antisamy-ebay.xml b/src/main/resources/antisamy-ebay.xml index 9f4ef15d..6beaae6f 100644 --- a/src/main/resources/antisamy-ebay.xml +++ b/src/main/resources/antisamy-ebay.xml @@ -48,7 +48,7 @@ http://www.w3.org/TR/html401/struct/global.html - + @@ -549,8 +549,6 @@ http://www.w3.org/TR/html401/struct/global.html - - @@ -2282,18 +2280,6 @@ http://www.w3.org/TR/html401/struct/global.html - - - - - - - - - - - - diff --git a/src/main/resources/antisamy-myspace.xml b/src/main/resources/antisamy-myspace.xml index e3adbab6..fbb17ba0 100644 --- a/src/main/resources/antisamy-myspace.xml +++ b/src/main/resources/antisamy-myspace.xml @@ -50,7 +50,7 @@ http://www.w3.org/TR/html401/struct/global.html - + @@ -711,8 +711,6 @@ http://www.w3.org/TR/html401/struct/global.html - - @@ -2439,18 +2437,6 @@ http://www.w3.org/TR/html401/struct/global.html - - - - - - - - - - - - diff --git a/src/main/resources/antisamy-slashdot.xml b/src/main/resources/antisamy-slashdot.xml index 4e32d028..b479dd66 100644 --- a/src/main/resources/antisamy-slashdot.xml +++ b/src/main/resources/antisamy-slashdot.xml @@ -34,7 +34,7 @@ Slashdot allowed tags taken from "Reply" page: --> - + diff --git a/src/main/resources/antisamy-tinymce.xml b/src/main/resources/antisamy-tinymce.xml index d565628a..fd47c968 100644 --- a/src/main/resources/antisamy-tinymce.xml +++ b/src/main/resources/antisamy-tinymce.xml @@ -29,7 +29,7 @@ - + diff --git a/src/main/resources/antisamy.xml b/src/main/resources/antisamy.xml index 7c5a1522..e9b403b0 100644 --- a/src/main/resources/antisamy.xml +++ b/src/main/resources/antisamy.xml @@ -54,19 +54,19 @@ http://www.w3.org/TR/html401/struct/global.html - + - + - + - + @@ -759,8 +759,6 @@ http://www.w3.org/TR/html401/struct/global.html - - @@ -2601,18 +2599,6 @@ http://www.w3.org/TR/html401/struct/global.html - - - - - - - - - - - - diff --git a/src/site/resources/images/owasp.png b/src/site/resources/images/owasp.png new file mode 100644 index 00000000..90fb9d2c Binary files /dev/null and b/src/site/resources/images/owasp.png differ diff --git a/src/site/site.xml b/src/site/site.xml new file mode 100644 index 00000000..d77aba25 --- /dev/null +++ b/src/site/site.xml @@ -0,0 +1,26 @@ + + + + /images/owasp.png + https://owasp.org/www-project-antisamy/ + + + org.apache.maven.skins + maven-fluido-skin + 1.9 + + + + false + true + + + + + + + + + + diff --git a/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java b/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java index 328eda6f..d1f46370 100644 --- a/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java +++ b/src/test/java/org/owasp/validator/html/test/AntiSamyTest.java @@ -31,12 +31,12 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.MatcherAssert.assertThat; import org.junit.Before; import org.junit.Test; @@ -101,7 +101,6 @@ public class AntiSamyTest { private AntiSamy as = new AntiSamy(); private TestPolicy policy = null; - @Before public void setUp() throws Exception { @@ -115,15 +114,12 @@ public void setUp() throws Exception { policy = TestPolicy.getInstance(url); } - @Test public void SAX() { try { CleanResults cr = as.scan("testtest thsidfshidf