C#: Improve some existing manual models. #19940
Open
+208
−149
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelnebel
force-pushed
the
csharp/fixmodels
branch
from
822992d to
8256741
Compare
Click to show differences in coveragecsharpGenerated file changes for csharp
- System,"``System.*``, ``System``",47,12139,54,5
+ System,"``System.*``, ``System``",47,12165,54,5
- Totals,,107,14403,407,9
+ Totals,,107,14429,407,9
- 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 |
michaelnebel
force-pushed
the
csharp/fixmodels
branch
from
8256741 to
c60a040
Compare
…the other models).
…Reader models to manual models.
…yReader.CreateBinaryReader.
michaelnebel
force-pushed
the
csharp/fixmodels
branch
from
c60a040 to
822486e
Compare
There was a problem hiding this comment.
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,MemoryStreamconstructors, 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
$ Alertannotation is placed on theJsonSerializerSettingsline rather than on theDeserializeObjectinvocation itself. Move the inline comment to the line starting withreturn 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 toBadBinaryFormatter1. Rename it back toBadBinaryFormatterto maintain compatibility with the test matcher.
class BadBinaryFormatter1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR we improve the models as data modelling across multiple classes.
System.Xml.XmlDictionaryReader.CreateBinaryReader.System.Runtime.Serialization.SerializationInfoandSystem.Runtime.Serialization.SerializationInfoEnumerator.System.Text.Encoding.GetBytes,System.Text.Encoding.GetCharsand the constructor forSystem.IO.MemoryStream.Also convert the deserialization tests to inline expectations.
The new findings in
mono/monoforcs/cleartext-storage-of-sensitive-informationandcs/exposure-of-sensitive-informationare both related to the improved models forGetBytes(where taint is propagated to another parameter instead of the return).