[
https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16848199#comment-16848199
]
Jacques Le Roux commented on OFBIZ-5254:
----------------------------------------
As mentionned by [~mthl] on dev ml the testCreateNewRequest was failing when
testing framework only. It was due to escaped single quotes in related data in
OrderTypeData.xml: {{subject="OFBiz - Your Request is received:
'${custRequestName}' #CR${custRequestId}"}}
This was a peculiar case that could be generalised to all escapable characters.
The general solution is to compare the original value with the filtered value
unescaped in UtilCodec::checkStringForHtmlSafe.
BTW, weirdly enough StringEscapeUtils::escapeHtml4 does not escape single quote.
Another weirdness is the test was passing with plugins data loaded. This is due
to duplicated demo data in scrumTypeData.xml (which is actually not only type
data, as ever the scrum component is a mess, that's not new and always wonder
if we should not get rid of it!)
Fixed in
trunk r1859968
R18 r1859969 (conflict in build.gradle handled by hand)
R17 r1859970 (conflict in build.gradle handled by hand)
R16 r1859971 (conflict in build.gradle handled by hand)
> 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
> Fix For: 17.12.01, 16.11.06, 18.12.01
>
> Attachments: OFBIZ-5254.patch, OFBIZ-5254.patch, OFBIZ-5254.patch,
> OFBIZ-5254.patch, 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)
