[ 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