Skip to content

JS: add query js/memory-exhaustion #3702

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jun 19, 2020
Merged

JS: add query js/memory-exhaustion #3702

merged 12 commits into from
Jun 19, 2020

Conversation

esbena
Copy link
Contributor

@esbena esbena commented Jun 12, 2020

CVE-2017-16026: FN - due to a source that can not be recognized 🙄

The query is primarily an implementation of an interprocedural isNumeric type analysis seeded by user inputs.

The query appears to have a scalability issue, but I haven't dug into it yet.

@esbena esbena added the JS label Jun 12, 2020
@esbena esbena requested review from mchammer01 and a team as code owners June 12, 2020 10:27
@esbena esbena changed the base branch from master to js-team-sprint June 12, 2020 10:27
erik-krogh
erik-krogh reviewed Jun 12, 2020
View reviewed changes
@esbena esbena force-pushed the js/memory-exhaustion branch from d727ea8 to cf118ea Compare June 12, 2020 12:48
erik-krogh
erik-krogh previously approved these changes Jun 12, 2020
View reviewed changes
@esbena esbena marked this pull request as draft June 15, 2020 12:57
@esbena esbena changed the base branch from js-team-sprint to master June 16, 2020 08:14
@esbena esbena dismissed erik-krogh’s stale review June 16, 2020 08:14

The base branch was changed.

@esbena esbena force-pushed the js/memory-exhaustion branch 2 times, most recently from 77f6cef to 2298892 Compare June 16, 2020 08:30
@esbena esbena marked this pull request as ready for review June 16, 2020 08:32
@esbena
Copy link
Contributor Author

esbena commented Jun 16, 2020

Changes since last time:

  • the Configuration pattern is now used
  • documentation
  • additional sinks: setTimeout/setInterval
  • flow steps that do not rely on the type inference (could possibly be made to work, but it doesn't seem worth it).
  • retargeted master

Missing:

  • evaluation

@erik-krogh
Copy link
Contributor

What about Underscore _.delay() / _.throttle() / _.debounce()?

@esbena esbena mentioned this pull request Jun 16, 2020
Closed
asgerf
asgerf reviewed Jun 16, 2020
View reviewed changes
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

LGTM with the qhelp fixed and either an evaluation or rebase onto the sprint branch.

@esbena esbena added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Jun 16, 2020
@esbena
Copy link
Contributor Author

esbena commented Jun 18, 2020

Small evaluation: https://git.semmle.com/esben/dist-compare-reports/tree/js/memory-exhaustion_1592466827664. And rebased onto the sprint branch.

@esbena esbena changed the base branch from master to js-team-sprint June 18, 2020 10:59
@esbena esbena requested review from jf205, shati-patel and a team as code owners June 18, 2020 10:59
@esbena
Copy link
Contributor Author

esbena commented Jun 18, 2020

:oh no: Rebasing again.

@esbena esbena force-pushed the js/memory-exhaustion branch from b28fe2c to ab01dda Compare June 18, 2020 11:01
@esbena esbena removed request for a team, mchammer01, jf205 and shati-patel June 18, 2020 11:01
asgerf
asgerf approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

LGTM

That was an interesting result in the evaluation.

The failing QLDoc check can be ignored.

@asgerf
Copy link
Contributor

asgerf commented Jun 18, 2020

Oh wait, I guess we're still missing doc review from @mchammer01

@esbena
Copy link
Contributor Author

esbena commented Jun 18, 2020 •
edited
Loading

((oops wrong context for this message) double doh)

That was an interesting result in the evaluation.

Yep. I have mentioned it in https://github.com/github/codeql-javascript-team/issues/186

@mchammer01
Copy link
Contributor

Added this to my TODO list 😉

mchammer01
mchammer01 requested changes Jun 19, 2020
View reviewed changes
Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

@esbena - finally reviewed this PR, it's looking good ✨
I made some suggestions on improvements.
Hope this helps!

javascript/ql/src/Security/CWE-770/ResourceExhaustion.ql
@@ -0,0 +1,20 @@
/**
* @name Resource exhaustion
* @description Allocating objects or timers with user-controlled
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be Allocating.... to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. "Allocating an object" generally means that it is created, with the implicit meaning that you have allocated some memory for it. It does look a bit weird now that you ask about it, but that is how it is. You can see som background information on wikipedia.

esbena and others added 5 commits June 19, 2020 09:46
@esbena @mchammer01
Update javascript/ql/src/Security/CWE-770/ResourceExhaustion.qhelp
4557af3 
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@esbena @mchammer01
Update javascript/ql/src/Security/CWE-770/ResourceExhaustion.qhelp
2846666 
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@esbena @mchammer01
Update javascript/ql/src/Security/CWE-770/ResourceExhaustion.qhelp
e73becc 
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@esbena @mchammer01
Update javascript/ql/src/Security/CWE-770/ResourceExhaustion.qhelp
b8229ca 
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@esbena @mchammer01
Update javascript/ql/src/Security/CWE-770/ResourceExhaustion.qhelp
0463c42 
Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@esbena
Copy link
Contributor Author

esbena commented Jun 19, 2020

Thanks @mchammer01.

@asgerf asgerf merged commit eca5e2d into github:js-team-sprint Jun 19, 2020
esbena added a commit to esbena/codeql that referenced this pull request Jun 22, 2020
@esbena
Revert "Merge pull request github#3702 from esbena/js/memory-exhaustion"
9a0bbb3 
This reverts commit eca5e2d, reversing
changes made to 1548eca.
This was referenced Jun 22, 2020
Closed
Merged
erik-krogh added a commit to erik-krogh/ql that referenced this pull request Sep 9, 2020
@erik-krogh
Revert "Revert "Merge pull request github#3702 from esbena/js/memory-…
7e36db5 
…exhaustion""

This reverts commit 9a0bbb3.
@esbena esbena mentioned this pull request Jan 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchammer01 mchammer01 mchammer01 requested changes

@asgerf asgerf asgerf approved these changes

@erik-krogh erik-krogh erik-krogh left review comments

Assignees
No one assigned
Labels
Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish JS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@esbena @erik-krogh @asgerf @mchammer01