Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C#: Re-generate .NET 8 Runtime models. #16574

Merged
merged 13 commits into from
Jun 3, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented May 23, 2024

Re-generating the .NET runtime models revealed the very broad set of database (reader) sources.

Since we are now generating models that applies to all subclasses of database reader, the issue with result multiplicity gets a lot bigger, if we still perceive all database reader expressions as sources. As a result, we need to adjust the database reader sources. Combined with the new summaries, it reduces result multiplicities and yields better path explanations. Furthermore, it turned out that we needed to promote some of the summaries from generated to manual as there are some cases where flow is not identified for the overrides in source code, which causes us to loose results for the mono repository.

@github-actions github-actions bot added the C# label May 23, 2024
Copy link
Contributor

github-actions bot commented May 23, 2024

⚠️ 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``",44,10429,59,9
+    System,"``System.*``, ``System``",44,9873,49,9
-    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``JsonToItemsTaskFactory``, ``Microsoft.Android.Build``, ``Microsoft.Apple.Build``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NET.WebAssembly.Webcil``, ``Microsoft.VisualBasic``, ``Microsoft.WebAssembly.Build.Tasks``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",54,1518,148,
+    Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``JsonToItemsTaskFactory``, ``Microsoft.Android.Build``, ``Microsoft.Apple.Build``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.CSharp``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.NET.WebAssembly.Webcil``, ``Microsoft.VisualBasic``, ``Microsoft.WebAssembly.Build.Tasks``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",54,1357,148,
-    Totals,,98,11954,401,9
+    Totals,,98,11237,391,9
  • Changes to framework-coverage-csharp.csv:
- ILLink.Shared,,,32,,,,,,,,,,,,,,,,,,,29,3
+ ILLink.Shared,,,32,,,,,,,,,,,,,,,,,,,30,2
- Internal.IL,,,69,,,,,,,,,,,,,,,,,,,67,2
+ Internal.IL,,,46,,,,,,,,,,,,,,,,,,,44,2
- Internal.TypeSystem,,,367,,,,,,,,,,,,,,,,,,,331,36
+ Internal.TypeSystem,,,291,,,,,,,,,,,,,,,,,,,275,16
- Microsoft.CSharp,,,24,,,,,,,,,,,,,,,,,,,24,
+ Microsoft.CSharp,,,10,,,,,,,,,,,,,,,,,,,10,
- Microsoft.Diagnostics.Tools.Pgo,,,13,,,,,,,,,,,,,,,,,,,13,
+ Microsoft.Diagnostics.Tools.Pgo,,,12,,,,,,,,,,,,,,,,,,,12,
- Microsoft.Extensions.Configuration,,2,83,,,,,,,,,,,,,2,,,,,,81,2
+ Microsoft.Extensions.Configuration,,2,77,,,,,,,,,,,,,2,,,,,,76,1
- Microsoft.Extensions.DependencyInjection,,,120,,,,,,,,,,,,,,,,,,,120,
+ Microsoft.Extensions.DependencyInjection,,,96,,,,,,,,,,,,,,,,,,,95,1
- Microsoft.Extensions.DependencyModel,,,12,,,,,,,,,,,,,,,,,,,12,
+ Microsoft.Extensions.DependencyModel,,,9,,,,,,,,,,,,,,,,,,,9,
- Microsoft.Extensions.Diagnostics.Metrics,,,13,,,,,,,,,,,,,,,,,,,13,
+ Microsoft.Extensions.Diagnostics.Metrics,,,15,,,,,,,,,,,,,,,,,,,15,
- Microsoft.Extensions.Hosting,,,23,,,,,,,,,,,,,,,,,,,22,1
+ Microsoft.Extensions.Hosting,,,26,,,,,,,,,,,,,,,,,,,25,1
- Microsoft.Extensions.Logging,,,60,,,,,,,,,,,,,,,,,,,59,1
+ Microsoft.Extensions.Logging,,,53,,,,,,,,,,,,,,,,,,,52,1
- Microsoft.Interop,,,78,,,,,,,,,,,,,,,,,,,78,
+ Microsoft.Interop,,,73,,,,,,,,,,,,,,,,,,,73,
- Microsoft.VisualBasic,,,10,,,,,,,,,,,,,,,,,,,5,5
+ Microsoft.VisualBasic,,,6,,,,,,,,,,,,,,,,,,,1,5
- Mono.Linker,,,161,,,,,,,,,,,,,,,,,,,161,
+ Mono.Linker,,,158,,,,,,,,,,,,,,,,,,,158,
- System,59,44,10429,,8,8,1,,,4,5,,33,2,,3,15,17,3,4,,8460,1969
+ System,49,44,9873,,3,3,1,,,4,5,,33,2,,3,15,17,3,4,,7968,1905

@michaelnebel michaelnebel force-pushed the csharp/updatenetruntimemodels branch 4 times, most recently from 1832c2a to fe7c822 Compare May 28, 2024 13:51
@michaelnebel
Copy link
Contributor Author

michaelnebel commented May 29, 2024

For the first DCA execution (remote flow sources) there are 5 new results. 1 in the powershell project and 4 in the mono project. I spot checked the result in the powershell project and this seems sound. The result path uses at least the following new summary:

"System.Reflection", "Assembly", True, "get_Location", "()", "", "Argument[this]", "ReturnValue", "taint", "df-generated"

The path consists around 100 steps, but it appears that the assembly location is "stored" which appears to be a true positive for the cs/cleartext-storage-of-sensitive-information query.

@michaelnebel
Copy link
Contributor Author

The "lost" sql-injection results from local sources are because flow is not identified via the source code overrides of GetString (IDataRecord member). To fix this issue we promote the generated summaries to manual as they are then still considered even though the source code is present.

@michaelnebel
Copy link
Contributor Author

The latest DCA run looks good; I have also spot checked one of the local source results.
The new cs/sql-injection result on mono is because of the summary

["System.Data", "IDataRecord", True, "GetString", "(System.Int32)", "", "Argument[this]", "ReturnValue", "taint", "manual"]

which looks sound.

That is, with the update .NET 8 Runtime models we

  • Get 15 new results on our DCA projects.
  • A reduction in the multiplicity of database sourced results AND better (longer) path explanations for database sources.

@michaelnebel michaelnebel force-pushed the csharp/updatenetruntimemodels branch from 9903630 to a243eab Compare May 30, 2024 17:05
@michaelnebel michaelnebel marked this pull request as ready for review May 30, 2024 17:50
@michaelnebel michaelnebel requested a review from a team as a code owner May 30, 2024 17:50
csharp/ql/lib/ext/System.Data.model.yml Outdated Show resolved Hide resolved
csharp/ql/lib/ext/System.Data.model.yml Outdated Show resolved Hide resolved
csharp/ql/lib/ext/System.Data.model.yml Outdated Show resolved Hide resolved
@michaelnebel michaelnebel force-pushed the csharp/updatenetruntimemodels branch from a243eab to d38894a Compare May 31, 2024 12:26
@michaelnebel
Copy link
Contributor Author

DCA still looks good.

@michaelnebel michaelnebel merged commit 88b978f into github:main Jun 3, 2024
21 checks passed
@michaelnebel michaelnebel deleted the csharp/updatenetruntimemodels branch June 3, 2024 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants