[
https://issues.apache.org/jira/browse/VELTOOLS-126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13224527#comment-13224527
]
Christopher Schultz commented on VELTOOLS-126:
----------------------------------------------
Here's a stab at a patch. I'm not sure exactly at which level a configuration
option should be put (ErrorsTool, ActionMessagesTool, or MessageResourcesTool),
so any suggestions would be appreciated.
Here's the meat of the patch (against 2.0.x, but I think trunk is identical):
Index: src/main/java/org/apache/velocity/tools/struts/StrutsUtils.java
===================================================================
--- src/main/java/org/apache/velocity/tools/struts/StrutsUtils.java
(revision 1238717)
+++ src/main/java/org/apache/velocity/tools/struts/StrutsUtils.java
(working copy)
@@ -26,6 +26,7 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession;
+import org.apache.commons.lang.StringEscapeUtils;
import org.apache.struts.Globals;
import org.apache.struts.action.ActionForm;
import org.apache.struts.action.ActionMessage;
@@ -464,7 +465,7 @@
HttpSession session,
ServletContext application)
{
- return errorMarkup(property, null, request, session, application);
+ return errorMarkup(property, null, "HTML", request, session,
application);
}
@@ -483,6 +484,7 @@
* @since VelocityTools 1.1
* @return The formatted error message. If no error messages are queued,
* an empty string is returned.
+ * @deprecated Use {@link
#errorMarkup(String,String,String,HttpServletRequest,HttpSession,ServletContext)}
instead
*/
public static String errorMarkup(String property,
String bundle,
@@ -490,6 +492,38 @@
HttpSession session,
ServletContext application)
{
+ return errorMarkup(property, bundle, "HTML", request, session,
+ application);
+ }
+
+ /**
+ * Returns a formatted error message. The error message is assembled from
+ * the following three pieces: First, value of message resource
+ * "errors.header" is prepended. Then, the list of error messages is
+ * rendered. Finally, the value of message resource "errors.footer"
+ * is appended.
+ *
+ * @param property the category of errors to markup and return
+ * @param bundle the message resource bundle to use
+ * @param escape A string describing the type of escaping to apply to each
+ * error message's parameters (if any). Valid values are
+ * "HTML", "XML", and "ECMAScript" (case-insensitive).
+ * Note that the message itself is not escaped, only the
+ * parameters for replacements such as "{0}", etc. are
+ * escaped.
+ * @param request the servlet request
+ * @param session the HTTP session
+ * @param application the servlet context
+ * @since VelocityTools 1.1
+ * @return The formatted error message. If no error messages are queued,
+ * an empty string is returned.
+ */
+ public static String errorMarkup(String property,
+ String bundle,
+ String escape,
+ HttpServletRequest request,
+ HttpSession session, ServletContext
application)
+ {
ActionMessages errors = getErrors(request);
if (errors == null)
{
@@ -550,6 +584,8 @@
results.append(header);
results.append("\r\n");
+ Escaper escaper = null;
+
String message;
while (reports.hasNext())
{
@@ -557,9 +593,35 @@
ActionMessage report = (ActionMessage)reports.next();
if (resources != null && report.isResource())
{
+ Object[] parameters = report.getValues();
+
+ // Check to see if we need to escape the parameter values
+ // for this message. See VELTOOLS-126.
+ boolean copied = false;
+ if(null != parameters && 0 < parameters.length)
+ {
+ if(null == escaper)
+ escaper = getEscaper(escape);
+
+ for(int i=0; i<parameters.length; ++i)
+ {
+ // Only have to escape string-like values
+ if(parameters[i] instanceof CharSequence)
+ {
+ // Only duplicate the array if necessary
+ if(!copied)
+ {
+ parameters = (Object[])parameters.clone();
+ copied = true;
+ }
+ parameters[i] =
escaper.escape((CharSequence)parameters[i]);
+ }
+ }
+ }
+
message = resources.getMessage(locale,
report.getKey(),
- report.getValues());
+ parameters);
}
results.append(prefix);
@@ -570,7 +632,15 @@
}
else
{
- results.append(report.getKey());
+ // VELTOOLS-126: escape the key
+ CharSequence key = report.getKey();
+
+ if(null == escaper)
+ escaper = getEscaper(escape);
+
+ key = escaper.escape(key);
+
+ results.append(key);
}
results.append(suffix);
@@ -584,4 +654,61 @@
return results.toString();
}
+ static interface Escaper
+ {
+ public CharSequence escape(CharSequence cs);
+ }
+
+ private static class NOPEscaper
+ implements Escaper
+ {
+ public CharSequence escape(CharSequence cs)
+ {
+ return cs;
+ }
+ }
+
+ private static class HTMLEscaper
+ implements Escaper
+ {
+ public CharSequence escape(CharSequence cs)
+ {
+ return StringEscapeUtils.escapeHtml(cs.toString());
+ }
+ }
+
+ private static class XMLEscaper
+ implements Escaper
+ {
+ public CharSequence escape(CharSequence cs)
+ {
+ return StringEscapeUtils.escapeXml(cs.toString());
+ }
+ }
+
+ private static class ECMAScriptEscaper
+ implements Escaper
+ {
+ public CharSequence escape(CharSequence cs)
+ {
+ return StringEscapeUtils.escapeJavaScript(cs.toString());
+ }
+ }
+ private static final NOPEscaper NOP_ESCAPER = new NOPEscaper();
+ private static final HTMLEscaper HTML_ESCAPER = new HTMLEscaper();
+ private static final XMLEscaper XML_ESCAPER = new XMLEscaper();
+ private static final ECMAScriptEscaper ECMASCRIPT_ESCAPER = new
ECMAScriptEscaper();
+ private static Escaper getEscaper(String escape)
+ {
+ if("html".equalsIgnoreCase(escape))
+ return HTML_ESCAPER;
+ else if("xml".equalsIgnoreCase(escape))
+ return XML_ESCAPER;
+ else if ("ecmascript".equalsIgnoreCase(escape))
+ return ECMASCRIPT_ESCAPER;
+ else if(null != escape)
+ throw new IllegalArgumentException("Unrecognized escape: '" +
escape + "'");
+ else
+ return NOP_ESCAPER;
+ }
}
> XSS Vulnerability when using struts/ErrorsTool.getMsgs
> ------------------------------------------------------
>
> Key: VELTOOLS-126
> URL: https://issues.apache.org/jira/browse/VELTOOLS-126
> Project: Velocity Tools
> Issue Type: Bug
> Components: VelocityStruts
> Affects Versions: 1.4, 2.x
> Environment: Identified in velocity-tools 1.4, verified by reading
> code in trunk.
> Reporter: Christopher Schultz
>
> The code for ErrorsTool.getMsgs goes roughly like this:
> String message = message("errors.header");
> foreach(error) {
> message += message("errors.prefix") + error + message("errors.suffix")
> message += message("errors.footer")
> return message;
> This is easily open to an XSS attack when an error message contains user
> input.
> Honestly, I'm not entirely sure if we should even do anything about this,
> because the ErrorsTool is not strictly for use in an HTML context, so
> escaping the error message itself may not be appropriate. Also, the message
> itself may contain markup which the developer wants to remain, while the user
> input should be escaped.
> It's possible that the solution to this problem is to put a big warning on
> the tool that XSS attacks are very easy using this tool.
> I've been running with it for years, and didn't notice until today. I
> replaced my use of errors.getMsgs with this:
> $!msg.errors.header
> #foreach($error in $errors.get($fieldName))
> $!msg.errors.prefix#htmlEscape($error)$!msg.errors.suffix
> #end
> $!msg.errors.header
> ...which is appropriate for my uses, but might not work for everyone.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
