[Javascript] CWE-348: Client supplied ip used in security check#6864
[Javascript] CWE-348: Client supplied ip used in security check#6864yabeow wants to merge 3 commits into
Conversation
|
Thanks. This generally LGTM. I have a few suggestions for code quality improvements. (I trust that all of the learnings from the two other PRs have made it into this PR.) |
| node.asExpr().(MethodCallExpr).getMethodName() = "split" and | ||
| source = node and | ||
| not aa.getIndex().getIntValue() = 0 and | ||
| source.flowsTo(aa.getAChildExpr().flow()) |
There was a problem hiding this comment.
getAChildExpr should generally not be used, but rather something like getIndex as seen above. What is being expressed in this final conjunct?
There was a problem hiding this comment.
I used getAChildExpr to get the variable being indexed. I think this is the correct way to do this, do you have any other suggestions?
This is the AST tree of temp[temp.length - 1];
There was a problem hiding this comment.
I see now, thanks for clarifying. We should just stick to the DataFlow library then. Something like the following should do the trick:
exists(DataFlow::MethodCallNode split, DataFlow::PropRead read |
split = node and
 split.getMethodName() = "split" and // n.split(...)
read = split.getAPropertyRead() // <n.split(...)>[...]
 not read.getPropertyNameExpr().getIntValue() = 0 and
There was a problem hiding this comment.
Hi @esbena,
Sorry for the late reply, I have modified the isSanitizer part and it should look good now. Thanks for your awesome suggestions. Do you have a moment to take a look at it again?
There was a problem hiding this comment.
Sorry about the delay. I'll take a look now.
9576171 to
a818580
Compare
esbena
left a comment
There was a problem hiding this comment.
Thank you, there's one http/HTTP casing mistake left to fix. I have started a final performance check of the query, I'll approve once it looks good.
…IpUsedInSecurityCheck.qhelp Co-authored-by: Esben Sparre Andreasen <esbena@github.com>
|
Have you run this query with vscode and viewed the results? Example: |
|
Draft-stating the PR due to inactivity, but feel free to undraft when you have implemented the requested changes. |
Hi there,
This merge request ports these two similar queries to Javascript:
I also used local data flow to detect more sanitized cases in Javascript.