C#: Refactor C# queries to use ThreatModelFlowSource instead of RemoteFlowSource

[egregius313]

Follow up to #15359.

Refactors C# queries to use ThreatModelFlowSource instead of RemoteFlowSource.

Since most of the C# queries define their sources with public classes like

class RemoteSource extends Source instanceof RemoteFlowSource {}

I cannot remove the classes without changing the public API. Therefore, I have marked the existing RemoteSource classes as deprecated, and added a new implementation based on ThreatModelFlowSource. Since threat modeling includes remote sources by default, I think this is a reasonable trade-off.

[egregius313]

DCA seems fine.

[michaelnebel]

It is worth mentioning the specific queries that also included local sources as default no longer does so under the default threat model configuration (e.g cs/code-injection). It is also worth bringing this up in the C# slack channel to make sure that product is informed about this change. IMO this should be perfectly fine as we don't have that many local sources defined in C# as it is - but we need to make sure that this is properly communicated both to product and via the change note. Also, it would be great to streamline this with java and furthermore it seems rather ad hoc that some queries use them while others don't. With the threat models - this will be streamlined.

[michaelnebel]

This is starting to look good @egregius313 !

Some actionable feedback that we need to consider as a part of this PR:

• XMLInjection.ql, ExternalAPIsQuery.qll needs to be re-factored to use threat models as well.
• In InsecureDirectObjectReferenceQuery.qll we need to replace RemoteFlowSource with ThreatModelFlowSource in hasIdParameter.
• In SqlInjection.ql, UncontrolledFormatString.ql the getSourceType predicate needs to be updated as well <— This probably requires that we lift the abstract getSourceType predicate to the SourceNode class instead (which will require that some further source node descriptions are provided).
• Import alerts need to be cleaned up.

[michaelnebel]

There is also an open question

• What should happen to all the “Stored” variants of queries? Should we go ahead and delete those (this should be a separate PR) and explain how the normal variants of the queries can be (threat model) configured to also capture the “stored” results?

[egregius313]

@michaelnebel thanks for the review!

This probably requires that we lift the abstract getSourceType predicate to the SourceNode class instead (which will require that some further source node descriptions are provided).

I have added a commit d48235d which moves getSourceType to SourceNode.

Should we go ahead and delete those (this should be a separate PR) and explain how the normal variants of the queries can be (threat model) configured to also capture the “stored” results?

I'm not sure about the deletions (I would like product's opinion on that before going through with deleting a query), but something about how to opt into the stored sources might be helpful.

As I understand it, one of the things which makes things trickier in C# than Java is that the way the threat model hierarchy is setup by default, the classes which are stored (file and database) are counted under local.

But that means that in the long run the stored queries become largely redundant. Especially if we transition to StoredFlowSources also providing getSourceType.