[
https://issues.apache.org/jira/browse/VELTOOLS-126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13224501#comment-13224501
]
Christopher Schultz commented on VELTOOLS-126:
----------------------------------------------
ErrorsTool now uses StrutsUtils.errorMarkup whose code looks a little different:
String message;
while (reports.hasNext())
{
message = null;
ActionMessage report = (ActionMessage)reports.next();
if (resources != null && report.isResource())
{
// TODO: VELTOOLS-126 - XSS vulnerability here
// TODO: unless we HTML-escape some stuff
message = resources.getMessage(locale,
report.getKey(),
report.getValues());
}
results.append(prefix);
if (message != null)
{
results.append(message);
}
else
{
// TODO: VELTOOLS-126: HTML-escape the key
results.append(report.getKey());
}
results.append(suffix);
results.append("\r\n");
}
I think if you look at the TODO comments I've added, it makes sense to
HTML-escape the values passed to resources.getMessage() and the key if there is
no message. Of course, ErrorsTool can be used in non-HTML contexts, so the
whole thing must be configurable. I'll take Nathan's suggestion of making it a
tool-parameter, but I think it makes sense to make the default configuration to
escape HTML since this is a security (XSS) issue.
> 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: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org
