C#: update MaD for HttpRequestMessage and UriBuilder

[LWSimpkins]

Update MaD for C# related to SSRF and URL path traversal scenarios. Some of these are regressions from 2.19.4 to 2.20.0 upgrade, some were missing models.

HttpRequestMessage

• Change model so constructor for Uri parameter matches string parameter, where the taint is to the object instead of an internal synthetic field (regression)

• Example:

  HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Post, new Uri(untrustedUrl)); // Fixed regression. Now the `request` variable is considered tainted again
  (new HttpClient()).SendAsync(request); // SSRF can be flagged
  

UriBuilder

• Change model for constructor so it flows to synthetic _uri field, which is used in the get_Uri model, so there is taint flow from the constructor to get_Uri

• Add missing variants of the constructor

  • https://learn.microsoft.com/en-us/dotnet/api/system.uribuilder.-ctor?view=net-9.0

• Add get/set property methods to support dataflow for MemberInitializer in ObjectInitializer

• Examples:

  (new HttpClient()).GetAsync(new UriBuilder("https", untrustedHost).Uri); // Fixed regression. `untrustedHost` flows to `.Uri`, SSRF can be flagged
  
  (new HttpClient()).GetAsync(new UriBuilder("https", untrustedHost, 443).Uri); // Added constructor variant that was previously missing. `untrustedHost` flows to `.Uri`, SSRF can be flagged
  
  // Added get/set property methods to support MemberInitializer in ObjectInitializer
    var uriBuilder = new UriBuilder
    {
        Scheme = "https",
        Host = untrustedHost,
        Port = 443,
    };
    (new HttpClient()).GetAsync(uriBuilder.Uri); // `untrustedHost` flows to `.Uri`, SSRF can be flagged
  

PR Overview

This pull request updates taint flow modeling for HttpRequestMessage and UriBuilder in C#, ensuring that constructed or updated URIs are appropriately recognized as tainted for SSRF and path traversal checks.

• Fixes regression for HttpRequestMessage constructors taking a Uri parameter, aligning them with taint flow from string-based constructors
• Adds taint flow to newly modeled UriBuilder constructors and properties

Changes

File	Description
csharp/ql/lib/ext/System.model.yml	Adds UriBuilder taint flow for multiple constructors, properties, & ToString
csharp/ql/lib/change-notes/2025-02-05-update-system.net.http.httprequestmessage-and-system.uribuilder-models.md	Documents the minor analysis changes for HttpRequestMessage & UriBuilder
csharp/ql/lib/ext/System.Net.Http.model.yml	Fixes taint flow regression for HttpRequestMessage constructor with Uri

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[github-actions]

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

csharp

Generated file changes for csharp

• Changes to framework-coverage-csharp.rst:

-    System,"``System.*``, ``System``",47,10864,54,5
+    System,"``System.*``, ``System``",47,10896,54,5
-    Totals,,108,12946,400,9
+    Totals,,108,12978,400,9

• Changes to framework-coverage-csharp.csv:

- System,54,47,10864,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5547,5317
+ System,54,47,10896,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5579,5317

[LWSimpkins]

DCA has been run for this PR (if we got the settings correct; not sure what you typically use). There were no changes to results.

[michaelnebel]

It looks like the regression happened after re-generating the models: #17666
The model generator mixes the heuristic and content based models for the URI builder in an unfortunate way for this particular usecase.

[michaelnebel]

If we want to model the URIBuilder class manually, I think we should go for a solution that uses the (1) properties of the builder or (2) a less precise solution which taints the entire object (as it was the case before with the generated models). Personally, I would favor (1) as we might risk false positives due to field conflation.
A couple of examples (based on the implementation found here - these are the models for one of the constructors - as this one only taints two specific properties).

- ["System", "UriBuilder", False, "UriBuilder", "(System.String,System.String)", "", "Argument[0]", "Argument[this].Property[System.UriBuilder.Scheme]", "taint", "manual"]
- ["System", "UriBuilder", False, "UriBuilder", "(System.String,System.String)", "", "Argument[1]", "Argument[this].Property[System.UriBuilder.Host]", "taint", "manual"]

[LWSimpkins]

@michaelnebel UriBuilder is complicated in terms of modeling, since multiple properties are tainted by the constructor parameters.

Taking the constructor with the most parameters as an example: public UriBuilder(string? scheme, string? host, int port, string? path, string? extraValue)

Parameter	Property
scheme	Scheme, also taints Uri
host	Host, also taints Uri
port	Port, also taints Uri
path	Path, also taints Uri
extraValue	Query and/or Fragment, also taints Uri

When I tried the suggested change to have the sink use the appropriate parameters and also tried both of the following for .Uri, there was no flow because the Uri property and UriBuilder object were not tainted from the constructor parameters.

- ["System", "UriBuilder", False, "get_Uri", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
- ["System", "UriBuilder", False, "get_Uri", "()", "", "Argument[this].Property[System.UriBuilder.Uri", "ReturnValue", "taint", "manual"]

Does MaD support modeling taint to multiple properties/sinks? If not, should I use Argument[this] for everything instead?

[michaelnebel]

@michaelnebel UriBuilder is complicated in terms of modeling, since multiple properties are tainted by the constructor parameters.

Taking the constructor with the most parameters as an example: public UriBuilder(string? scheme, string? host, int port, string? path, string? extraValue)

Parameter Property
scheme Scheme, also taints Uri
host Host, also taints Uri
port Port, also taints Uri
path Path, also taints Uri
extraValue Query and/or Fragment, also taints Uri
When I tried the suggested change to have the sink use the appropriate parameters and also tried both of the following for .Uri, there was no flow because the Uri property and UriBuilder object were not tainted from the constructor parameters.

- ["System", "UriBuilder", False, "get_Uri", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"] - ["System", "UriBuilder", False, "get_Uri", "()", "", "Argument[this].Property[System.UriBuilder.Uri", "ReturnValue", "taint", "manual"]

Does MaD support modeling taint to multiple properties/sinks? If not, should I use Argument[this] for everything instead?

Yes, it does get a bit involved to do the modelling - and you are right the models referring to the Uri property will get a bit more complicated. The Uri property is basically a taint propagator for all the other properties.
That is, (some of) the models for Uri should look something like (one will be required for each other property).

- ["System", "UriBuilder", False, "get_Uri", "()", "", "Argument[this].Property[System.UriBuilder.Scheme]", "ReturnValue", "taint", "manual"]
- ["System", "UriBuilder", False, "get_Uri", "()", "", "Argument[this].Property[System.UriBuilder.Host]", "ReturnValue", "taint", "manual"]

The same pattern is then needed for the constructors that "taints" all properties, eg

- ["System", "UriBuilder", False, "UriBuilder", "(System.Uri)", "", "Argument[0]", "Argument[this].Property[System.UriBuilder.Scheme]", "taint", "manual"]
- ["System", "UriBuilder", False, "UriBuilder", "(System.Uri)", "", "Argument[0]", "Argument[this].Property[System.UriBuilder.Host]", "taint", "manual"]
- ... <For all other properties>

The (other) simpler approach is simply to say that the entire object is tainted if any property is set. However, this carries the risk of false positives. Since, we didn't have a single example in our DCA codebase (and actually in our entire 5k repo suite, where we noticed this regressions), it is probably acceptable to go with the simpler solution (which will require less modelling).

[hvitved]

I think shorthand notation like this will work as well

- ["System", "UriBuilder", False, "get_Uri", "()", "", "Argument[this].Property[System.UriBuilder.Scheme,System.UriBuilder.Host]", "ReturnValue", "taint", "manual"]
- ["System", "UriBuilder", False, "UriBuilder", "(System.Uri)", "", "Argument[0]", "Argument[this].Property[System.UriBuilder.Scheme,System.UriBuilder.Host]", "taint", "manual"]

[LWSimpkins]

@michaelnebel Excellent, did not realize multiple properties could be modeled like that! Updated PR using the shorthand notation (it worked for me @hvitved). Thank you!

We may open-source parts of the Microsoft SSRF queries in the future, so I went ahead with the more detailed modeling.