Skip to content

Conversation

SvanBoxel
Copy link

@SvanBoxel SvanBoxel commented Jun 15, 2020

Disclaimer: 👶 This is my first CodeQL query, so I expect there are still many things to improve here.

This Pull Request introduces a CodeQL query that looks for user-controlled delays in schedule methods in Javascript which can lead to memory leaks. The reason for this is a recent customer engagement I had. A team encountered exactly this issue which caused a bit of downtime in the end.

It's using the dataFlow library and explicitly looks for setTimeout and setInterval calls.

In the TaintedScheduleMethod.qhelp I describe that capping the user-controlled delay is a way to prevent this from happening, but I haven't implemented a check in the query that checks whether the user provided value is capped. This is something I can use your help with.

@SvanBoxel SvanBoxel marked this pull request as ready for review June 15, 2020 10:49
@SvanBoxel SvanBoxel requested a review from a team as a code owner June 15, 2020 10:49
@esbena
Copy link
Contributor

esbena commented Jun 16, 2020

Disclaimer: baby This is my first CodeQL query, so I expect there are still many things to improve here.

Not really. You have written pretty idiomatic QL.
The only missing piece is use of the sanitizers (as you say yourself), and the use of the customizable Configuration design pattern that we would like for Configuration queries that are merged into non-experimental directory.

I happen to have written a similar query last week here: #3702. I have added your sinks to that query, and would like to merge that instead of continuing with this PR.

For reference, my query also uses Buffer.alloc(sink), Array(sink).fill(x), x.repeat(sink) as sinks.

For reference, you can see here how the sanitizers for your original query would look:

    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
      guard instanceof LoopBoundInjection::LengthCheckSanitizerGuard or
      guard instanceof UpperBoundsCheckSanitizerGuard or
      guard instanceof TypeTestGuard
    }

@asgerf
Copy link
Contributor

asgerf commented Jun 16, 2020

@SvanBoxel I just want to say that we really appreciate feedback from the field, and I hope you'll more of these in the future.

As @esbena mentioned, this time we happened to have another query in flight that was similar enough to this one that it makes more sense to merge the two queries. I hope you don't take this as a form of rejection -- two weeks ago we'd have loved to help finish this PR, the timing was just a bit unfortunate for a first contribution.

@SvanBoxel
Copy link
Author

That seems more than reasonable @esbena @asgerf. 👍 Thanks for looking into this and providing some context on the process. Also subscribing to #3702 to educate myself a bit.

I'll keep you informed on what is happening in the field!

@SvanBoxel SvanBoxel closed this Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants