codeql icon indicating copy to clipboard operation
codeql copied to clipboard

Improve data flow in the `async` package

Open Vasco-jofra opened this issue 7 months ago • 1 comments

This PR makes these changes:

  1. In FlowSummaryPrivate.qll: Added support for anyProperty content set in flow summaries:
    • Please confirm this is the correct way to support this. I needed this to find the async_.map({a: source()}, call_sink) case. Let me know if there's a better way to do it
    • As a note, I was also unable to use Element, which, according to the documentation, should select "an element of an array, iterator, or set object."
  2. In AsyncPackage.qll: Improve taint tracking through functions from the async package:
    • Improve tracking of the callback arguments
    • Implemented flow summaries for more robust taint tracking
    • Updated tests

Vasco-jofra avatar Jun 15 '25 16:06 Vasco-jofra

Also, let me know if there's a way to avoid having two DataFlow::SummarizedCallable when all I want to do is change the argument number. I found no way to get a reference to the call in the propagatesFlow predicate against which I could call getIteratorCallbackIndex.

Vasco-jofra avatar Jun 16 '25 08:06 Vasco-jofra

Also, let me know if there's a way to avoid having two DataFlow::SummarizedCallable when all I want to do is change the argument number. I found no way to get a reference to the call in the propagatesFlow predicate against which I could call getIteratorCallbackIndex.

You can merge the two classes by adding a field to the class, and embedding the value of the field in its this binding. See this example.

asgerf avatar Jun 24 '25 07:06 asgerf

As a note, I was also unable to use Element, which, according to the documentation, should select "an element of an array, iterator, or set object."

Thanks, I'll make sure to fix the discrepancy.

asgerf avatar Jun 24 '25 07:06 asgerf

I fixed what you pointed out in your comments and merged the DataFlow::SummarizedCallable classes into a single one as you suggested.

I don't have time to test now, but in a codebase I was running this on, there was an asyncHelper.each function that called async.each inside. This change was causing asyncHelper.each(array1, cb1); asyncHelper.each(array2, cb2), to incorrectly flow data from array1 to the first parameter of both cb1 and cb2. This caused several false positives. I haven't investigated why this happens, but we should fix it before merging.

Vasco-jofra avatar Jun 26 '25 08:06 Vasco-jofra