Java: CWE-1004 Query to check sensitive cookies without the HttpOnly flag set#5307
Conversation
owen-mc
left a comment
There was a problem hiding this comment.
I think the definition of the source should be much simpler and not specify what the source flows to, as that should be dealt with later. (But it would be okay to specify what flows to the source.) And the sanitizers are far too broad, quite possibly by mistake.
owen-mc
left a comment
There was a problem hiding this comment.
Great. The sources and sinks are looking much better. Now I have some suggestions for improving the sanitizers - by removing all of them! I think the logic naturally goes in other places.
|
@luchua-bc: We ran your query over a large number of projects on LGTM. There seem to be hundreds of projects that are getting flagged because they have cut 'n' pasted "Example 2" from this webpage. I am not an expert on CSRF, so please could you take a look at that example and confirm whether it is incorrect? Thanks, Kev. |
|
Hi @kevinbackhouse, Thanks for reviewing this PR. Code from "Example 2" on that page creates a cookie of Spring CSRF token. CSRF tokens are quite different from other tokens such as access tokens and session tokens, and they can be used in JavaScript. I've added one check to the query as well as a new test case to exclude CSRF tokens so that we can eliminate those FPs. |
|
Hi @luchua-bc: thanks for the update. It eliminated a lot of false positives. Looking through the new results, I am seeing some that are like this one, where they have added "HttpOnly" as a hard-coded string. Do you think you could eliminate those false positives too? |
|
@luchua-bc: please could contact me privately so I can share some more results? Not sure if they're true or false positives, but I would like to check them with you first. I am kevinbackhouse@github.com. |
|
@kevinbackhouse: I've checked all the sample projects, and have modified the query to accommodate their special cases. Now there is no FPs reported. There are four cases in total, some of which are not used that often but it's still nice to cover all of them to reduce FPs and make it a powerful query. They are:
Please retest and let me know if we need to test against more projects. Thanks, |
owen-mc
left a comment
There was a problem hiding this comment.
Two places where you should really be using taint flow rather than trying to imitate it yourself. Also, please squash Remove unused import into Update the query to accommodate more cases, if you are comfortable with editing git history. (You will have to force-push when you next push.)
5b084fb to
08c3bf2
Compare
owen-mc
left a comment
There was a problem hiding this comment.
That looks much better. Well done for realising you had to use TaintTracking2, which I probably should have mentioned. Just one thing to fix - making sure the source of a taint tracking configuration is constrained.
|
The checks are failing, but it's unrelated to this PR. |
smowton
left a comment
There was a problem hiding this comment.
Partial review, to be continued tomorrow. It would be helpful to give an outline of how this query works at the top of the .ql file, as it's a bit more complicated than expected from its description.
smowton
left a comment
There was a problem hiding this comment.
One last comment: suggest re-ordering the predicates and configurations here such that we get used predicates before their users (e.g. predicate f() { ... } should come before predicate g(...) { f(...) }). This would make this a bit easier to read.
|
Merged with slight amendments via #5663. |
Cross-Site Scripting (XSS) has been a popular attack for many years, which is ranked as one of the OWASP Top 10 Security Vulnerabilities 2020. Cookies holding sensitive information are vulnerable to XSS attacks if they don't have the
HttpOnlyflag set.The
HttpOnlyflag directs compatible browsers to prevent client-side script from accessing cookies. Including the HttpOnly flag in the Set-Cookie HTTP response header for a sensitive cookie helps mitigate the risk associated with XSS where an attacker's script code attempts to read the contents of a cookie and exfiltrate information obtained.The query detects all the following three scenarios of the Java EE framework:
HttpOnlynot set injavax.servlet.http.CookieHttpOnlynot set injavax.ws.rs.core.Cookieorjakarta.ws.rs.core.CookieHttpOnlynot directly set inresponse.addHeader("Set-Cookie", ...)orresponse.setHeader("Set-Cookie", ...)Please consider to merge the PR. Thanks.