ofbiz-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jacques Le Roux (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (OFBIZ-5254) Services allow arbitrary HTML for parameters with allow-html set to "safe"
Date Fri, 17 May 2019 08:50:00 GMT

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

Jacques Le Roux commented on OFBIZ-5254:

[^OFBIZ-5254.patch] is a new patch from today, the one I want to commit. I have only a concern.
I wanted to use labels instead of English text for the warning messages. But I then cross
issues with UtilCodecTests. So the patch contains labels that will maybe not used, hence not
committed. I'll check if I can do something about that before committing.

I have tested some (simple) scenarios and it should be OK. Anyway that can be easily extended
with CustomSafePolicy class.

> 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
> 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,
> {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
> {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

View raw message