-
Notifications
You must be signed in to change notification settings - Fork 1.9k
JS: add new prototype pollution query and reorganize #4778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
erik-krogh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good query, nice results, great performance 👍
Just a few minor comments for now.
javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/semmle/javascript/security/dataflow/PrototypePollutingAssignment.qll
Outdated
Show resolved
Hide resolved
9aae46b to
254ac7f
Compare
|
Thanks for the review! Evaluation and rerun of slowest look good (internal inks). Added the links to the PR description as well. |
|
@mchammer01 may I ask you for a doc review? |
|
Yes sure, will try to fit this in later today or tomorrow. |
mchammer01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asgerf 👋🏻
This LGTM ✨
There's a tiny typo, and also, I think the CWE should be CWE-078 and not 079 on the new query)
Apart from that, I have made a few minor comments, feel free to ignore them if you don't agree 🙂
javascript/ql/src/Security/CWE-915/PrototypePollutingAssignment.ql
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-915/PrototypePollutingAssignment.qhelp
Outdated
Show resolved
Hide resolved
javascript/ql/src/Security/CWE-915/PrototypePollutingAssignment.qhelp
Outdated
Show resolved
Hide resolved
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
|
Thanks for the review @mchammer01, I've addressed the comments. |
mchammer01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments @asgerf ⚡ ✨
This is good to go from a docs point of view 👍🏻
@erik-krogh any final comments from your end? |
erik-krogh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erik-krogh any final comments from your end?
Nope. LGTM.
Adds
js/prototype-polluting-assignmentfor flagging property assignments of the formobj[x].prop = valuewherexmay be the user-controlled value__proto__.Since the property read
obj[x]and the assignmentbase.prop = valuecan occur arbitrarily far away from each other, we use a flow label to track the value fromobj[x]tobase.Currently the new query does not do anything special to account for the
constructor.prototypepayload, as such sinks are often exploitable by__proto__as well (the notable exception being when people explicitly guard against__proto__but notconstructor/prototype).Also reorganizes the prototype pollution queries, moving all to the Security/CWE-915 folder and tagging them with CWE-079, CWE-094, CWE-400, and CWE-915.
Also, the query
js/type-confusion-through-parameter-tamperingnow treats thexinx === "__proto__"as a sink, since it can be bypassed by["__proto__"], slightly simplifying the sanitizer logic in the new query.Evaluations: (internal links)