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.

Sign up for GitHub

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

Jump to bottom

[IAST] Improve RestSharp SSRF detection #6060

Merged
merged 9 commits into from
Sep 25, 2024
Merged

[IAST] Improve RestSharp SSRF detection #6060

merged 9 commits into from
Sep 25, 2024

Conversation

daniel-romano-DD
Copy link
Contributor

@daniel-romano-DD daniel-romano-DD commented Sep 20, 2024 •
edited
Loading

Summary of changes

Improvements in the tainting of string.Replace and string.Format.
Improvements in escape functions.
Addition of SSRF secure mark.
Instrumentation of UrlEscape methods.

Reason for change

Inaccurate detection of SSRF in RestSharp queries have been reported.

Implementation details

Added a check to verify if the result string is the same as the original when tainting the whole string.
This happens in Replace and Format strings, when the function does not modify the input.
For instance "text".Replace("1", "2") will return "text", but if "2" is tainted, the result would be also, and this was incorrect.

Fixed the escape function to propagate the secure mark in the escaped function, and not in the origin, as was done until now.

Added a CallTarget in the UrlEncode to properly escape the result and propagate a secure mark for SSRF, for when AddParameter is called with the encode argument to true (by default).

Test coverage

Added unit tests to check the new proper Replace and Format functionality, as well as the UrlEncode and the RestSharp SSRF detection

Other details

@andrewlock
Copy link
Member

andrewlock commented Sep 20, 2024 •
edited
Loading

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:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

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 (6060) - mean (71ms)  : 68, 74
     .   : milestone, 71,
    master - mean (70ms)  : 67, 72
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (6060) - mean (1,111ms)  : 1089, 1132
     .   : milestone, 1111,
    master - mean (1,101ms)  : 1080, 1122
     .   : milestone, 1101,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6060) - mean (110ms)  : 107, 113
     .   : milestone, 110,
    master - mean (109ms)  : 106, 111
     .   : milestone, 109,

    section CallTarget+Inlining+NGEN
    This PR (6060) - mean (776ms)  : 758, 794
     .   : milestone, 776,
    master - mean (768ms)  : 751, 786
     .   : milestone, 768,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6060) - mean (94ms)  : 89, 98
     .   : milestone, 94,
    master - mean (92ms)  : 89, 94
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (6060) - mean (732ms)  : 712, 753
     .   : milestone, 732,
    master - mean (724ms)  : 707, 742
     .   : milestone, 724,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6060) - mean (191ms)  : 187, 194
     .   : milestone, 191,
    master - mean (190ms)  : 187, 194
     .   : milestone, 190,

    section CallTarget+Inlining+NGEN
    This PR (6060) - mean (1,198ms)  : 1175, 1220
     .   : milestone, 1198,
    master - mean (1,190ms)  : 1169, 1211
     .   : milestone, 1190,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6060) - mean (277ms)  : 273, 281
     .   : milestone, 277,
    master - mean (276ms)  : 271, 282
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (6060) - mean (945ms)  : 924, 966
     .   : milestone, 945,
    master - mean (933ms)  : 918, 949
     .   : milestone, 933,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6060) - mean (265ms)  : 261, 269
     .   : milestone, 265,
    master - mean (265ms)  : 261, 269
     .   : milestone, 265,

    section CallTarget+Inlining+NGEN
    This PR (6060) - mean (927ms)  : 910, 944
     .   : milestone, 927,
    master - mean (922ms)  : 905, 938
     .   : milestone, 922,
Loading

@andrewlock
Copy link
Member

andrewlock commented Sep 20, 2024 •
edited
Loading

Benchmarks Report for appsec 🐌

Benchmarks for #6060 compared to master:

  • All benchmarks have the same speed
  • 2 benchmarks have fewer allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 71.1μs 113ns 421ns 0.0705 0 0 6.01 KB
master AllCycleSimpleBody netcoreapp3.1 62.6μs 102ns 396ns 0.0931 0 0 6.95 KB
master AllCycleSimpleBody net472 48.1μs 40.5ns 152ns 1.32 0 0 8.34 KB
master AllCycleMoreComplexBody net6.0 78.4μs 78.2ns 303ns 0.117 0 0 9.51 KB
master AllCycleMoreComplexBody netcoreapp3.1 70.5μs 96.6ns 374ns 0.106 0 0 10.37 KB
master AllCycleMoreComplexBody net472 55.1μs 35.2ns 136ns 1.88 0.0276 0 11.85 KB
master ObjectExtractorSimpleBody net6.0 141ns 0.0722ns 0.26ns 0.00398 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 200ns 0.322ns 1.16ns 0.00358 0 0 272 B
master ObjectExtractorSimpleBody net472 177ns 0.18ns 0.672ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.11μs 1.69ns 6.54ns 0.0528 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 3.83μs 4.29ns 16.6ns 0.0496 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.91μs 4.54ns 17ns 0.601 0.00586 0 3.8 KB
#6060 AllCycleSimpleBody net6.0 72.9μs 64.9ns 243ns 0.0727 0 0 6 KB
#6060 AllCycleSimpleBody netcoreapp3.1 61.2μs 110ns 413ns 0.091 0 0 6.95 KB
#6060 AllCycleSimpleBody net472 47.9μs 53.3ns 206ns 1.31 0 0 8.34 KB
#6060 AllCycleMoreComplexBody net6.0 78.6μs 99ns 357ns 0.117 0 0 9.51 KB
#6060 AllCycleMoreComplexBody netcoreapp3.1 68.5μs 92.5ns 346ns 0.137 0 0 10.37 KB
#6060 AllCycleMoreComplexBody net472 56μs 105ns 407ns 1.87 0.028 0 11.85 KB
#6060 ObjectExtractorSimpleBody net6.0 140ns 0.0659ns 0.247ns 0.00391 0 0 280 B
#6060 ObjectExtractorSimpleBody netcoreapp3.1 205ns 0.139ns 0.5ns 0.00361 0 0 272 B
#6060 ObjectExtractorSimpleBody net472 176ns 0.056ns 0.217ns 0.0446 0 0 281 B
#6060 ObjectExtractorMoreComplexBody net6.0 3.06μs 2.02ns 7.55ns 0.0533 0 0 3.78 KB
#6060 ObjectExtractorMoreComplexBody netcoreapp3.1 3.88μs 4.82ns 18.6ns 0.0501 0 0 3.69 KB
#6060 ObjectExtractorMoreComplexBody net472 3.8μs 1.38ns 5.18ns 0.603 0.00569 0 3.8 KB
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EncodeArgs net6.0 36.8μs 23.9ns 92.7ns 0.443 0 0 32.4 KB
master EncodeArgs netcoreapp3.1 54.3μs 31.6ns 122ns 0.433 0 0 32.4 KB
master EncodeArgs net472 66.4μs 13.4ns 48.3ns 5.16 0.0662 0 32.5 KB
master EncodeLegacyArgs net6.0 71μs 45.9ns 178ns 0 0 0 2.14 KB
master EncodeLegacyArgs netcoreapp3.1 106μs 256ns 990ns 0 0 0 2.14 KB
master EncodeLegacyArgs net472 153μs 89.3ns 334ns 0.305 0 0 2.15 KB
#6060 EncodeArgs net6.0 37μs 22.9ns 85.7ns 0.46 0 0 32.4 KB
#6060 EncodeArgs netcoreapp3.1 54μs 23.5ns 88ns 0.431 0 0 32.4 KB
#6060 EncodeArgs net472 66.4μs 32.5ns 121ns 5.13 0.0662 0 32.5 KB
#6060 EncodeLegacyArgs net6.0 72μs 44.2ns 165ns 0 0 0 2.14 KB
#6060 EncodeLegacyArgs netcoreapp3.1 106μs 76.5ns 296ns 0 0 0 2.15 KB
#6060 EncodeLegacyArgs net472 153μs 101ns 390ns 0.306 0 0 2.15 KB
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunWafRealisticBenchmark net6.0 184μs 57ns 213ns 0 0 0 2.44 KB
master RunWafRealisticBenchmark netcoreapp3.1 196μs 123ns 477ns 0 0 0 2.39 KB
master RunWafRealisticBenchmark net472 209μs 57.1ns 214ns 0.312 0 0 2.46 KB
master RunWafRealisticBenchmarkWithAttack net6.0 123μs 49.8ns 193ns 0 0 0 1.47 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 130μs 125ns 468ns 0 0 0 1.46 KB
master RunWafRealisticBenchmarkWithAttack net472 139μs 22.4ns 80.7ns 0.208 0 0 1.48 KB
#6060 RunWafRealisticBenchmark net6.0 185μs 146ns 564ns 0 0 0 2.44 KB
#6060 RunWafRealisticBenchmark netcoreapp3.1 198μs 215ns 832ns 0 0 0 2.39 KB
#6060 RunWafRealisticBenchmark net472 209μs 86.1ns 334ns 0.312 0 0 2.46 KB
#6060 RunWafRealisticBenchmarkWithAttack net6.0 122μs 148ns 574ns 0 0 0 1.47 KB
#6060 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 131μs 99.7ns 360ns 0 0 0 1.46 KB
#6060 RunWafRealisticBenchmarkWithAttack net472 139μs 78ns 302ns 0.209 0 0 1.49 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #6060

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 256.82 KB 255.46 KB -1.35 KB -0.53%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 59.6 KB 57.34 KB -2.26 KB -3.79%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 60.5μs 803ns 8.03μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 60.2μs 873ns 8.69μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37μs 66.7ns 250ns 0 0 0 59.6 KB
master StringConcatAspectBenchmark net6.0 316μs 1.25μs 4.33μs 0 0 0 256.82 KB
master StringConcatAspectBenchmark netcoreapp3.1 341μs 1.64μs 8.96μs 0 0 0 253.41 KB
master StringConcatAspectBenchmark net472 282μs 6.1μs 60.4μs 0 0 0 278.53 KB
#6060 StringConcatBenchmark net6.0 60.8μs 876ns 8.72μs 0 0 0 43.44 KB
#6060 StringConcatBenchmark netcoreapp3.1 52.4μs 244ns 978ns 0 0 0 42.64 KB
#6060 StringConcatBenchmark net472 37.1μs 150ns 635ns 0 0 0 57.34 KB
#6060 StringConcatAspectBenchmark net6.0 313μs 1.77μs 12μs 0 0 0 255.46 KB
#6060 StringConcatAspectBenchmark netcoreapp3.1 348μs 2.02μs 16.7μs 0 0 0 252.82 KB
#6060 StringConcatAspectBenchmark net472 279μs 6.94μs 66.9μs 0 0 0 278.53 KB

@andrewlock
Copy link
Member

andrewlock commented Sep 20, 2024 •
edited
Loading

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 (6060) (10.907M)   : 0, 10906875
    master (10.985M)   : 0, 10985482
    benchmarks/2.9.0 (11.081M)   : 0, 11080577

    section Automatic
    This PR (6060) (7.065M)   : 0, 7065286
    master (7.069M)   : 0, 7068614
    benchmarks/2.9.0 (7.732M)   : 0, 7732233

    section Trace stats
    master (7.445M)   : 0, 7444611

    section Manual
    master (10.788M)   : 0, 10788136

    section Manual + Automatic
    This PR (6060) (6.543M)   : 0, 6543421
    master (6.507M)   : 0, 6507289

    section DD_TRACE_ENABLED=0
    master (9.793M)   : 0, 9793388
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6060) (9.376M)   : 0, 9375715
    master (9.540M)   : 0, 9540100
    benchmarks/2.9.0 (9.798M)   : 0, 9798067

    section Automatic
    This PR (6060) (6.667M)   : 0, 6667342
    master (6.620M)   : 0, 6620231

    section Trace stats
    master (6.802M)   : 0, 6801791

    section Manual
    master (9.545M)   : 0, 9545216

    section Manual + Automatic
    This PR (6060) (6.157M)   : 0, 6156784
    master (6.127M)   : 0, 6126637

    section DD_TRACE_ENABLED=0
    master (8.654M)   : 0, 8654022
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6060) (10.268M)   : 0, 10268012
    master (10.337M)   : 0, 10337059
    benchmarks/2.9.0 (10.067M)   : 0, 10067315

    section Automatic
    This PR (6060) (6.687M)   : 0, 6687022
    master (6.596M)   : 0, 6595628
    benchmarks/2.9.0 (7.552M)   : 0, 7552193

    section Trace stats
    master (7.428M)   : 0, 7427719

    section Manual
    master (9.976M)   : 0, 9976270

    section Manual + Automatic
    This PR (6060) (6.420M)   : 0, 6420085
    master (6.092M)   : 0, 6091995

    section DD_TRACE_ENABLED=0
    master (9.372M)   : 0, 9371961
Loading

@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Sep 20, 2024 •
edited
Loading

Datadog Report

Branch report: dani/iast/string_replace_tainting_fix
Commit report: 2138428
Test service: dd-trace-dotnet

✅ 0 Failed, 483557 Passed, 3910 Skipped, 42h 18m 33.82s Total Time

@andrewlock
Copy link
Member

andrewlock commented Sep 20, 2024 •
edited
Loading

Benchmarks Report for tracer 🐌

Benchmarks for #6060 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.121
  • 2 benchmarks are slower, with geometric mean 1.155
  • All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 7.7μs 41.9ns 230ns 0.0155 0.00388 0 5.42 KB
master StartStopWithChild netcoreapp3.1 9.71μs 50.2ns 246ns 0.0155 0.00518 0 5.62 KB
master StartStopWithChild net472 16.1μs 34.1ns 132ns 1.02 0.319 0.0958 6.06 KB
#6060 StartStopWithChild net6.0 7.65μs 43.4ns 298ns 0.0214 0.0107 0 5.43 KB
#6060 StartStopWithChild netcoreapp3.1 9.95μs 52.7ns 264ns 0.0199 0.00995 0 5.61 KB
#6060 StartStopWithChild net472 16.3μs 59.6ns 223ns 1 0.294 0.0795 6.05 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 479μs 274ns 1.02μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 649μs 329ns 1.27μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 831μs 761ns 2.95μs 0.414 0 0 3.3 KB
#6060 WriteAndFlushEnrichedTraces net6.0 464μs 204ns 735ns 0 0 0 2.7 KB
#6060 WriteAndFlushEnrichedTraces netcoreapp3.1 645μs 351ns 1.36μs 0 0 0 2.7 KB
#6060 WriteAndFlushEnrichedTraces net472 841μs 714ns 2.76μs 0.419 0 0 3.3 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 192μs 1.05μs 6.41μs 0.189 0 0 18.45 KB
master SendRequest netcoreapp3.1 214μs 1.09μs 4.87μs 0.21 0 0 20.61 KB
master SendRequest net472 0.000791ns 0.000284ns 0.00106ns 0 0 0 0 b
#6060 SendRequest net6.0 189μs 904ns 3.5μs 0.195 0 0 18.45 KB
#6060 SendRequest netcoreapp3.1 218μs 1.23μs 8.42μs 0.219 0 0 20.61 KB
#6060 SendRequest net472 0.00218ns 0.00106ns 0.00396ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 544μs 1.76μs 6.36μs 0.571 0 0 41.48 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 661μs 2.19μs 7.6μs 0.338 0 0 41.89 KB
master WriteAndFlushEnrichedTraces net472 835μs 3.25μs 12.2μs 8.45 2.53 0.422 53.32 KB
#6060 WriteAndFlushEnrichedTraces net6.0 578μs 2.05μs 7.93μs 0.581 0 0 41.56 KB
#6060 WriteAndFlushEnrichedTraces netcoreapp3.1 702μs 3.71μs 19.3μs 0.34 0 0 41.72 KB
#6060 WriteAndFlushEnrichedTraces net472 868μs 4.22μs 16.9μs 8.13 2.57 0.428 53.31 KB
Benchmarks.Trace.DbCommandBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6060

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑net6.0 1.154 1,190.02 1,373.59

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.19μs 1.21ns 4.7ns 0.0142 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.71μs 1.33ns 5.14ns 0.0138 0 0 1.02 KB
master ExecuteNonQuery net472 2.03μs 2.81ns 10.9ns 0.157 0 0 987 B
#6060 ExecuteNonQuery net6.0 1.37μs 1.85ns 6.93ns 0.0145 0 0 1.02 KB
#6060 ExecuteNonQuery netcoreapp3.1 1.76μs 1.69ns 6.34ns 0.0132 0 0 1.02 KB
#6060 ExecuteNonQuery net472 2.16μs 4.15ns 15.5ns 0.156 0 0 987 B
Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6060

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net6.0 1.156 1,143.06 1,320.87

Faster 🎉 in #6060

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑net6.0 1.121 1,381.93 1,233.12

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.14μs 0.499ns 1.87ns 0.0137 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.55μs 0.933ns 3.23ns 0.0132 0 0 976 B
master CallElasticsearch net472 2.55μs 0.653ns 2.44ns 0.157 0 0 995 B
master CallElasticsearchAsync net6.0 1.38μs 1.84ns 6.64ns 0.0133 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.58μs 1.13ns 4.39ns 0.0135 0 0 1.02 KB
master CallElasticsearchAsync net472 2.69μs 1.1ns 4.26ns 0.167 0 0 1.05 KB
#6060 CallElasticsearch net6.0 1.32μs 1.29ns 4.98ns 0.0132 0 0 976 B
#6060 CallElasticsearch netcoreapp3.1 1.59μs 5.87ns 22.7ns 0.0132 0 0 976 B
#6060 CallElasticsearch net472 2.44μs 1.33ns 5.14ns 0.158 0 0 995 B
#6060 CallElasticsearchAsync net6.0 1.24μs 1.65ns 6.41ns 0.0133 0 0 952 B
#6060 CallElasticsearchAsync netcoreapp3.1 1.68μs 1.1ns 4.12ns 0.0135 0 0 1.02 KB
#6060 CallElasticsearchAsync net472 2.56μs 1.44ns 5.38ns 0.166 0 0 1.05 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net6.0 1.21μs 1.85ns 7.17ns 0.0132 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.63μs 0.708ns 2.65ns 0.013 0 0 952 B
master ExecuteAsync net472 1.74μs 0.634ns 2.46ns 0.145 0 0 915 B
#6060 ExecuteAsync net6.0 1.31μs 0.585ns 2.27ns 0.013 0 0 952 B
#6060 ExecuteAsync netcoreapp3.1 1.59μs 1.63ns 6.3ns 0.0127 0 0 952 B
#6060 ExecuteAsync net472 1.83μs 0.805ns 3.12ns 0.145 0 0 915 B
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net6.0 4.18μs 1.75ns 6.55ns 0.0313 0 0 2.22 KB
master SendAsync netcoreapp3.1 5.03μs 2.52ns 9.43ns 0.0376 0 0 2.76 KB
master SendAsync net472 7.83μs 2.08ns 8.06ns 0.499 0 0 3.15 KB
#6060 SendAsync net6.0 4.18μs 6.03ns 22.6ns 0.0311 0 0 2.22 KB
#6060 SendAsync netcoreapp3.1 5.04μs 1.49ns 5.56ns 0.038 0 0 2.76 KB
#6060 SendAsync net472 7.8μs 2.71ns 10.5ns 0.499 0 0 3.15 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 1.55μs 0.553ns 2.07ns 0.0232 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.26μs 1.38ns 5.15ns 0.0226 0 0 1.64 KB
master EnrichedLog net472 2.61μs 1.03ns 3.85ns 0.25 0 0 1.57 KB
#6060 EnrichedLog net6.0 1.45μs 0.621ns 2.4ns 0.0232 0 0 1.64 KB
#6060 EnrichedLog netcoreapp3.1 2.26μs 1.26ns 4.72ns 0.0214 0 0 1.64 KB
#6060 EnrichedLog net472 2.51μs 0.737ns 2.85ns 0.249 0 0 1.57 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 115μs 121ns 436ns 0.0574 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 118μs 195ns 754ns 0.0594 0 0 4.28 KB
master EnrichedLog net472 146μs 207ns 773ns 0.652 0.217 0 4.46 KB
#6060 EnrichedLog net6.0 114μs 172ns 667ns 0 0 0 4.28 KB
#6060 EnrichedLog netcoreapp3.1 118μs 137ns 531ns 0.0594 0 0 4.28 KB
#6060 EnrichedLog net472 146μs 178ns 690ns 0.655 0.218 0 4.46 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.07μs 7.39ns 28.6ns 0.0307 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.21μs 1.21ns 4.53ns 0.0294 0 0 2.2 KB
master EnrichedLog net472 5.05μs 1.51ns 5.83ns 0.32 0 0 2.02 KB
#6060 EnrichedLog net6.0 3.21μs 1.31ns 5.08ns 0.0304 0 0 2.2 KB
#6060 EnrichedLog netcoreapp3.1 4.24μs 3.53ns 13.7ns 0.0294 0 0 2.2 KB
#6060 EnrichedLog net472 5.01μs 1.51ns 5.86ns 0.32 0 0 2.02 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.37μs 0.684ns 2.56ns 0.0158 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.77μs 1.11ns 4.01ns 0.015 0 0 1.14 KB
master SendReceive net472 2.09μs 0.76ns 2.94ns 0.183 0 0 1.16 KB
#6060 SendReceive net6.0 1.36μs 0.636ns 2.46ns 0.0163 0 0 1.14 KB
#6060 SendReceive netcoreapp3.1 1.77μs 0.446ns 1.67ns 0.0159 0 0 1.14 KB
#6060 SendReceive net472 2.03μs 0.918ns 3.56ns 0.183 0.00102 0 1.16 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 2.71μs 1.26ns 4.73ns 0.023 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.86μs 1.18ns 4.57ns 0.0213 0 0 1.65 KB
master EnrichedLog net472 4.38μs 1.32ns 4.94ns 0.324 0 0 2.04 KB
#6060 EnrichedLog net6.0 2.65μs 1.37ns 5.29ns 0.0225 0 0 1.6 KB
#6060 EnrichedLog netcoreapp3.1 3.88μs 1.99ns 7.69ns 0.0211 0 0 1.65 KB
#6060 EnrichedLog net472 4.32μs 1.65ns 6.38ns 0.324 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 410ns 0.216ns 0.837ns 0.00802 0 0 576 B
master StartFinishSpan netcoreapp3.1 551ns 0.557ns 2.08ns 0.00774 0 0 576 B
master StartFinishSpan net472 683ns 0.499ns 1.93ns 0.0918 0 0 578 B
master StartFinishScope net6.0 545ns 0.346ns 1.34ns 0.00982 0 0 696 B
master StartFinishScope netcoreapp3.1 666ns 0.358ns 1.29ns 0.00955 0 0 696 B
master StartFinishScope net472 837ns 0.921ns 3.57ns 0.104 0 0 658 B
#6060 StartFinishSpan net6.0 416ns 0.281ns 1.09ns 0.00812 0 0 576 B
#6060 StartFinishSpan netcoreapp3.1 544ns 0.476ns 1.84ns 0.00791 0 0 576 B
#6060 StartFinishSpan net472 654ns 0.843ns 3.27ns 0.0916 0 0 578 B
#6060 StartFinishScope net6.0 555ns 0.684ns 2.65ns 0.00975 0 0 696 B
#6060 StartFinishScope netcoreapp3.1 680ns 0.456ns 1.64ns 0.00923 0 0 696 B
#6060 StartFinishScope net472 880ns 0.542ns 2.1ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 643ns 0.362ns 1.4ns 0.00975 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 917ns 0.414ns 1.55ns 0.00973 0 0 696 B
master RunOnMethodBegin net472 1.07μs 0.763ns 2.96ns 0.105 0 0 658 B
#6060 RunOnMethodBegin net6.0 596ns 0.365ns 1.42ns 0.0098 0 0 696 B
#6060 RunOnMethodBegin netcoreapp3.1 954ns 0.856ns 3.32ns 0.00957 0 0 696 B
#6060 RunOnMethodBegin net472 1.06μs 1.61ns 6.25ns 0.104 0 0 658 B

@daniel-romano-DD daniel-romano-DD force-pushed the dani/iast/string_replace_tainting_fix branch from ceae797 to e896bcf Compare September 20, 2024 13:12
@github-actions GitHub Actions
Copy link
Contributor

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

1 occurrences of :

-      "hash": -1837181716,
+      "hash": -636226626,
[...]
-      "hash": 1990393425,
+      "hash": -60481650,
[...]
-      "hash": 1170867602,
+      "hash": 990913114,
[...]
-      "hash": -1837181716,
+      "hash": -636226626,
[...]
-      "hash": 1990393425,
+      "hash": -60481650,
[...]
-      "hash": 1170867602,
+      "hash": 990913114,
[...]
-      "hash": -1837181716,
+      "hash": -636226626,
[...]
-      "hash": 1990393425,
+      "hash": -60481650,
[...]
-      "hash": 1170867602,
+      "hash": 990913114,

@daniel-romano-DD daniel-romano-DD force-pushed the dani/iast/string_replace_tainting_fix branch 2 times, most recently from 6fdfb66 to 99c81fb Compare September 24, 2024 13:47
@daniel-romano-DD daniel-romano-DD force-pushed the dani/iast/string_replace_tainting_fix branch from 99c81fb to e422bd0 Compare September 24, 2024 16:13
bouwkast
bouwkast approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@bouwkast bouwkast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I didn't review much of this as most seems to fall underneath ASM but 👍

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/RestSharp/UrlEncode2Integration.cs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniel-romano-DD

Could you add /tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/RestSharp/ @DataDog/asm-dotnet/

to the CODEOWNERS?
That way apm-idm-dotnet isn're required for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/RestSharp/UrlEncode2Integration.cs Outdated
Comment on lines 25 to 26
MinimumVersion = "1.0.0",
MaximumVersion = "112.*.*",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh is this correct that we support v 1.0.0 up to 112 with this?

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/RestSharp/UrlEncodeIntegration.cs Outdated
Comment on lines 25 to 26
MinimumVersion = "1.0.0",
MaximumVersion = "112.*.*",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh is this correct that we support v 1.0.0 up to 112 with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. first version is 104.0.0

NachoEchevarria
NachoEchevarria reviewed Sep 25, 2024
View reviewed changes
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/RestSharp/UrlEncodeIntegration.cs
/// <param name="state">Calltarget state value</param>
/// <returns>CallTargetReturn</returns>
internal static CallTargetReturn<TReturn> OnMethodEnd<TTarget, TReturn>(TReturn returnValue, Exception exception, CallTargetState state)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Since the code of this method is the same as the one in UrlEncode2Integration.cs, it would probably make sense to create a common method that is called from both instrumentation points.

NachoEchevarria
NachoEchevarria reviewed Sep 25, 2024
View reviewed changes
tracer/src/Datadog.Trace/Iast/Propagation/PropagationModuleImpl.cs
@@ -92,98 +94,186 @@ public static void AddTaintedSource(string? input, Source source)
return result;
}

public static object? PropagateResultWhenInputTainted(string? result, object? firstInput, object? secondInput = null, object? thirdInput = null, object? fourthInput = null)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static bool TryPropagateWholeResult(string? result, object? input, out TaintedObjects? taintedObjects)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: since this method does not propagate the result if it's different than the input, I would change the name of the method to avoid confusions.

NachoEchevarria
NachoEchevarria reviewed Sep 25, 2024
View reviewed changes
Datadog.Trace.sln
@@ -576,6 +576,13 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "RuntimeMetricsShutdown", "t
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Samples.Debugger.AspNetCore5", "tracer\test\test-applications\debugger\Samples.Debugger.AspNetCore5\Samples.Debugger.AspNetCore5.csproj", "{3978A7D5-7B6E-4152-9C3A-5852F1F6E223}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Generated", "Generated", "{E1B0F72C-991A-409D-9266-DE5ED1BD940E}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that these changes are done automatically when compiling the solution?

NachoEchevarria
NachoEchevarria approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@NachoEchevarria NachoEchevarria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@daniel-romano-DD daniel-romano-DD merged commit 4121af0 into master Sep 25, 2024
75 of 79 checks passed
@daniel-romano-DD daniel-romano-DD deleted the dani/iast/string_replace_tainting_fix branch September 25, 2024 13:11
@github-actions github-actions bot added this to the vNext-v3 milestone Sep 25, 2024
@andrewlock andrewlock added the type:enhancement Improvement to an existing feature label Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NachoEchevarria NachoEchevarria NachoEchevarria approved these changes

@bouwkast bouwkast bouwkast approved these changes

Assignees
No one assigned
Labels
area:asm-iast type:enhancement Improvement to an existing feature
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@daniel-romano-DD @andrewlock @bouwkast @NachoEchevarria