2n part of ICryptoTransform.

[raulgarciamsft]

Detecting potential unsafe usage (object shared across multiple threads) on variables captured by Lambda

[calumgrant]

Hi Raul, Many thanks for your contribution, we really appreciate it. I've made a few minor suggestions, but my main request is whether we could also extend this query to cover other ways of creating threads, in particular, Parallel.Invoke. Thanks again for your work.

[raulgarciamsft]

Hi Raul, Many thanks for your contribution, we really appreciate it. I've made a few minor suggestions, but my main request is whether we could also extend this query to cover other ways of creating threads, in particular, Parallel.Invoke. Thanks again for your work.

Thanks a lot. I covered Parallel.Invoke, and I think it would be easy to add more ways to create threads. Let me know what do you think of the approach I took to add coverage for Parallel.Invoke, and I can use the same for other thread-creation mechanisms.

[raulgarciamsft]

Please also let me know what other mechanisms would you like me to add, I would like to focus on the most commonly used ones first. Thanks

[calumgrant]

Good to have this extra coverage of Parallel.Invoke. I can't think of any additional sinks - well there's Parallel.ForEach but it's really a case of combing the API. What I think this shows is that we need another abstraction - something like

abstract class ParallelSink extends DataFlow::Node {
}

class LambdaParallelSink extends ParallelSink {
  LambdaParallelSink() {
    exists( Class c, Method m, MethodCall mc, Expr e | 
        e = this.asExpr() |
        c.getABaseType*().hasQualifiedName("System.Threading.Tasks", "Parallel")
        and c.getAMethod() = m
        and m.getName() = "Invoke"
        and m.getACall() = mc
        and mc.getAnArgument() = e
      )
  }
}

and so on for the other cases. Then they can be combined into one configuration, thus

class NotThreadSafeCryptoUsageConfig extends TaintTracking::Configuration  {
  NotThreadSafeCryptoUsageIntoParallelInvokeConfig() { this = "NotThreadSafeCryptoUsageConfig" }

   override predicate isSource(DataFlow::Node source) {    
    ///
  }

   override predicate isSink(DataFlow::Node sink) {
    sink instanceof ParallelSink
  }
}

This is exactly the same logic that you had before, but just removing the duplicate parts and abstracting them into ParallelSink.

The big idea is that we'll probably want to add more parallel sinks in future, and even move the ParallelSink class into its own QLL file so that it can be modularized and perhaps used in other queries as well. The abstract class mechanism supports the Open-closed principle, so we can add additional cases without modifying or cloning too much existing code.

For now, we can keep them in the query file until we need to reuse them.

Does this make sense?

[jf205]

Hi @raulgarciamsft.
I have two very minor comments about the query metadata that would be good to fix.
The qhelp documentation LGTM.
Thanks.

[raulgarciamsft]

I added 2 QLL libraries, one for ParallelSink, which may be useful in other scenarios, and one for all the ICryptoTransform related classes & predicates as suggested by Calum (I think it makes sense adding them right now).
Please let em know if you would like me to rename or/and change the location for them.
Thanks

[jf205]

Thanks for fixing the typos 👍

[calumgrant]

Awesome. One last thing. I'm not sure we want to tailor the error message for each different sink type - ultimately the lambda is run concurrently using a variety of ways to kick off a thread. If this is the case, then perhaps we could merge the two configurations into one, and then the isSink predicate would look like

override predicate isSink(DataFlow::Node sink) {
    sink instanceof ParallelSink
    or
    exists( DelegateCreation dc, Expr e | 
      e = sink.asExpr() |
      dc.getArgument() = e
      and dc.getType().getName().matches("%Start")
    )
  }

Then we can make the further refactoring by moving exists( DelegateCreation dc, Expr e | e = this.asExpr() | dc.getArgument() = e and dc.getType().getName().matches("%Start") ) into ParallelSink.qll as an additional class:

class ThreadStartParallelSink extends ParallelSink {
  ThreadStartParallelSink() {
    exists( DelegateCreation dc, Expr e | 
      e = sink.asExpr() |
      dc.getArgument() = e
      and dc.getType().getName().matches("%Start")
    )
  }
}

If you really wanted to tailor the error message, then it could be a predicate on class ParallelSink.

The reason for this comment, is that we don't normally use several data flow configurations in one query.

Another quite minor thing, is that you could consider turning this query into a "path query" - but this is completely optional.

[calumgrant]

Thanks, Raul. LGTM, merging.