Issue 521 (ESAPI#535)

kwwall · web-flow · commit 422ba9f27ad1 · 2020-02-08T21:34:19.000-06:00
* Add formal deprecation policy. * Add property Validator.ValidationRule.getValid.ignore509Fix. Truly a kludge if there every was one. * Add static field VALIDATOR_IGNORE509 for kludge. * Address issue ESAPI#521 by splitting out failing JUnit test cases, testGetValidSafeHTML() and testIsValidSafeHTML() into separate test files. New files will be src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleLogsTest.java and src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleThrowsTest.java * Address issue ESAPI#521 by kludge to add backward-compatibility flag to restore the old behavior accidentally broken by the changes to address issue ESAPI#509. * Javadoc clarifications to address issue ESAPI#521 for behavior broken by issue ESAPI#509 commits. * New test files for GitHub issue ESAPI#521; initial commit. * Add additional sentence about ESAPI deprecation policy. * Since we've deprecated Log4J 1 logger, let's go all in and remove it from the default ESAPI.Logger in ESAPI.properties as well. * Changed new property name from the horribly named Validator.ValidationRule.getValid.ignore509Fix to the more appropriately named Validator.HtmlValidationAction whose possible values are "clean" (for legacy behavior) and "throw" for the new behavior as fixed by GitHub issue ESAPI#509. If the property is not encountered, it is treated as if "clean" had been specified, i.e., the legacy behavior. * Added string constant for new property, Validator.HtmlValidationAction. * Changes in keeping with new prop name, Validator.HtmlValidationAction * Rename JUnit test file. * Rename JUnit test class to sync w/ new file name. * Convert from JUnit 3 to JUnit 4.
diff --git a/README.md b/README.md
@@ -23,6 +23,10 @@ The default branch for ESAPI legacy is now the 'develop' branch (rather than the
 # Where can I find ESAPI 3.x?
 https://github.com/ESAPI/esapi-java
 
+# ESAPI Deprecation Policy
+Unless we unintentionally screw-up, our intent is to keep classes, methods, and/or fields whihc have been annotated as "@deprecated" for a minimum of two (2) years or until the next major release number (e.g., 3.x as of now), which ever comes first, before we remove them.
+Note that this policy does not apply to classes under the **org.owasp.esapi.reference** package. You are not expected to be using such classes directly in your code.
+
 # Contributing to ESAPI legacy
 ## How can I contribute or help with fix bugs?
 Fork and submit a pull request! Simple as pi! We generally only accept bug fixes, not new features because as a legacy project, we don't intend on adding new features, although we may make exceptions. If you wish to propose a new feature, the best place to discuss it is via the ESAPI-DEV mailing list mentioned below. Note that we vet all pull requests, including coding style of any contributions; use the same coding style found in the files you are already editing.
diff --git a/configuration/esapi/ESAPI.properties b/configuration/esapi/ESAPI.properties
@@ -67,8 +67,9 @@ ESAPI.Executor=org.owasp.esapi.reference.DefaultExecutor
 ESAPI.HTTPUtilities=org.owasp.esapi.reference.DefaultHTTPUtilities
 ESAPI.IntrusionDetector=org.owasp.esapi.reference.DefaultIntrusionDetector
 # Log4JFactory Requires log4j.xml or log4j.properties in classpath - http://www.laliluna.de/log4j-tutorial.html
-ESAPI.Logger=org.owasp.esapi.logging.log4j.Log4JLogFactory
-#ESAPI.Logger=org.owasp.esapi.reference.JavaLogFactory
+# Note that this is now considered deprecated!
+#ESAPI.Logger=org.owasp.esapi.logging.log4j.Log4JLogFactory
+ESAPI.Logger=org.owasp.esapi.logging.java.JavaLogFactory
 # To use the new SLF4J logger in ESAPI (see GitHub issue #129), set
 #    ESAPI.Logger=org.owasp.esapi.logging.slf4j.Slf4JLogFactory
 # and do whatever other normal SLF4J configuration that you normally would do for your application.
@@ -499,3 +500,38 @@ Validator.DirectoryName=^[a-zA-Z0-9:/\\\\!@#$%^&{}\\[\\]()_+\\-=,.~'` ]{1,255}$
 # Validation of dates. Controls whether or not 'lenient' dates are accepted.
 # See DataFormat.setLenient(boolean flag) for further details.
 Validator.AcceptLenientDates=false
+
+#                       ~~~~~ Important Note ~~~~~
+# This is a workaround to make sure that a commit to address GitHub issue #509
+# doesn't accidentally break someone's production code. So essentially what we
+# are doing is to reverting back to the previous possibly buggy (by
+# documentation intent at least), but, by now, expected legacy behavior.
+# Prior to the code changes for issue #509, if invalid / malicious HTML input was
+# observed, AntiSamy would simply attempt to sanitize (cleanse) it and it would
+# only be logged. However, the code change made ESAPI comply with its
+# documentation, which stated that a ValidationException should be thrown in
+# such cases. Unfortunately, changing this behavior--especially when no one is
+# 100% certain that the documentation was correct--could break existing code
+# using ESAPI so after a lot of debate, issue #521 was created to restore the
+# previous behavior, but still allow the documented behavior. (We did this
+# because it wasn't really causing an security issues since AntiSamy would clean
+# it up anyway and we value backward compatibility as long as it doesn't clearly
+# present security vulnerabilities.)
+# More defaults about this are written up under GitHub issue #521 and
+# the pull request it references. Future major releases of ESAPI (e.g., ESAPI 3.x)
+# will not support this previous behavior, but it will remain for ESAPI 2.x.
+# Set this to 'throw' if you want the originally intended behavior of throwing
+# that was fixed via issue #509. Set to 'clean' if you want want the HTML input
+# sanitized instead.
+#
+# Possible values:
+#   clean -- Use the legacy behavior where unsafe HTML input is logged and the
+#            sanitized (i.e., clean) input as determined by AntiSamy and your
+#            AntiSamy rules is returned. This is the default behavior if this
+#            new property is not found.
+#   throw -- The new, presumably correct and originally intended behavior where
+#            a ValidationException is thrown when unsafe HTML input is
+#            encountered.
+#
+#Validator.HtmlValidationAction=clean
+Validator.HtmlValidationAction=throw
diff --git a/src/main/java/org/owasp/esapi/ValidationRule.java b/src/main/java/org/owasp/esapi/ValidationRule.java
@@ -15,14 +15,23 @@ public interface ValidationRule {
 	 *            the value to be parsed
 	 * @return a validated value
 	 * @throws ValidationException
-	 *             if any validation rules fail
+	 *             if any validation rules fail, <i>except</i> if the
+     *             <b>{@code ESAPI.properties}></b> property
+     *             "Validator.ValidationRule.getValid.ignore509Fix" is set to
+     *             {@code true}, which is the default behavior for ESAPI 2.x
+     *             releases. See
+     *             {@link https://github.com/ESAPI/esapi-java-legacy/issues/509}
+     *             and {@link https://github.com/ESAPI/esapi-java-legacy/issues/521}
+     *             for futher details.
+     *
+     * @see #getValid(String context, String input, ValidationErrorList errorList)
 	 */
 	Object getValid(String context, String input)
 			throws ValidationException;
 
 	/**
-	 * Whether or not a valid valid can be null. getValid will throw an
-	 * Exception and getSafe will return the default value if flag is set to
+	 * Whether or not a valid valid can be null. {@code getValid} will throw an
+	 * Exception and {#code getSafe} will return the default value if flag is set to
 	 * true
 	 * 
 	 * @param flag
@@ -59,7 +68,8 @@ Object getValid(String context, String input,
 			ValidationErrorList errorList) throws ValidationException;
 
 	/**
-	 * Try to call get valid, then call sanitize, finally return a default value
+	 * Try to call {@code getvalid}, then call a 'sanitize' method for sanitization (if one exists),
+     * finally return a default value.
 	 */
 	Object getSafe(String context, String input);
 	
@@ -78,4 +88,4 @@ Object getValid(String context, String input,
 	 */
 	String whitelist(String input, Set<Character> list);
 
-}
+}
diff --git a/src/main/java/org/owasp/esapi/reference/DefaultSecurityConfiguration.java b/src/main/java/org/owasp/esapi/reference/DefaultSecurityConfiguration.java
@@ -114,7 +114,7 @@ public static SecurityConfiguration getInstance() {
     public static final String DIGITAL_SIGNATURE_ALGORITHM = "Encryptor.DigitalSignatureAlgorithm";
     public static final String DIGITAL_SIGNATURE_KEY_LENGTH = "Encryptor.DigitalSignatureKeyLength";
     			// ==================================//
-    			//		New in ESAPI Java 2.0		 //
+    			//		New in ESAPI Java 2.x		 //
     			// ================================= //
     public static final String PREFERRED_JCE_PROVIDER = "Encryptor.PreferredJCEProvider";
     public static final String CIPHER_TRANSFORMATION_IMPLEMENTATION = "Encryptor.CipherTransformation";
@@ -157,6 +157,7 @@ public static SecurityConfiguration getInstance() {
     public static final String VALIDATION_PROPERTIES = "Validator.ConfigurationFile";
     public static final String VALIDATION_PROPERTIES_MULTIVALUED = "Validator.ConfigurationFile.MultiValued";
     public static final String ACCEPT_LENIENT_DATES = "Validator.AcceptLenientDates";
+    public static final String VALIDATOR_HTML_VALIDATION_ACTION = "Validator.HtmlValidationAction";
 
     /**
      * Special {@code System} property that, if set to {@code true}, will
diff --git a/src/main/java/org/owasp/esapi/reference/validation/HTMLValidationRule.java b/src/main/java/org/owasp/esapi/reference/validation/HTMLValidationRule.java
@@ -97,8 +97,60 @@ public String sanitize( String context, String input ) {
 		return safe;
 	}
 
+    /**
+     * Check whether we want the legacy behavior ("clean") or the presumably intended
+     * behavior of "throw" for how to treat unsafe HTML input when AntiSamy is invoked.
+     * This admittedly is an UGLY hack to ensure that issue 509 and its corresponding
+     * fix in PR #510 does not break existing developer's existing code. Full
+     * details are described in GitHub issue 521.
+     *
+     * Checks new ESAPI property "Validator.HtmlValidationAction". A value of "clean"
+     * means to revert to legacy behavior. A value of "throw" means to use the new
+     * behavior as implemented in GitHub issue 509.
+     *
+     * @return false - If  "Validator.HtmlValidationAction" is set to "throw". Otherwise {@code true}.
+     * @since 2.2.1.0
+     */
+    private boolean legacyHtmlValidation() {
+        boolean legacy = true;          // Make legacy support the default behavior for backward compatibility.
+        String propValue = "clean";     // For legacy support.
+        try {
+            // DISCUSS:
+            // Hindsight: maybe we should have getBooleanProp(), getStringProp(),
+            // getIntProp() methods that take a default arg as well?
+            // At least for ESAPI 3.x.
+            propValue = ESAPI.securityConfiguration().getStringProp(
+                                // Future: This will be moved to a new PropNames class
+                            org.owasp.esapi.reference.DefaultSecurityConfiguration.VALIDATOR_HTML_VALIDATION_ACTION );
+            switch ( propValue.toLowerCase() ) {
+                case "throw":
+                    legacy = false;     // New, presumably correct behavior, as addressed by GitHub issue 509
+                    break;
+                case "clean":
+                    legacy = true;      // Give the caller that legacy behavior of sanitizing.
+                    break;
+                default:
+                    LOGGER.warning(Logger.EVENT_FAILURE, "ESAPI property " + 
+                                   org.owasp.esapi.reference.DefaultSecurityConfiguration.VALIDATOR_HTML_VALIDATION_ACTION +
+                                   " was set to \"" + propValue + "\".  Must be set to either \"clean\"" +
+                                   " (the default for legacy support) or \"throw\"; assuming \"clean\" for legacy behavior.");
+                    legacy = true;
+                    break;
+            }
+        } catch( ConfigurationException cex ) {
+            // OPEN ISSUE: Should we log this? I think so. Convince me otherwise. But maybe
+            //             we should only log it once or every Nth time??
+            LOGGER.warning(Logger.EVENT_FAILURE, "ESAPI property " + 
+                           org.owasp.esapi.reference.DefaultSecurityConfiguration.VALIDATOR_HTML_VALIDATION_ACTION +
+                           " must be set to either \"clean\" (the default for legacy support) or \"throw\"; assuming \"clean\"",
+                           cex);
+        }
+
+        return legacy;
+    }
+
 	private String invokeAntiSamy( String context, String input ) throws ValidationException {
-		// CHECKME should this allow empty Strings? "   " us IsBlank instead?
+		// CHECKME should this allow empty Strings? "   " use IsBlank instead?
 	    if ( StringUtilities.isEmpty(input) ) {
 			if (allowNull) {
 				return null;
@@ -114,7 +166,11 @@ private String invokeAntiSamy( String context, String input ) throws ValidationE
 
 			List<String> errors = test.getErrorMessages();
 			if ( !errors.isEmpty() ) {
-				throw new ValidationException( context + ": Invalid HTML input", "Invalid HTML input does not follow rules in antisamy-esapi.xml: context=" + context + " errors=" + errors.toString());
+                if ( legacyHtmlValidation() ) {        // See GitHub issues 509 and 521
+                    LOGGER.info(Logger.SECURITY_FAILURE, "Cleaned up invalid HTML input: " + errors );
+                } else {
+				    throw new ValidationException( context + ": Invalid HTML input", "Invalid HTML input does not follow rules in antisamy-esapi.xml: context=" + context + " errors=" + errors.toString());
+                }
 			}
 
 			return test.getCleanHTML().trim();
diff --git a/src/test/java/org/owasp/esapi/reference/ValidatorTest.java b/src/test/java/org/owasp/esapi/reference/ValidatorTest.java
@@ -238,39 +238,8 @@ public void testGetValidRedirectLocation() {
         // instance.getValidRedirectLocation(String, String, boolean, ValidationErrorList)
     }
 
-    public void testGetValidSafeHTML() throws Exception {
-        System.out.println("getValidSafeHTML");
-        Validator instance = ESAPI.validator();
-        ValidationErrorList errors = new ValidationErrorList();
-
-        // new school test case setup
-        HTMLValidationRule rule = new HTMLValidationRule("test");
-        ESAPI.validator().addRule(rule);
-
-        assertEquals("Test.", ESAPI.validator().getRule("test").getValid("test", "Test. <script>alert(document.cookie)</script>"));
-
-        String test1 = "<b>Jeff</b>";
-        String result1 = instance.getValidSafeHTML("test", test1, 100, false, errors);
-        assertEquals(test1, result1);
-
-        String test2 = "<a href=\"http://www.aspectsecurity.com\">Aspect Security</a>";
-        String result2 = instance.getValidSafeHTML("test", test2, 100, false, errors);
-        assertEquals(test2, result2);
-
-        String test3 = "Test. <script>alert(document.cookie)</script>";
-        assertEquals("Test.", rule.getSafe("test", test3));
-
-        assertEquals("Test. &lt;<div>load=alert()</div>", rule.getSafe("test", "Test. <<div on<script></script>load=alert()"));
-        assertEquals("Test. <div>b</div>", rule.getSafe("test", "Test. <div style={xss:expression(xss)}>b</div>"));
-        assertEquals("Test. alert(document.cookie)", rule.getSafe("test", "Test. <s%00cript>alert(document.cookie)</script>"));
-        assertEquals("Test. alert(document.cookie)", rule.getSafe("test", "Test. <s\tcript>alert(document.cookie)</script>"));
-        assertEquals("Test. alert(document.cookie)", rule.getSafe("test", "Test. <s\tcript>alert(document.cookie)</script>"));
-        // TODO: ENHANCE waiting for a way to validate text headed for an attribute for scripts
-        // This would be nice to catch, but just looks like text to AntiSamy
-        // assertFalse(instance.isValidSafeHTML("test", "\" onload=\"alert(document.cookie)\" "));
-        // String result4 = instance.getValidSafeHTML("test", test4);
-        // assertEquals("", result4);
-    }
+    //      Test split out and moved to HTMLValidationRuleLogsTest.java & HTMLValidationRuleThrowsTest.java
+    // public void testGetValidSafeHTML() throws Exception {
 
     public void testIsInvalidFilename() {
         System.out.println("testIsInvalidFilename");
@@ -881,32 +850,8 @@ public void testIsValidRedirectLocation() {
         //		isValidRedirectLocation(String, String, boolean)
     }
 
-    public void testIsValidSafeHTML() {
-        System.out.println("isValidSafeHTML");
-        Validator instance = ESAPI.validator();
-
-        assertTrue(instance.isValidSafeHTML("test", "<b>Jeff</b>", 100, false));
-        assertTrue(instance.isValidSafeHTML("test", "<a href=\"http://www.aspectsecurity.com\">Aspect Security</a>", 100, false));
-        assertTrue(instance.isValidSafeHTML("test", "Test. <script>alert(document.cookie)</script>", 100, false));
-        assertTrue(instance.isValidSafeHTML("test", "Test. <div style={xss:expression(xss)}>", 100, false));
-        assertTrue(instance.isValidSafeHTML("test", "Test. <s%00cript>alert(document.cookie)</script>", 100, false));
-        assertTrue(instance.isValidSafeHTML("test", "Test. <s\tcript>alert(document.cookie)</script>", 100, false));
-        assertTrue(instance.isValidSafeHTML("test", "Test. <s\r\n\0cript>alert(document.cookie)</script>", 100, false));
-
-        // TODO: waiting for a way to validate text headed for an attribute for scripts
-        // This would be nice to catch, but just looks like text to AntiSamy
-        // assertFalse(instance.isValidSafeHTML("test", "\" onload=\"alert(document.cookie)\" "));
-        ValidationErrorList errors = new ValidationErrorList();
-        assertTrue(instance.isValidSafeHTML("test1", "<b>Jeff</b>", 100, false, errors));
-        assertTrue(instance.isValidSafeHTML("test2", "<a href=\"http://www.aspectsecurity.com\">Aspect Security</a>", 100, false, errors));
-        assertTrue(instance.isValidSafeHTML("test3", "Test. <script>alert(document.cookie)</script>", 100, false, errors));
-        assertTrue(instance.isValidSafeHTML("test4", "Test. <div style={xss:expression(xss)}>", 100, false, errors));
-        assertTrue(instance.isValidSafeHTML("test5", "Test. <s%00cript>alert(document.cookie)</script>", 100, false, errors));
-        assertTrue(instance.isValidSafeHTML("test6", "Test. <s\tcript>alert(document.cookie)</script>", 100, false, errors));
-        assertTrue(instance.isValidSafeHTML("test7", "Test. <s\r\n\0cript>alert(document.cookie)</script>", 100, false, errors));
-        assertTrue(errors.size() == 0);
-
-    }
+    //      Test split out and moved to HTMLValidationRuleLogsTest.java & HTMLValidationRuleThrowsTest.java
+    // public void testIsValidSafeHTML() {
 
     public void testSafeReadLine() {
         System.out.println("safeReadLine");
diff --git a/src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleCleanTest.java b/src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleCleanTest.java
diff --git a/src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleThrowsTest.java b/src/test/java/org/owasp/esapi/reference/validation/HTMLValidationRuleThrowsTest.java
diff --git a/src/test/resources/esapi/ESAPI.properties b/src/test/resources/esapi/ESAPI.properties
