Rust: Add support for MaD sources and sinks with access paths

[hvitved]

Overview

This PR adds support for defining flow sources/sinks for Rust with non-trivial access paths. For example, one may define

extensions:
  - addsTo:
      pack: codeql/rust-all
      extensible: sourceModel
    data:
      - ["repo::test", "crate::my_option_source", "ReturnValue.Variant[crate::option::Option::Some(0)]", "test-source", "manual"]

which models calls to crate::my_option_source as flow sources of kind test-source, but where the origin of tainted data is not directly the result of the call, but rather stored inside Option::Some in the result of the call.

Similarly, one may define a sink restricted to data stored inside Option::Some:

extensions:
  - addsTo:
      pack: codeql/rust-all
      extensible: sinkModel
    data:
      - ["repo::test", "crate::my_option_sink", "Argument[0].Variant[crate::option::Option::Some(0)]", "test-sink", "manual"]

Implementation

The implementation piggy-backs on the existing FlowSummaryImpl library, which already has functionality for specifying flow summaries with non-trivial input/output access paths. For a flow source like my_option_source above, we synthesize a store step from synthetic source nodes to actual calls to my_option_source, and dually for sinks like my_option_sink, we synthesize read steps from arguments to synthetic sink nodes.

The functionality is currently limited to Rust, but other languages should be able to adopt relatively easily.

[geoffw0]

I'm looking forward to this PR being merged. I want to use this functionality in several places already, the tests look like exactly what I need, and I'm experimenting with this branch locally.

I don't feel very confident reviewing the code changes in this PR though - if there are any specific parts you'd like me to look at or discuss please say so.

[aibaars]

What does the repo:: prefix mean, and why do we need the crate:: prefix?

[hvitved]

The repo::test bit matches Resolvable.getResolvedCrateOrigin and crate::enum_source matches Resolvable.getResolvedPath, so this is simply what we get from the extractor. As for the repo prefix, I'm not sure, but for the crate prefix this appears to be always present in canonical paths.

[aibaars]

How would this work if there are items from multiple crates involved?

[hvitved]

I would expect those to have different getResolvedCrateOrigin values.

[aibaars]

I would expect those to have different getResolvedCrateOrigin values.

That's right. I was wondering about the case where a function from one crate has arguments or return values of types in other crates. How would the lines above look if MyFieldEnum was defined in a different crate than enum_source ?

[hvitved]

Ah, right. I decided to not include the crate origins in the Variant arguments, since that would bloat things. But you are right that this can potentially give rise to some ambiguity, if two crates define variants with the same canonical paths.

[hvitved]

@aschackmull : Are you able to review the shared bits in FlowSummaryImpl.qll?

[aschackmull]

I think something needs to change here. We worked very hard to untangle and stratify DataFlow::Node vs. SummaryNode such that FlowSummaryImpl wasn't dependent on DataFlow::Node. But now they've been made mutually recursive. We need to fix that, i.e. we should not embed DataFlow::Nodes into SummaryNodes.

[hvitved]

SinkNode is not a subset of DataFlow::Node, rather it is a subset of the parameterized type SinkBase, which for Rust is subset of AstNode.

[aschackmull]

Ah! Right.
So I guess the only new connection to DataFlow::Node is in sourceLocalStep via StepsInput::getSourceNode?

[aschackmull]

Maybe SourceNode and SinkNode ought to be called something different, because it's perhaps somewhat unclear what kind of "nodes" they are supposed to be - at least I got confused. They're not DataFlow::Nodes and they are not SummaryNodes. Maybe simply use SourceElement/SinkElement instead? And likely elaborate related qldoc a bit.

[aschackmull]

Shared code LGTM.

[geoffw0]

🎉