Skip to content

JS: recognize more library input#5826

Merged
codeql-ci merged 5 commits intogithub:mainfrom
erik-krogh:moreLib
May 5, 2021
Merged

JS: recognize more library input#5826
codeql-ci merged 5 commits intogithub:mainfrom
erik-krogh:moreLib

Conversation

@erik-krogh
Copy link
Copy Markdown
Contributor

@erik-krogh erik-krogh commented May 4, 2021

Recognizes a library-input source for CVE-2017-0931 and CVE-2017-1001004.

Evaluation looks good.
No change in performance, and a few new poly-redos TPs.

@github-actions github-actions Bot added the JS label May 4, 2021
@erik-krogh erik-krogh added Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis no-change-note-required This PR does not need a change note and removed Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels May 4, 2021
@erik-krogh erik-krogh marked this pull request as ready for review May 4, 2021 14:09
@erik-krogh erik-krogh requested a review from a team as a code owner May 4, 2021 14:09
Copy link
Copy Markdown
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think https://github.com/github/codeql/pull/5826/files#diff-f8a3dfa03038e23405f8fc8c6002982fdf9f4c38a65c10ed68a12e8005a5db7cR21 has become a bit outdated now Maybe the line should just be deleted.

Seeing how big and heuristic the disjunction in getAValueExportedByPackage has become, I would like to see source code examples for each disjunct. If not in this PR, then a follow-up PR.

// ....
// }));
// ```
exists(ImmediatelyInvokedFunctionExpr func, DataFlow::ParameterNode prev, int i |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be related to the define.amd check? I think name-matching on "factory" is a bit too heuristic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've related it to the define(..) call.

I also missed a check that the file was actually exported 😬
That is added now.

@codeql-ci codeql-ci merged commit 69cd9df into github:main May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis JS no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants