ESAPI for XSS prevention not working
I am working on fixing Cross site scripting issues in our code mainly in JSPS.
Below is the original code
//scriplet code
<% String userId = request.getParameter("sid");
...%>
and in the same Jsp they have
<input type = hidden name = "userID" value = "<%= userId %>" />
I have made changes to include esapi-2.1.0.jar in lib and ESAPI.properties, validation.properties in classpath. Then made below changes to scriplet code to fix the above code
//scriplet code
<% String userId = ESAPI.encoder().encodeForHTML(request.getParameter("sid"));
...%>
I thought this would fix the issue but when I scan my code using Fortify, these lines are again highlighted as having XSS issue. Please help if you guys have any idea on how this should be handled. Thanks.
------- UPDATE
Thanks a lot @avgvstvs. This is very insightful.Follwd guidelines, Not sure if I am missng somethn. Code -
String userSID=ESAPI.encoder().encodeForHTMLAttribute(request.getHeader("janus_sid")); session.setAttribute("username",userSID);<input type=hidden name="USERNAME" value="<%= userSID %>"
And for another varibale debug, below is the usage
String debugFlag = ESAPI.encoder().encodeForJavaScript(request.getParameter("debug"));var debugFlag = "<%= debugFlag%>";if(debugFlag == "y"){
document.title= title + " (" + host + ")";
defaultAppTitle = title + " (" + host + ")";
}
Latest Fortify scan still lists them as vulnerabilities :-(
Answer
The first question you need to ask should be "What code interpreters am I handing this value off to?"
Preventing XSS unfortunately isn't a recipe-based task, and from the looks of it--your application is using scriptlets, which aside from being bad Java practice, makes it much more difficult for static code-scanning tools like Fortify to give you accurate results. JSPs are compiled at runtime, but Fortify scans the source only. You should note that there should be future tasks/stories filed to refactor scriptlets to JSTL--you'll thank me for that later. THEN you can use esapi taglibs for these use cases. Fortify does an okay job of scanning pure Java code, and taglibs give you that.
ESAPI.encoder().encodeForHTML(userId)is only going to protect you from XSS in the use case where the variable in question is going to be placed between tags, like<span><%= userId %></span>This isn't the case you have.ESAPI.encoder().encodeForHTMLAttribute(userId)is what you want in your particular, narrow use-case. This is because escaping rules are different in Html attributes than they are for data placed within tags. This should fix your immediate problem.If the value is going to be used exclusively by JavaScript, then you want
ESAPI.encoder().encodeForJavaScript(userId)If the value is going to be renderable HTML being sent to a javascript function that will render your markup, like
element.innerHTML = “<HTML> Tags and markup”;you want<%=Encoder.encodeForJS(Encoder.encodeForHTML(userId))%>
This is just a few examples, here are a few more--but the overriding gist of my answer is this: You need to know the complete flow of data for every variable in your application and always encode for the appropriate output context. And in the security world, "context" means "Data is being handed off to a new code interpreter." If you implement this understanding well, you won't need Fortify anymore! :-)
Added complexity
In your comment you noted that the value is later being used by JavaScript. So the correct solution in this case would likely to be to fork two different variables, each one set for the proper encoding. For the HTML Attribute case:
String escapedHtmlUserId = ESAPI.encoder().encodeForHTMLAttribute(userId);
For the Javascript case:
String escapedJSUserId = ESAPI.encoder().encodeForJavaScript(userId);
Then use the values appropriately. If you used JSTL/taglibs, you could just use the right esapi taglib in the right place instead of splitting into two separate scriptlet variables.
AND ONE MORE THING
From the comment on the OP:
We have an initial scriptlet declaration:
<% String userId = ESAPI.encoder().encodeForHTML(request.getParameter("sid"));
...%>
and it is used later in a javascript function:
<%= ESAPI.encoder().encodeForJavascripl(userId)%>`
Just to point out, as presented, the userId variable stops being raw text on initialization. Effectively, the encoding for javascript becomes this:
<% ESAPI.encoder().encodeForJavascript(ESAPI.encoder().encodeForHTML(request.getParameter("sid")));
...%>
If the javascript is going to be rendering HTML using *.innerHTML() or *.outerHTML() this is fine. Otherwise, just note that the input has changed from its raw state.
Also, watch for the fact some JavaScript can undo what you've done and ensure your JavaScript isn't doing something similar.
==========More added code, more questions========
So we've traced all our data paths, we've double-checked that our JSP in question isn't being included in a different JSP that introduces new contexts for us to defend against, it's at this point where I'd smell "false positive," but if I were you I'd contact HP support and raise the issue for a false positive. I don't know enough of the specifics of your application to really be of much more use to you here unless I had access to the full source. Because you're dealing with scriptlets--its possible that Fortify can't trace the full data path from variable instantiation to eventual output, so is failing fast so it can at least warn you as to what its found "so far."