[
https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16842065#comment-16842065
]
Jacques Le Roux commented on OFBIZ-5254:
----------------------------------------
I did not dig into it, but it's interesting to let a link about that here:
https://android.googlesource.com/platform/packages/apps/UnifiedEmail/+/ec0fa48/src/com/android/mail/utils/HtmlSanitizer.java
> 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: Critical
> Labels: security
> Attachments: OFBIZ-5254.patch, OFBIZ-5254.patch, UtilCodec.java
>
>
> 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)
