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

egregius313 · 2024-01-24T03:30:10Z

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.

Folllow-up PRs

egregius313 · 2024-01-27T01:55:23Z

DCA seems fine.

michaelnebel · 2024-01-29T08:54:23Z

csharp/ql/src/change-notes/2024-01-23-threat-models.md

+---
+category: minorAnalysis
+---
+* Most data flow queries that track flow from *remote* flow sources now use the current *threat model* configuration instead. This doesn't lead to any changes in the produced alerts (as the default configuration is *remote* flow sources) unless the threat model configuration is changed.


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 · 2024-01-29T09:31:35Z

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 · 2024-01-31T03:49:40Z

@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.

michaelnebel · 2024-01-31T10:58:50Z

@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.

Great, I will take a look!

Yes, you are absolutely right - the "open" question needs to be aligned with product. We need to ask, what they think, before deleting any of those. I would recommend asking the the C# slack channel.

If we only want to use stored flow sources for a query execution it is still possible using threat models, as threat models supports priorities (it is possible to enable a category of threat models, but disable certain sub categories).

csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Stored.qll

csharp/ql/src/change-notes/2024-01-23-threat-models.md

csharp/ql/lib/semmle/code/csharp/security/dataflow/ExternalAPIsQuery.qll

michaelnebel

The failing test can be fixed by adding data extension file that enables local threat models for that test.
We probably need to run DCA again after fixing the last minor changes and then get a second review as well (in case I missed something).
Furthermore, we need to have "approval" from product on the change of semantics (removing the local sources as the default for a set of queries).

michaelnebel

LGTM!
Let's wait with merging until the discussion with product has finished.

egregius313 · 2024-02-09T01:40:37Z

Let's wait until the discussion with product has finished

After discussion with product, this is currently on hold until the release model for the threat model release.

csharp/ql/lib/semmle/code/csharp/security/dataflow/CodeInjectionQuery.qll

michaelnebel · 2024-02-29T08:10:40Z

csharp/ql/src/Security Features/CWE-114/AssemblyPathInjection.ql

@@ -22,7 +22,7 @@ import AssemblyPathInjection::PathGraph
 */
 module AssemblyPathInjectionConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) {
-    source instanceof RemoteFlowSource or
+    source instanceof ThreatModelFlowSource or
    source.asExpr() = any(MainMethod main).getParameter(0).getAnAccess()


This is basically a local source (we shouldn't do anything in this PR), but if main parameters are modelled generally and being "configurable" via threat models then we should consider to remove the hardcoding of main parameters here.

csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql

csharp/ql/src/change-notes/2024-02-06-threat-models.md

michaelnebel

Just a few more minor changes and then run DCA again :-)

…ource` instead

…odelFlowSource`

Since `FlowSources` now re-exports `Remote`, these can be safely removed.

…irectly. Since `ThreatModelFlowSource` contains `RemoteFlowSource` by default, we can safely remove the `RemoteSource` from the default of the queries.

1. Change `LocalSource` to extend `DataFlow::Node`, thus removed from the definiton of `Source` 2. Add a private class `AddLocalSource` which extends `Source`. This allows us to currently preserve the inclusion of local sources, while making it easier to remove it in the future.

Co-authored-by: Michael Nebel <michaelnebel@github.com>

michaelnebel

Thank you!
I will start a DCA run and if that is uneventful then we merge.

michaelnebel · 2024-03-01T13:08:00Z

DCA looks good.

github-actions bot added C# documentation labels Jan 24, 2024

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from 30fd992 to d347967 Compare January 25, 2024 16:39

egregius313 marked this pull request as ready for review January 27, 2024 01:54

egregius313 requested a review from a team as a code owner January 27, 2024 01:54

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from d347967 to 1789d26 Compare January 27, 2024 02:13

michaelnebel self-requested a review January 29, 2024 08:26

michaelnebel reviewed Jan 29, 2024

View reviewed changes

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from 1789d26 to 6dd50b3 Compare January 31, 2024 03:33

michaelnebel self-requested a review January 31, 2024 10:51

michaelnebel reviewed Jan 31, 2024

View reviewed changes

csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Stored.qll Outdated Show resolved Hide resolved

michaelnebel reviewed Jan 31, 2024

View reviewed changes

csharp/ql/src/change-notes/2024-01-23-threat-models.md Outdated Show resolved Hide resolved

michaelnebel reviewed Jan 31, 2024

View reviewed changes

csharp/ql/lib/semmle/code/csharp/security/dataflow/ExternalAPIsQuery.qll Show resolved Hide resolved

michaelnebel reviewed Jan 31, 2024

View reviewed changes

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from 3e358f3 to 6abc86a Compare February 2, 2024 16:38

michaelnebel previously approved these changes Feb 5, 2024

View reviewed changes

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from 771bfba to 11d43c0 Compare February 15, 2024 22:01

egregius313 mentioned this pull request Feb 15, 2024

C#: Remove Stored variants of queries #15629

Merged

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from 11d43c0 to 20daa89 Compare February 26, 2024 14:58

michaelnebel reviewed Feb 28, 2024

View reviewed changes

csharp/ql/lib/semmle/code/csharp/security/dataflow/CodeInjectionQuery.qll Outdated Show resolved Hide resolved

michaelnebel reviewed Feb 28, 2024

View reviewed changes

csharp/ql/lib/semmle/code/csharp/security/dataflow/CodeInjectionQuery.qll Outdated Show resolved Hide resolved

egregius313 dismissed michaelnebel’s stale review via dbad866 February 28, 2024 17:45

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from 20daa89 to dbad866 Compare February 28, 2024 17:45

egregius313 mentioned this pull request Feb 28, 2024

C#: Remove AddLocalSource classes from queries #15756

Merged

michaelnebel reviewed Feb 29, 2024

View reviewed changes

csharp/ql/src/Security Features/CWE-134/UncontrolledFormatString.ql Outdated Show resolved Hide resolved

michaelnebel reviewed Feb 29, 2024

View reviewed changes

csharp/ql/src/change-notes/2024-02-06-threat-models.md Outdated Show resolved Hide resolved

michaelnebel reviewed Feb 29, 2024

View reviewed changes

egregius313 and others added 19 commits February 29, 2024 12:02

Deprecated direct use of RemoteFlowSource and use `ThreatModelFlowS…

bd0137a

…ource` instead

Deprecate direct uses of RemoteFlowSource and replace with `ThreatM…

f388a0f

…odelFlowSource`

Change note

7577266

Move getSourceType to SourceNode

31d4d0c

Refactor to using SourceNode::getSourceType

fd3738b

Refactor to using ThreatModelFlowSource

b76795f

Refactor to using ThreatModelFlowSource

1086abc

Refactor to using ThreatModelFlowSource

434fa20

Remove unnecessary flowsources.Remote imports

b4dabee

Since `FlowSources` now re-exports `Remote`, these can be safely removed.

Change note changes to mention local sources as well

eb81946

Update change note date

22e8da2

Add references to changed queries in change note

5198f52

Refactor to use ThreatModelFlowSource

f237d2a

Remove commented out code

af749e2

Add threat-model configuration for UncontrolledFormatString test

e64826d

Modify deprecated RemoteSource classes to extend DataFlow::Node d…

3563c7e

…irectly. Since `ThreatModelFlowSource` contains `RemoteFlowSource` by default, we can safely remove the `RemoteSource` from the default of the queries.

Remove discussion of local flow in change note

89bdb7f

Co-authored-by: Michael Nebel <michaelnebel@github.com>

Add LocalFlowSource back to UncontrolledFormatString

f488f23

egregius313 force-pushed the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch from 0e16ebb to f488f23 Compare February 29, 2024 17:07

michaelnebel self-requested a review March 1, 2024 09:52

michaelnebel approved these changes Mar 1, 2024

View reviewed changes

michaelnebel merged commit a97510a into main Mar 1, 2024

michaelnebel deleted the egregius313/csharp/dataflow/threat-modeling/refactor-to-threatmodelflowsource branch March 1, 2024 13:40

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

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

Uh oh!

Conversation

egregius313 commented Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Folllow-up PRs

Uh oh!

egregius313 commented Jan 27, 2024

Uh oh!

michaelnebel Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Jan 29, 2024

Uh oh!

egregius313 commented Jan 31, 2024

Uh oh!

michaelnebel commented Jan 31, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

egregius313 commented Feb 9, 2024

Uh oh!

Uh oh!

Uh oh!

michaelnebel Feb 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

michaelnebel commented Mar 1, 2024

Uh oh!

Uh oh!

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

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

egregius313 commented Jan 24, 2024 •

edited

Loading

michaelnebel Jan 29, 2024 •

edited

Loading