C#: Improve some existing manual models. #19940

michaelnebel · 2025-07-01T11:23:31Z

In this PR we improve the models as data modelling across multiple classes.

Explicitly added summary models for all overloads of System.Xml.XmlDictionaryReader.CreateBinaryReader.
Added models for some of the methods and properties in System.Runtime.Serialization.SerializationInfo and System.Runtime.Serialization.SerializationInfoEnumerator.
Updated models for System.Text.Encoding.GetBytes, System.Text.Encoding.GetChars and the constructor for System.IO.MemoryStream.

Also convert the deserialization tests to inline expectations.

The new findings in mono/mono for cs/cleartext-storage-of-sensitive-information and
cs/exposure-of-sensitive-information are both related to the improved models for GetBytes (where taint is propagated to another parameter instead of the return).

github-actions · 2025-07-01T13:38:14Z

⚠️ 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,12139,54,5
+    System,"``System.*``, ``System``",47,12165,54,5
-    Totals,,107,14403,407,9
+    Totals,,107,14429,407,9

Changes to framework-coverage-csharp.csv:

- System,54,47,12139,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5903,6236
+ System,54,47,12165,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5929,6236

Copilot

Pull Request Overview

This PR enhances security test queries by converting deserialization tests to inline alert annotations and improves the library’s manual summary models to reduce false negatives.

Converted multiple unsafe/unsafe-untrusted-deserialization tests to use inline // $ Alert[...] comments.
Renamed and expanded binary‐formatter test classes for broader coverage.
Added and refined summary models for XmlDictionaryReader.CreateBinaryReader, Encoding.GetBytes/GetChars, MemoryStream constructors, and serialization APIs.

Reviewed Changes

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

Show a summary per file

File	Description
csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInputNewtonsoftJson/Test.cs	Moved JSON unsafe-deserialization alert to inline comment.
csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInput/XmlSerializerUntrustedInputBad.cs	Added inline alert annotation after the return call.
csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInput/BinaryFormatterUntrustedInputBad.cs	Renamed `BadBinaryFormatter` class and added a second test class.
csharp/ql/src/change-notes/2025-07-01-improve-summary-models.md	Documented the new and updated summary models.
csharp/ql/lib/ext/System.Xml.model.yml	Added summary models for all `XmlDictionaryReader.CreateBinaryReader` overloads.
csharp/ql/lib/ext/System.Text.model.yml	Updated taint mappings for `Encoding.GetBytes`/`GetChars`.
csharp/ql/lib/ext/System.Runtime.Serialization.model.yml	Added models for `SerializationInfo` and `SerializationInfoEnumerator`.
csharp/ql/lib/ext/System.IO.model.yml	Adjusted `MemoryStream` taint model to use `.Element` for byte arrays.

Files not reviewed (7)

csharp/ql/test/library-tests/dataflow/library/FlowSummaries.expected: Language not supported
csharp/ql/test/library-tests/dataflow/library/FlowSummariesFiltered.expected: Language not supported
csharp/ql/test/query-tests/Security Features/CWE-502/DeserializedDelegate/DeserializedDelegate.qlref: Language not supported
csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserialization/UnsafeDeserialization.qlref: Language not supported
csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInput/UnsafeDeserializationUntrustedInput.expected: Language not supported
csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInput/UnsafeDeserializationUntrustedInput.qlref: Language not supported
csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInputNewtonsoftJson/UnsafeDeserializationUntrustedInput.qlref: Language not supported

Comments suppressed due to low confidence (2)

csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInputNewtonsoftJson/Test.cs:17

The $ Alert annotation is placed on the JsonSerializerSettings line rather than on the DeserializeObject invocation itself. Move the inline comment to the line starting with return JsonConvert.DeserializeObject(...) to ensure the test framework picks up the alert.

        return JsonConvert.DeserializeObject(data.Text, new JsonSerializerSettings // $ Alert[cs/unsafe-deserialization-untrusted-input]

csharp/ql/test/query-tests/Security Features/CWE-502/UnsafeDeserializationUntrustedInput/BinaryFormatterUntrustedInputBad.cs:7

The test harness expects a class named BadBinaryFormatter, but this has been renamed to BadBinaryFormatter1. Rename it back to BadBinaryFormatter to maintain compatibility with the test matcher.

class BadBinaryFormatter1

owen-mc

Just a few questions about how the models all work together.

owen-mc · 2025-07-15T11:59:25Z

csharp/ql/lib/ext/System.Runtime.Serialization.model.yml

+      - ["System.Runtime.Serialization", "SerializationInfo", False, "GetEnumerator", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["System.Runtime.Serialization", "SerializationInfo", False, "GetString", "(System.String)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["System.Runtime.Serialization", "SerializationInfo", False, "GetValue", "(System.String,System.Type)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["System.Runtime.Serialization", "SerializationInfoEnumerator", False, "get_Current", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]


This returns a SerializationEntry struct. I'm not familiar with CodeQL for C#, so I just want to check that it's sufficient to taint that struct, and then accessing the fields will propagate the taint, without requiring any extra models.

No, we would need extra models for that as well. The model itself doesn't bring any value unless models for SerializationEntry is also provided. However, I decided to include it for consistency (such that all properties of SerializationInfoEnumerator are modelled - or at least those where it makes sense to have a model). But I have stop the modelling somehere 😄

That makes sense. Perhaps a comment on this line saying that it won't work until models for SerializationEntry are added, would save the next person to run into this some time.

owen-mc · 2025-07-15T12:08:11Z

csharp/ql/lib/ext/System.IO.model.yml

@@ -47,7 +47,7 @@ extensions:
      - ["System.IO", "FileStream", False, "FileStream", "(System.String,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare,System.Int32)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
      - ["System.IO", "FileStream", False, "FileStream", "(System.String,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare,System.Int32,System.Boolean)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
      - ["System.IO", "FileStream", False, "FileStream", "(System.String,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare,System.Int32,System.IO.FileOptions)", "", "Argument[0]", "Argument[this]", "taint", "manual"]
-      - ["System.IO", "MemoryStream", False, "MemoryStream", "(System.Byte[])", "", "Argument[0]", "Argument[this]", "taint", "manual"]
+      - ["System.IO", "MemoryStream", False, "MemoryStream", "(System.Byte[])", "", "Argument[0].Element", "Argument[this]", "taint", "manual"]


Interesting. We do the opposite in CodeQL for Go - byte arrays are often used synonymously with strings, and we always taint the whole array rather than individual elements, just as we would if it was a string. I guess either way works as long as your consistent, but I seem to recall that for Go it was better to taint the whole array because strings can be considered as byte arrays in some circumstances. Just thought I'd mention it.

In Java, we would also taint a whole byte[] rather than its members - in fact single byte values are typically considered too small to carry taint and used as type-based sanitizers.

Thank you both for looking into this!
So the change is probably a bit more controversial than advertised in slack. 😄
To address the questions/comments: Yes, I think it makes sense to consider both byte[] and char[] as "bulk" like datatypes like it is done for Java. The change in this PR solves the reported issue (as it is now) and doesn't break the pattern that is already being used for C# in terms of modelling - but we should probably change this.
Will it be acceptable to merge the models as they are and then as a follow up try and convert all the models (at least all the manual ones) to check what the consequences will be? This might also tie into model generator as well.

owen-mc · 2025-07-15T12:23:42Z

csharp/ql/lib/ext/System.Text.model.yml

@@ -3,18 +3,18 @@ extensions:
      pack: codeql/csharp-all
      extensible: summaryModel
    data:
-      - ["System.Text", "Encoding", True, "GetBytes", "(System.Char*,System.Int32,System.Byte*,System.Int32)", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["System.Text", "Encoding", True, "GetBytes", "(System.Char*,System.Int32,System.Byte*,System.Int32)", "", "Argument[0].Element", "Argument[2]", "taint", "manual"]


For many of the models in this file, I don't understand why the input has .Element but the output doesn't. Please can you explain how this works? (Obviously the old model was just wrong, though.)

That is an excellent question - I didn't want to investigate that as a part of adding the models - but now I have checked: The cause is that for C#, support for implicit reads at sinks hasn't been implemented. That is, if the elements of a collection are tainted but the sink is a collection type, then taint isn't propagated. Since we have a few byte[] sinks, it appears that the models are made like this.
This should probably be addressed (but not as a part of this PR) - will open an issue for it (I think it is just a matter of making a simple implementation for defaultImplicitTaintRead - but it will need to be tested separately).

Looks like all the skeletons are falling out of the closet 😄

That seems fine to me, as long as there's a follow-up issue and we don't totally forget about it.

I have opened issues for both ((1) Implicit reads at sinks and (2) Convert byte[] and char[] models)).

…the other models).

…Reader models to manual models.

…yReader.CreateBinaryReader.

…ator.

github-actions bot added the C# label Jul 1, 2025

michaelnebel force-pushed the csharp/fixmodels branch from 822992d to 8256741 Compare July 1, 2025 13:35

michaelnebel force-pushed the csharp/fixmodels branch from 8256741 to c60a040 Compare July 1, 2025 14:21

github-actions bot added the documentation label Jul 1, 2025

michaelnebel force-pushed the csharp/fixmodels branch from c60a040 to 822486e Compare July 3, 2025 12:18

michaelnebel marked this pull request as ready for review July 3, 2025 13:26

Copilot AI review requested due to automatic review settings July 3, 2025 13:26

michaelnebel requested a review from a team as a code owner July 3, 2025 13:26

Copilot AI reviewed Jul 3, 2025

View reviewed changes

owen-mc reviewed Jul 15, 2025

View reviewed changes

michaelnebel added 11 commits July 16, 2025 08:41

C#: Add Deserialize testcase.

4036140

C#: Fix erroneous model the MemoryStream constructor (and align with …

13b40bb

…the other models).

C#: Update test expected output.

8ee16f6

C#: Promote the generated System.Xml.XmlDictionaryReader.CreateBinary…

3ae69d5

…Reader models to manual models.

C#: Add models for the remaining overloads of System.Xml.XmlDictionar…

064c4fc

…yReader.CreateBinaryReader.

C#: Improve the models for System.Text.Encoding.[GetBytes|GetChars].

5c05ff8

C#: Add some models for SerializationInto and SerializationInfoEnumer…

95763dd

…ator.

C#: Update flow summaries expected output.

eba901f

C#: Add change-note.

8f8b042

C#: Convert Deserialization tests to use inline expectations.

70bf61d

C#: Address review comments.

e9fdca7

michaelnebel force-pushed the csharp/fixmodels branch from 5d1f2a1 to e9fdca7 Compare July 16, 2025 09:12

owen-mc approved these changes Jul 16, 2025

View reviewed changes

C#: Improve some existing manual models. #19940

Are you sure you want to change the base?

C#: Improve some existing manual models. #19940

Conversation

michaelnebel commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

csharp

Generated file changes for csharp

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

owen-mc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschackmull Jul 16, 2025 • 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 Jul 16, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

michaelnebel Jul 16, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelnebel Jul 16, 2025 • 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!

Uh oh!

michaelnebel commented Jul 1, 2025 •

edited

Loading

github-actions bot commented Jul 1, 2025 •

edited

Loading

aschackmull Jul 16, 2025 •

edited

Loading

michaelnebel Jul 16, 2025 •

edited

Loading

michaelnebel Jul 16, 2025 •

edited

Loading

michaelnebel Jul 16, 2025 •

edited

Loading