[jira] [Commented] (OFBIZ-5254) Services allow arbitrary HTML for parameters with allow-html set to "safe"

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13793649#comment-13793649 ]

Jacques Le Roux commented on OFBIZ-5254:
----------------------------------------

Here is the patch I'd have committed at some point, but please don't
{code}
Index: base/src/org/ofbiz/base/util/StringUtil.java
===================================================================
--- base/src/org/ofbiz/base/util/StringUtil.java (revision 1531448)
+++ base/src/org/ofbiz/base/util/StringUtil.java (working copy)
@@ -619,34 +619,6 @@
             errorMessageList.add("In field [" + valueName + "] less-than (<) and greater-than (>) symbols are not allowed.");
         }
 
-        /* NOTE DEJ 20090311: After playing with this more this doesn't seem to be necessary; the canonicalize will convert all such characters into actual text before this check is done, including other illegal chars like &lt; which will canonicalize to < and then get caught
-        // check for & followed a semicolon within 7 characters, no spaces in-between (and perhaps other things sometime?)
-        int curAmpIndex = value.indexOf("&");
-        while (curAmpIndex > -1) {
-            int semicolonIndex = value.indexOf(";", curAmpIndex + 1);
-            int spaceIndex = value.indexOf(" ", curAmpIndex + 1);
-            if (semicolonIndex > -1 && (semicolonIndex - curAmpIndex <= 7) && (spaceIndex < 0 || (spaceIndex > curAmpIndex && spaceIndex < semicolonIndex))) {
-                errorMessageList.add("In field [" + valueName + "] the ampersand (&) symbol is only allowed if not used as an encoded character: no semicolon (;) within 7 spaces or there is a space between.");
-                // once we find one like this we have the message so no need to check for more
-                break;
-            }
-            curAmpIndex = value.indexOf("&", curAmpIndex + 1);
-        }
-         */
-
-        /* NOTE DEJ 20090311: After playing with this more this doesn't seem to be necessary; the canonicalize will convert all such characters into actual text before this check is done, including other illegal chars like %3C which will canonicalize to < and then get caught
-        // check for % followed by 2 hex characters
-        int curPercIndex = value.indexOf("%");
-        while (curPercIndex >= 0) {
-            if (value.length() > (curPercIndex + 3) && UtilValidate.isHexDigit(value.charAt(curPercIndex + 1)) && UtilValidate.isHexDigit(value.charAt(curPercIndex + 2))) {
-                errorMessageList.add("In field [" + valueName + "] the percent (%) symbol is only allowed if followed by a space.");
-                // once we find one like this we have the message so no need to check for more
-                break;
-            }
-            curPercIndex = value.indexOf("%", curPercIndex + 1);
-        }
-         */
-
         // TODO: anything else to check for that can be used to get HTML or JavaScript going without these characters?
 
         return value;
@@ -668,6 +640,40 @@
     }
 
     /**
+     * Uses a white-list approach to check for safe HTML.
+     * Based on the ESAPI validator configured in the antisamy-esapi.xml file.
+     *
+     * @param valueName "input" field
+     * @param value "input" field value
+     * @return Boolean true if safe HTML.
+     */
+    public static Boolean isStringForHtmlSafeOnly(String valueName, String value) {
+        return defaultWebValidator.isValidSafeHTML(valueName, value, Integer.MAX_VALUE, true);
+    }
+
+    /**
+     * Uses a white-list approach to check for strict HTML.
+     * Based on the ESAPI validator configured in the antisamy-esapi.xml file.
+     *
+     * @param valueName "input" field
+     * @param value "input" field value
+     * @return Boolean true if strict HTML.
+     */
+    public static Boolean isStringForHtmlStrictNone(String valueName, String value) {
+        if (UtilValidate.isEmpty(value)) return true;
+        String verified = null;
+        try {
+            verified = defaultWebEncoder.canonicalize(value, true);
+        } catch (IntrusionException e) {
+            return false;
+        }
+        if (verified.indexOf("<") >= 0 || verified.indexOf(">") >= 0) {
+            return false;
+        }        
+        return true;
+    }
+
+    /**
      * Remove/collapse multiple newline characters
      *
      * @param str string to collapse newlines in
Index: service/src/org/ofbiz/service/ModelService.java
===================================================================
--- service/src/org/ofbiz/service/ModelService.java (revision 1531448)
+++ service/src/org/ofbiz/service/ModelService.java (working copy)
@@ -585,7 +585,9 @@
                     if ("none".equals(modelParam.allowHtml)) {
                         StringUtil.checkStringForHtmlStrictNone(modelParam.name, value, errorMessageList);
                     } else if ("safe".equals(modelParam.allowHtml)) {
-                        StringUtil.checkStringForHtmlSafeOnly(modelParam.name, value, errorMessageList);
+//                        if (!StringUtil.isStringForHtmlSafeOnly(modelParam.name, value)) {
+//                            throw new ServiceValidationException("The string is not HTML safe", this);
+//                        }
                     }
                 }
             }
{code}

> Services allow arbitrary HTML for parameters with allow-html set to "safe"
> --------------------------------------------------------------------------
>
>                 Key: OFBIZ-5254
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-5254
>             Project: OFBiz
>          Issue Type: Bug
>          Components: framework
>    Affects Versions: SVN trunk
>            Reporter: Christoph Neuroth
>            Assignee: Jacques Le Roux
>              Labels: security
>
> For any given service with allow-html=safe parameters, the parameter data is not properly validated. See Model.Service.java:588:
> {code}
>                         StringUtil.checkStringForHtmlSafeOnly(modelParam.name, value, errorMessageList);
> {code}
> Looking at that method:
> {code}
>     public static String checkStringForHtmlSafeOnly(String valueName, String value, List<String> errorMessageList) {
>         ValidationErrorList vel = new ValidationErrorList();
>         value = defaultWebValidator.getValidSafeHTML(valueName, value, Integer.MAX_VALUE, true, vel);
>         errorMessageList.addAll(UtilGenerics.checkList(vel.errors(), String.class));
>         return value;
>     }
> {code}
> you can see that it expects the defaultWebValidator.getValidSafeHTML would add all validation errors to the given ValidationErrorList, but if you look at the implementation of ESAPI that is not the case. First, consider the overloaded getValidSafeHTML that takes the ValidationErrorList:
> {code} public String getValidSafeHTML(String context, String input, int maxLength, boolean allowNull, ValidationErrorList errors) throws IntrusionException {
> try {
> return getValidSafeHTML(context, input, maxLength, allowNull);
> } catch (ValidationException e) {
> errors.addError(context, e);
> }
> return input;
> }
> {code}
> Then, step into that method to see that ValidationExceptions are only thrown for things like exceeding the maximum length - not for policy violations that can be "cleaned", such as tags that are not allowed by the policy:
> {code}
> AntiSamy as = new AntiSamy();
> CleanResults test = as.scan(input, antiSamyPolicy);
> List errors = test.getErrorMessages();
> if ( errors.size() > 0 ) {
> // just create new exception to get it logged and intrusion detected
> new ValidationException( "Invalid HTML input: context=" + context, "Invalid HTML input: context=" + context + ", errors=" + errors, context );
> }
> {code}
> I guess that is an expected, although maybe not clearly documented behavior of ESAPI: Non-cleanable violations throw the exception and therefore will fail the ofbiz service, while non-allowed tags are cleaned. However, if you consider ModelService:588 and following lines again:
> {code}                        StringUtil.checkStringForHtmlSafeOnly(modelParam.name, value, errorMessageList);
> //(...)
>             if (errorMessageList.size() > 0) {
>                 throw new ServiceValidationException(errorMessageList, this, mode);
>             }
> {code}
> the cleaned return value is ignored. Therefore, you will see an "IntrusionDetection" in the logs, giving you a false sense of security but the unfiltered HTML will still go into the service. So, if you want the service to fail if non-allowed HTML is encountered, you should use isValidSafeHTML instead. If you want the incoming HTML to be filtered, you should use the return value of getValidSafeHTML.
> Some additional notes on this:
> * When changing this, it should be properly documented as users may well be relying on this behavior - for example, we send full HTML mails to our customers for their ecommerce purchases and require HTML to go through - so maybe for services like the communicationEvents allowing only safe HTML might not be desired.
> * The ESAPI code samples above are from version 1.4.4. I was really surprised to find a JAR that is not only outdated, but patched and built by a third party, without even indicating that in the filename in OfBiz trunk. This has been there for years (see OFBIZ-3135) and should really be replaced with an official, up to date version since that issue was fixed upstream years ago.

--
This message was sent by Atlassian JIRA
(v6.1#6144)