Christoph Neuroth created OFBIZ-5254:
----------------------------------------
Summary: 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
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 is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
