[
https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13793644#comment-13793644
]
Jacques Le Roux commented on OFBIZ-5254:
----------------------------------------
I guess you stumbled upon something like this:
{code}
2013-10-12 19:30:05,000 (OFBiz-JobQueue-0) [ EmailServices.java:107:INFO ]
SendMail Running, for communicationEventId : 10010
2013-10-12 19:30:05,062 (OFBiz-JobQueue-0) [ EmailServices.java:264:INFO ]
2 multiparts found
2013-10-12 19:30:05,062 (OFBiz-JobQueue-0) [ EmailServices.java:270:INFO ]
part of type: text/html and size: 10377
2013-10-12 19:30:05,062 (OFBiz-JobQueue-0) [ EmailServices.java:274:INFO ]
part of type: application/pdf and size: 6583
2013-10-12 19:30:06,171 (OFBiz-JobQueue-0) [ ServiceEcaRule.java:157:INFO ]
Running Service ECA Service: updateCommEventAfterEmail, triggered by rule on
Service: sendMailMultiPart
Oct 12, 2013 7:30:06 PM AppNameNotSpecified IntrusionDetector
WARNING: SECURITY-FAILURE Anonymous@unknown:unknown -- Invalid HTML input:
context=content, errors=[The <b>html</b> tag has been filtered for security
reasons. The contents of the tag will remain in place., T
he <b>head</b> tag has been filtered for security reasons. The contents of the
tag will remain in place., The <b>meta</b> tag has been filtered for security
reasons. The contents of the tag will remain in pla
ce., The <b>title</b> tag has been filtered for security reasons. The contents
of the tag will remain in place., The <b>style</b> tag has been filtered for
security reasons. The contents of the tag will remai
n in place., The <b>body</b> tag has been filtered for security reasons. The
contents of the tag will remain in place., The <b>h1</b> tag has been filtered
for security reasons. The contents of the tag will r
emain in place., The <b>div</b> tag contained an attribute that we could not
process. The <b>id</b> attribute has been filtered out, but the tag is still in
place., The <b>div</b> tag contained an attribute t
hat we could not process. The <b>class</b> attribute has been filtered out, but
the tag is still in place., The <b>div</b> tag contained an attribute that we
could not process. The <b>class</b> attribute has
[...]
[java] ValidationException @
org.owasp.esapi.reference.DefaultValidator.getValidSafeHTML(null:-1)
{code}
Which I agree is disturbing, wrong and useless. Wrong and useless, because
actually the HTML input is not filtered, so no needs to worry anybody about,
even in log.
It comes from http://svn.apache.org/viewvc?view=revision&revision=751990. So it
was not done by chance, only the effect in log was overwieved.
I decided to not change the "none" case (I also noticed we have any in services
definitions) but to simplify the "safe" case to not log an useless, confusing
message. For that I simply replaced
[getValidSafeHTML()|https://code.google.com/p/owasp-esapi-java/source/browse/tags/releases/1.4.0/source/src/org/owasp/esapi/reference/DefaultValidator.java?r=1650#342]
by
[isValidSafeHTML()|https://code.google.com/p/owasp-esapi-java/source/browse/tags/releases/1.4.0/source/src/org/owasp/esapi/reference/DefaultValidator.java?r=1650#233].
But then I realised that this was not a good solution because then email
services, like sendConfirmationEmail which calls numerous email services either
directly or in a transitive way, will not succeed; due to presence of not HTML
safe code in the body. I guess because we set our defaultWebEncoder with an
HTMLEntityCodec (and PercentCodec). This is why the IntrusionDetector throws
"Invalid HTML input" in log when actually the HTML is safe for us since it's
generated from our own email templates and there are no ways (at this stage) to
change anything about it.
I had a closer look and for me the reason is related to a email services
changes Hans did (to allow-html="safe") from previous (allow-html="any ")
settings I did before. Like for instance (not complete I believe):
Me: http://svn.apache.org/viewvc?view=revision&revision=r743100 (see texts
changed)
Hans: http://svn.apache.org/viewvc?view=revision&revision=r789506
So, at this stage, I see 3 options:
# Let it like that
# Remove "safe HTML" check in ModelService.vaidate()
# Changes all email related services which uses allow-html="safe" to
allow-html="any" when we are sure it's generated from our own email templates
and there are no ways to change them inside the OFBiz process which generates
them.
For me the solution 3 is the better one. Maybe Hans had good reasons for those
changes. But I wonder what they were. Because, at least dor the one above, they
have been done after David's one above so they were already useless. They only
show a log message which has no meanings, since nothing it's done about it, and
moreover nothing is actually wanted.
Opinions?
> 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
> Assignee: Jacques Le Roux
> Labels: security
>
> 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
(v6.1#6144)
