[ 
https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Jacques Le Roux updated OFBIZ-5254:
-----------------------------------
    Attachment: OFBIZ-5254.patch

> 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: Trunk
>            Reporter: Christoph Neuroth
>            Assignee: Jacques Le Roux
>            Priority: Major
>              Labels: security
>         Attachments: OFBIZ-5254.patch
>
>
> 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
(v7.6.3#76005)

Reply via email to