[
https://issues.apache.org/jira/browse/OFBIZ-5254?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13793649#comment-13793649
]
Jacques Le Roux commented on OFBIZ-5254:
----------------------------------------
Here is the patch I'd have committed at some point, but please don't
{code}
Index: base/src/org/ofbiz/base/util/StringUtil.java
===================================================================
--- base/src/org/ofbiz/base/util/StringUtil.java (revision 1531448)
+++ base/src/org/ofbiz/base/util/StringUtil.java (working copy)
@@ -619,34 +619,6 @@
errorMessageList.add("In field [" + valueName + "] less-than (<)
and greater-than (>) symbols are not allowed.");
}
- /* NOTE DEJ 20090311: After playing with this more this doesn't seem
to be necessary; the canonicalize will convert all such characters into actual
text before this check is done, including other illegal chars like < which
will canonicalize to < and then get caught
- // check for & followed a semicolon within 7 characters, no spaces
in-between (and perhaps other things sometime?)
- int curAmpIndex = value.indexOf("&");
- while (curAmpIndex > -1) {
- int semicolonIndex = value.indexOf(";", curAmpIndex + 1);
- int spaceIndex = value.indexOf(" ", curAmpIndex + 1);
- if (semicolonIndex > -1 && (semicolonIndex - curAmpIndex <= 7) &&
(spaceIndex < 0 || (spaceIndex > curAmpIndex && spaceIndex < semicolonIndex))) {
- errorMessageList.add("In field [" + valueName + "] the
ampersand (&) symbol is only allowed if not used as an encoded character: no
semicolon (;) within 7 spaces or there is a space between.");
- // once we find one like this we have the message so no need
to check for more
- break;
- }
- curAmpIndex = value.indexOf("&", curAmpIndex + 1);
- }
- */
-
- /* NOTE DEJ 20090311: After playing with this more this doesn't seem
to be necessary; the canonicalize will convert all such characters into actual
text before this check is done, including other illegal chars like %3C which
will canonicalize to < and then get caught
- // check for % followed by 2 hex characters
- int curPercIndex = value.indexOf("%");
- while (curPercIndex >= 0) {
- if (value.length() > (curPercIndex + 3) &&
UtilValidate.isHexDigit(value.charAt(curPercIndex + 1)) &&
UtilValidate.isHexDigit(value.charAt(curPercIndex + 2))) {
- errorMessageList.add("In field [" + valueName + "] the percent
(%) symbol is only allowed if followed by a space.");
- // once we find one like this we have the message so no need
to check for more
- break;
- }
- curPercIndex = value.indexOf("%", curPercIndex + 1);
- }
- */
-
// TODO: anything else to check for that can be used to get HTML or
JavaScript going without these characters?
return value;
@@ -668,6 +640,40 @@
}
/**
+ * Uses a white-list approach to check for safe HTML.
+ * Based on the ESAPI validator configured in the antisamy-esapi.xml file.
+ *
+ * @param valueName "input" field
+ * @param value "input" field value
+ * @return Boolean true if safe HTML.
+ */
+ public static Boolean isStringForHtmlSafeOnly(String valueName, String
value) {
+ return defaultWebValidator.isValidSafeHTML(valueName, value,
Integer.MAX_VALUE, true);
+ }
+
+ /**
+ * Uses a white-list approach to check for strict HTML.
+ * Based on the ESAPI validator configured in the antisamy-esapi.xml file.
+ *
+ * @param valueName "input" field
+ * @param value "input" field value
+ * @return Boolean true if strict HTML.
+ */
+ public static Boolean isStringForHtmlStrictNone(String valueName, String
value) {
+ if (UtilValidate.isEmpty(value)) return true;
+ String verified = null;
+ try {
+ verified = defaultWebEncoder.canonicalize(value, true);
+ } catch (IntrusionException e) {
+ return false;
+ }
+ if (verified.indexOf("<") >= 0 || verified.indexOf(">") >= 0) {
+ return false;
+ }
+ return true;
+ }
+
+ /**
* Remove/collapse multiple newline characters
*
* @param str string to collapse newlines in
Index: service/src/org/ofbiz/service/ModelService.java
===================================================================
--- service/src/org/ofbiz/service/ModelService.java (revision 1531448)
+++ service/src/org/ofbiz/service/ModelService.java (working copy)
@@ -585,7 +585,9 @@
if ("none".equals(modelParam.allowHtml)) {
StringUtil.checkStringForHtmlStrictNone(modelParam.name, value,
errorMessageList);
} else if ("safe".equals(modelParam.allowHtml)) {
- StringUtil.checkStringForHtmlSafeOnly(modelParam.name,
value, errorMessageList);
+// if
(!StringUtil.isStringForHtmlSafeOnly(modelParam.name, value)) {
+// throw new ServiceValidationException("The string
is not HTML safe", this);
+// }
}
}
}
{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<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)
