[ASM] iast: Tainting of DefaultInterpolatedStringHandler #6340
Conversation
This file contains 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
Datadog ReportBranch report: ✅ 0 Failed, 465025 Passed, 3606 Skipped, 33h 42m 41.34s Total Time New Flaky Tests (1)
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6340) - mean (69ms) : 66, 72
. : milestone, 69,
master - mean (69ms) : 67, 71
. : milestone, 69,
section CallTarget+Inlining+NGEN
This PR (6340) - mean (980ms) : 954, 1006
. : milestone, 980,
master - mean (982ms) : 953, 1010
. : milestone, 982,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6340) - mean (108ms) : 106, 110
. : milestone, 108,
master - mean (108ms) : 106, 111
. : milestone, 108,
section CallTarget+Inlining+NGEN
This PR (6340) - mean (676ms) : 660, 692
. : milestone, 676,
master - mean (677ms) : 663, 691
. : milestone, 677,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6340) - mean (91ms) : 89, 93
. : milestone, 91,
master - mean (91ms) : 89, 93
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6340) - mean (627ms) : 613, 640
. : milestone, 627,
master - mean (635ms) : 617, 653
. : milestone, 635,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6340) - mean (190ms) : 186, 194
. : milestone, 190,
master - mean (191ms) : 186, 196
. : milestone, 191,
section CallTarget+Inlining+NGEN
This PR (6340) - mean (1,095ms) : 1062, 1127
. : milestone, 1095,
master - mean (1,091ms) : 1059, 1122
. : milestone, 1091,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6340) - mean (275ms) : 272, 279
. : milestone, 275,
master - mean (276ms) : 271, 282
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (6340) - mean (870ms) : 843, 896
. : milestone, 870,
master - mean (871ms) : 839, 903
. : milestone, 871,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6340) - mean (266ms) : 262, 270
. : milestone, 266,
master - mean (265ms) : 261, 269
. : milestone, 265,
section CallTarget+Inlining+NGEN
This PR (6340) - mean (852ms) : 818, 886
. : milestone, 852,
master - mean (851ms) : 820, 881
. : milestone, 851,
|
Benchmarks Report for tracer 🐌Benchmarks for #6340 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Benchmarks Report for appsec 🐌Benchmarks for #6340 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations
|
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 | 255.27 KB | 256.74 KB | 1.46 KB | 0.57% |
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net472 | 278.53 KB | 276.15 KB | -2.38 KB | -0.85% |
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 | 62.06 KB | 57.34 KB | -4.71 KB | -7.59% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StringConcatBenchmark |
net6.0 | 59.3μs | 856ns | 8.56μs | 0 | 0 | 0 | 43.44 KB |
| master | StringConcatBenchmark |
netcoreapp3.1 | 60.2μs | 834ns | 8.08μs | 0 | 0 | 0 | 42.64 KB |
| master | StringConcatBenchmark |
net472 | 37.5μs | 110ns | 411ns | 0 | 0 | 0 | 62.06 KB |
| master | StringConcatAspectBenchmark |
net6.0 | 317μs | 1.68μs | 11.9μs | 0 | 0 | 0 | 255.27 KB |
| master | StringConcatAspectBenchmark |
netcoreapp3.1 | 339μs | 1.93μs | 14.2μs | 0 | 0 | 0 | 252.96 KB |
| master | StringConcatAspectBenchmark |
net472 | 287μs | 6.12μs | 59.7μs | 0 | 0 | 0 | 278.53 KB |
| #6340 | StringConcatBenchmark |
net6.0 | 52.5μs | 276ns | 1.43μs | 0 | 0 | 0 | 43.44 KB |
| #6340 | StringConcatBenchmark |
netcoreapp3.1 | 54.6μs | 297ns | 1.6μs | 0 | 0 | 0 | 42.64 KB |
| #6340 | StringConcatBenchmark |
net472 | 38μs | 145ns | 544ns | 0 | 0 | 0 | 57.34 KB |
| #6340 | StringConcatAspectBenchmark |
net6.0 | 314μs | 5.2μs | 49.9μs | 0 | 0 | 0 | 256.74 KB |
| #6340 | StringConcatAspectBenchmark |
netcoreapp3.1 | 344μs | 1.88μs | 12.3μs | 0 | 0 | 0 | 253.94 KB |
| #6340 | StringConcatAspectBenchmark |
net472 | 259μs | 1.05μs | 3.65μs | 0 | 0 | 0 | 276.15 KB |
e-n-0
force-pushed
the
flavien/iast/interpolatedstring
branch
from
7758970 to
e09b3ff
Compare
e-n-0
force-pushed
the
flavien/iast/interpolatedstring
branch
from
e09b3ff to
8b65a31
Compare
tracer/src/Datadog.Trace/Iast/Propagation/DefaultInterpolatedStringHandlerModuleImpl.cs
Outdated
Show resolved
Hide resolved
daniel-romano-DD
approved these changes
Nov 25, 2024
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6340) (11.179M) : 0, 11179393
master (11.171M) : 0, 11171321
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6340) (7.339M) : 0, 7338549
master (7.220M) : 0, 7220293
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.672M) : 0, 7672287
section Manual
master (11.297M) : 0, 11297089
section Manual + Automatic
This PR (6340) (6.791M) : 0, 6791397
master (6.666M) : 0, 6665729
section DD_TRACE_ENABLED=0
master (10.155M) : 0, 10155271
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6340) (9.480M) : 0, 9479788
master (9.792M) : 0, 9792239
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6340) (6.446M) : 0, 6446414
master (6.460M) : 0, 6460434
section Trace stats
master (6.580M) : 0, 6580287
section Manual
master (9.509M) : 0, 9508789
section Manual + Automatic
This PR (6340) (6.031M) : 0, 6030977
master (5.968M) : 0, 5968264
section DD_TRACE_ENABLED=0
master (8.863M) : 0, 8863124
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6340) (10.658M) : 0, 10657560
master (9.925M) : 0, 9924990
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6340) (6.925M) : 0, 6924546
master (6.383M) : 0, 6383332
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Trace stats
master (7.050M) : 0, 7050172
section Manual
master (9.974M) : 0, 9974221
section Manual + Automatic
This PR (6340) (6.442M) : 0, 6442129
master (5.768M) : 0, 5768402
section DD_TRACE_ENABLED=0
master (9.135M) : 0, 9134686
|
tracer/src/Datadog.Trace/Iast/Aspects/System.Runtime/DefaultInterpolatedStringHandlerAspect.cs
Show resolved
Hide resolved
| /// <param name="target"> the ref DefaultInterpolatedStringHandler </param> | ||
| /// <param name="value"> the string value </param> | ||
| [AspectMethodReplace("System.Runtime.CompilerServices.DefaultInterpolatedStringHandler::AppendFormatted(System.String)")] | ||
| public static void AppendFormatted1(ref DefaultInterpolatedStringHandler target, string value) |
There was a problem hiding this comment.
You might also consider to add these overloads:
AppendFormatted(Object, Int32, String)
AppendFormatted(T, String)
AppendFormatted(T, Int32, String)
Actually, you could add unit tests in IastInstrumentationUnitTests.TestMethodsAspectCover and TestAllAspectsHaveACorrespondingMethod to make sure that you cover every method that you want to cover for every framework and that you don't have useless aspects for some frameworks
There was a problem hiding this comment.
We should cover relevant string related overloads, those used by the compiler when generating code for interpolated strings
There was a problem hiding this comment.
But if a string is declared as an object, we could potentially miss it. For instance, this test would fail:
[Fact]
public void GivenImplicitInterpolatedString_WhenAddingTaintedValuesComplex_GetString_Vulnerable()
{
var interpolatedString = $"""
Hello "{((object)TaintedValue)}".
.
""";
AssertTainted(interpolatedString);
}
There was a problem hiding this comment.
Also, adding these unit test could detect if we are not covering a new string method in the DefaultInterpolatedStringHandler class that could be included in future versions of .Net
There was a problem hiding this comment.
😄
- I applied in new commits the supports of this test case
- I also added the tests in the
IastInstrumentationUnitTestsclass for the aspects checks
|
|
||
| internal static class DefaultInterpolatedStringHandlerModuleImpl | ||
| { | ||
| public static unsafe void Append(IntPtr target, string? value) |
There was a problem hiding this comment.
We could consider adding some logic to keep the ranges in some methods such as append without format (like we do with strings). Anyway, this could be done in a separate PR and we should evaluate also the performance cost, so maybe it's not worth it.
There was a problem hiding this comment.
Do you mean like keeping correct start and length of tainted data in the ranges?
Currently it's difficult to know the current length before appending the new data to the DefaultInterpolatedStringHandler. The _pos int of the struct is private. 😞
We could get the value by knowing the exact offset in the struct but that would be unsafe (and not done for this PR atm).
andrewlock
approved these changes
Nov 26, 2024
tracer/src/Datadog.Trace/Iast/Aspects/System.Runtime/DefaultInterpolatedStringHandlerAspect.cs
Outdated
Show resolved
Hide resolved
tracer/src/Datadog.Trace/Iast/Aspects/System.Runtime/DefaultInterpolatedStringHandlerAspect.cs
Outdated
Show resolved
Hide resolved
e-n-0
force-pushed
the
flavien/iast/interpolatedstring
branch
from
f848ba5 to
c7ffef0
Compare
e-n-0
force-pushed
the
flavien/iast/interpolatedstring
branch
from
64cced2 to
8e6d1c9
Compare
e-n-0
force-pushed
the
flavien/iast/interpolatedstring
branch
from
8e6d1c9 to
bfdfd64
Compare
veerbia
pushed a commit
that referenced
this pull request
Dec 16, 2024
## Summary of changes This PR introduce the support of `DefaultInterpolatedStringHandler` for IAST. The resulting strings of `DefaultInterpolatedStringHandler` will now be tainted. ## Reason for change Since the release of .NET Core 6, interpolated strings got a performance optimisation and now use [DefaultInterpolatedStringHandler](https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.defaultinterpolatedstringhandler?view=net-6.0) to build strings. Some vulnerabilities couldn't be detected because strings built with `DefaultInterpolatedStringHandler` weren't tainted. ## Implementation details As `DefaultInterpolatedStringHandler` is a ref struct, we call some IL to get it's own stack pointer value and taint it. We need to get that pointer value to track it and its tainted sources. ## Test coverage - New unit tests were added: - testing all aspects with an explicit call to the `DefaultInterpolatedStringHandler` ref struct - testing implicit interpolated strings (`$""`) in various complex cases - Aspects tests added for `IastInstrumentationUnitTests` ## Other details This PR don't handle the correct values for `start` and `length` of tainted Sources. --------- Co-authored-by: Daniel Romano <108014683+daniel-romano-DD@users.noreply.github.com>
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.
Summary of changes
This PR introduce the support of
DefaultInterpolatedStringHandlerfor IAST.The resulting strings of
DefaultInterpolatedStringHandlerwill now be tainted.Reason for change
Since the release of .NET Core 6, interpolated strings got a performance optimisation and now use DefaultInterpolatedStringHandler to build strings.
Some vulnerabilities couldn't be detected because strings built with
DefaultInterpolatedStringHandlerweren't tainted.Implementation details
As
DefaultInterpolatedStringHandleris a ref struct, we call some IL to get it's own stack pointer value and taint it.We need to get that pointer value to track it and its tainted sources.
Test coverage
DefaultInterpolatedStringHandlerref struct$"") in various complex casesIastInstrumentationUnitTestsOther details
This PR don't handle the correct values for
startandlengthof tainted Sources.