[jira] [Comment Edited] (OFBIZ-5254) Services allow arbitrary HTML for parameters with allow-html set to "safe"
[
https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13806604#comment-13806604
]
Jacques Le Roux edited comment on OFBIZ-5254 at 10/28/13 8:18 AM:
--
== "none" is the default ==
Actually when you think about it, after
[r751990|http://svn.apache.org/viewvc?view=revision&revision=751990],
<> and <> are the same: they do nothing!
The only difference is the warning message from the OWASP Antisamy
IntrusionDetector, which is both, as Christoph noted "giving you a false sense
of security" or as I wrote "disturbing, wrong and useless". So there are no
longer any reasons for differencing "safe" and "any".
So I will:
* deprecate "safe" (making it clear in the XSD documentation), keeping only
"none" and "any". This is for backward compatibility, else we could completly
remove the misleading "safe" . Note that "none" is the default.
* to replace in services definition all allow-html="safe" by allow-html="any"
* to remove from ModelService.java (near line 587) the code which throws the
OWASP Antisamy IntrusionDetector message in log , I mean
{code}
} else if ("safe".equals(modelParam.allowHtml)) {
StringUtil.checkStringForHtmlSafeOnly(modelParam.name, value,
errorMessageList);
}
{code}
was (Author: jacques.le.roux):
Actually when you think about it, after
[r751990|http://svn.apache.org/viewvc?view=revision&revision=751990],
<> and <> are the same: they do nothing!
The only difference is the warning message from the OWASP Antisamy
IntrusionDetector, which is both, as Christoph noted "giving you a false sense
of security" or as I wrote "disturbing, wrong and useless". So there are no
longer any reasons for differencing "safe" and "any".
So I will:
* deprecate "safe" (making it clear in the XSD documentation), keeping only
"none" and "any". This is for backward compatibility, else we could completly
remove the misleading "safe" . Note that "none" is never used OOTB.
* to replace in services definition all allow-html="safe" by allow-html="any"
* to remove from ModelService.java (near line 587) the code which throws the
OWASP Antisamy IntrusionDetector message in log , I mean
{code}
} else if ("safe".equals(modelParam.allowHtml)) {
StringUtil.checkStringForHtmlSafeOnly(modelParam.name, value,
errorMessageList);
}
{code}
> 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 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
>
[jira] [Comment Edited] (OFBIZ-5254) Services allow arbitrary HTML for parameters with allow-html set to "safe"
[
https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13798892#comment-13798892
]
Jacques Le Roux edited comment on OFBIZ-5254 at 10/21/13 11:02 PM:
---
== CLARIFY ==
Hans, if you use allow-html="any" there are no worries for those characters.
All characters are allowed and since those are generated by OFBiz email
templates or such allow-html="any" is OK. If this was your only concern then I
will carefully put back allow-html="any" where it should be used.
Summary: for email services (and all secured one where OFBiz generates the
content) the only difference will be that the useless log warnings from ESAPI
will not show. Please read my comments above for more details..
was (Author: jacques.le.roux):
Hans, if you use any there are no worries for those characters. All are allowed
and since those are generated by OFBiz email templates or such allow-html="any"
is OK. If this was your only concern then I will carefully put back
allow-html="any" where it should be used.
Summary: for email services (and all secured one where OFBiz generates the
content) the only difference will be that the useless log warnings from ESAPI
will not show. Please read my comments above for more details..
> 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 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, yo
