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

[ASM] Stack trace leak vulnerability detection #5067

Merged
merged 28 commits into from
Jan 24, 2024
Merged

[ASM] Stack trace leak vulnerability detection #5067

merged 28 commits into from
Jan 24, 2024

Conversation

NachoEchevarria
Copy link
Contributor

@NachoEchevarria NachoEchevarria commented Jan 16, 2024 •
edited
Loading

Summary of changes

This PR contains the implemenation of the stack trace leak vulnerability. This vulnerability is launched when there is an unhandled exception in an application and the stack is shown in the developer page. This PR does not cover the tainting of stacks.

The specifications of the vulnerability can be found here: https://datadoghq.atlassian.net/wiki/spaces/APS/pages/3162177559/Stacktrace+leak

Asp for .net framework in classic mode is not supported.

Reason for change

It's one of the vulnerabilities of this quarter.

Implementation details

Test coverage

Other details

@NachoEchevarria NachoEchevarria changed the title Nacho/stack trace leak vulnerability [ASM] Stack trace leak vulnerability detection Jan 16, 2024
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Jan 16, 2024 •
edited
Loading

Datadog Report

Branch report: nacho/StackTraceLeakVulnerability
Commit report: 4f9e3b9
Test service: dd-trace-dotnet

✅ 0 Failed, 305615 Passed, 1537 Skipped, 1h 1m 5.29s Wall Time

@andrewlock
Copy link
Member

andrewlock commented Jan 16, 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 (5067) - mean (72ms)  : 58, 85
     .   : milestone, 72,
    master - mean (72ms)  : 60, 83
     .   : milestone, 72,

    section CallTarget+Inlining+NGEN
    This PR (5067) - mean (967ms)  : 945, 989
     .   : milestone, 967,
    master - mean (959ms)  : 938, 979
     .   : milestone, 959,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5067) - mean (106ms)  : 103, 109
     .   : milestone, 106,
    master - mean (108ms)  : 104, 111
     .   : milestone, 108,

    section CallTarget+Inlining+NGEN
    This PR (5067) - mean (699ms)  : 682, 715
     .   : milestone, 699,
    master - mean (701ms)  : 683, 718
     .   : milestone, 701,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5067) - mean (90ms)  : 86, 93
     .   : milestone, 90,
    master - mean (91ms)  : 88, 93
     .   : milestone, 91,

    section CallTarget+Inlining+NGEN
    This PR (5067) - mean (657ms)  : 635, 679
     .   : milestone, 657,
    master - mean (648ms)  : 628, 669
     .   : milestone, 648,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5067) - mean (189ms)  : 186, 192
     .   : milestone, 189,
    master - mean (188ms)  : 184, 193
     .   : milestone, 188,

    section CallTarget+Inlining+NGEN
    This PR (5067) - mean (1,069ms)  : 1050, 1088
     .   : milestone, 1069,
    master - mean (1,069ms)  : 1045, 1092
     .   : milestone, 1069,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5067) - mean (272ms)  : 267, 276
     .   : milestone, 272,
    master - mean (272ms)  : 267, 278
     .   : milestone, 272,

    section CallTarget+Inlining+NGEN
    This PR (5067) - mean (1,055ms)  : 1032, 1078
     .   : milestone, 1055,
    master - mean (1,051ms)  : 1021, 1080
     .   : milestone, 1051,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5067) - mean (262ms)  : 258, 265
     .   : milestone, 262,
    master - mean (261ms)  : 256, 265
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (5067) - mean (1,020ms)  : 996, 1043
     .   : milestone, 1020,
    master - mean (1,015ms)  : 996, 1034
     .   : milestone, 1015,
Loading

@andrewlock
Copy link
Member

andrewlock commented Jan 17, 2024 •
edited
Loading

Benchmarks Report 🐌

Benchmarks for #5067 compared to master:

  • 3 benchmarks are faster, with geometric mean 1.184
  • 1 benchmarks are slower, with geometric mean 1.183
  • 3 benchmarks have fewer allocations
  • 1 benchmarks have more 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 8.83μs 46.5ns 237ns 0.0217 0.00434 0 7.48 KB
master StartStopWithChild netcoreapp3.1 10.9μs 58.5ns 321ns 0.0223 0.0111 0 7.58 KB
master StartStopWithChild net472 17.3μs 34.2ns 132ns 1.32 0.344 0.112 7.94 KB
#5067 StartStopWithChild net6.0 8.83μs 49.3ns 327ns 0.0172 0.00861 0 7.47 KB
#5067 StartStopWithChild netcoreapp3.1 10.8μs 56.6ns 294ns 0.0371 0.0159 0 7.58 KB
#5067 StartStopWithChild net472 17.4μs 51.4ns 199ns 1.35 0.347 0.122 7.95 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 471μs 190ns 736ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 623μs 159ns 593ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 781μs 454ns 1.76μs 0.391 0 0 3.3 KB
#5067 WriteAndFlushEnrichedTraces net6.0 475μs 166ns 642ns 0 0 0 2.7 KB
#5067 WriteAndFlushEnrichedTraces netcoreapp3.1 670μs 159ns 615ns 0 0 0 2.7 KB
#5067 WriteAndFlushEnrichedTraces net472 795μs 320ns 1.24μs 0.396 0 0 3.3 KB
Benchmarks.Trace.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5067

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorMoreComplexBody‑net472 1.183 3,701.21 4,378.20

Faster 🎉 in #5067

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1 1.223 250.17 204.59

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 38.4μs 15.5ns 58ns 0.0193 0 0 1.77 KB
master AllCycleSimpleBody netcoreapp3.1 41.9μs 65.1ns 252ns 0.0209 0 0 1.74 KB
master AllCycleSimpleBody net472 45.4μs 40.7ns 158ns 0.271 0 0 1.81 KB
master AllCycleMoreComplexBody net6.0 199μs 37.9ns 137ns 0.0998 0 0 9.25 KB
master AllCycleMoreComplexBody netcoreapp3.1 212μs 371ns 1.44μs 0.105 0 0 9.14 KB
master AllCycleMoreComplexBody net472 226μs 135ns 525ns 1.46 0 0 9.32 KB
master ObjectExtractorSimpleBody net6.0 139ns 0.189ns 0.731ns 0.00394 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 250ns 0.192ns 0.742ns 0.00362 0 0 272 B
master ObjectExtractorSimpleBody net472 169ns 0.244ns 0.944ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.02μs 1.62ns 6.26ns 0.0538 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 4.02μs 2.09ns 7.81ns 0.0482 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.7μs 1.23ns 4.42ns 0.603 0.00553 0 3.8 KB
#5067 AllCycleSimpleBody net6.0 38.2μs 20.8ns 80.7ns 0.0192 0 0 1.77 KB
#5067 AllCycleSimpleBody netcoreapp3.1 41.4μs 34.4ns 129ns 0.0208 0 0 1.74 KB
#5067 AllCycleSimpleBody net472 44.4μs 17.5ns 68ns 0.288 0 0 1.81 KB
#5067 AllCycleMoreComplexBody net6.0 204μs 117ns 452ns 0.101 0 0 9.25 KB
#5067 AllCycleMoreComplexBody netcoreapp3.1 212μs 212ns 821ns 0.106 0 0 9.14 KB
#5067 AllCycleMoreComplexBody net472 230μs 67.3ns 260ns 1.37 0 0 9.32 KB
#5067 ObjectExtractorSimpleBody net6.0 140ns 0.117ns 0.422ns 0.00394 0 0 280 B
#5067 ObjectExtractorSimpleBody netcoreapp3.1 205ns 0.0956ns 0.358ns 0.00371 0 0 272 B
#5067 ObjectExtractorSimpleBody net472 166ns 0.107ns 0.415ns 0.0446 0 0 281 B
#5067 ObjectExtractorMoreComplexBody net6.0 3.05μs 1.4ns 5.04ns 0.0532 0 0 3.78 KB
#5067 ObjectExtractorMoreComplexBody netcoreapp3.1 4.05μs 1.77ns 6.61ns 0.0505 0 0 3.69 KB
#5067 ObjectExtractorMoreComplexBody net472 4.38μs 2.28ns 8.52ns 0.602 0.00654 0 3.8 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 RunWaf(args=NestedMap (10)) net6.0 57.1μs 327ns 2.45μs 0.204 0 0 16.06 KB
master RunWaf(args=NestedMap (10)) netcoreapp3.1 70.4μs 401ns 2.8μs 0.211 0 0 16.06 KB
master RunWaf(args=NestedMap (10)) net472 101μs 49.8ns 186ns 2.56 0.0966 0 16.14 KB
master RunWafTwice(args=NestedMap (10)) net6.0 57.3μs 145ns 666ns 0.231 0 0 16.6 KB
master RunWafTwice(args=NestedMap (10)) netcoreapp3.1 73.9μs 349ns 1.44μs 0.192 0 0 16.58 KB
master RunWafTwice(args=NestedMap (10)) net472 108μs 69.4ns 250ns 2.63 0.103 0 16.69 KB
master RunWafWithAttack(args=Neste(...)tack) [22]) net6.0 115μs 59.9ns 232ns 0.325 0 0 22.41 KB
master RunWafWithAttack(args=Neste(...)tack) [22]) netcoreapp3.1 129μs 153ns 551ns 0.258 0 0 22.36 KB
master RunWafWithAttack(args=Neste(...)tack) [22]) net472 161μs 824ns 3.69μs 3.54 0.157 0 22.7 KB
master RunWaf(args=NestedMap (100)) net6.0 101μs 28.3ns 110ns 0.453 0 0 32.76 KB
master RunWaf(args=NestedMap (100)) netcoreapp3.1 135μs 451ns 1.75μs 0.408 0 0 33.33 KB
master RunWaf(args=NestedMap (100)) net472 185μs 81.6ns 294ns 5.29 0.371 0 33.67 KB
master RunWafTwice(args=NestedMap (100)) net6.0 105μs 29.2ns 113ns 0.473 0 0 33.3 KB
master RunWafTwice(args=NestedMap (100)) netcoreapp3.1 140μs 769ns 4.68μs 0.457 0 0 33.86 KB
master RunWafTwice(args=NestedMap (100)) net472 196μs 173ns 670ns 5.41 0.386 0 34.23 KB
master RunWafWithAttack(args=Neste(...)tack) [23]) net6.0 166μs 62ns 223ns 0.495 0 0 39.1 KB
master RunWafWithAttack(args=Neste(...)tack) [23]) netcoreapp3.1 201μs 438ns 1.7μs 0.484 0 0 39.63 KB
master RunWafWithAttack(args=Neste(...)tack) [23]) net472 256μs 1.21μs 4.68μs 6.29 0.503 0 40.23 KB
master RunWaf(args=NestedMap (20)) net6.0 109μs 319ns 1.24μs 0.46 0 0 32.18 KB
master RunWaf(args=NestedMap (20)) netcoreapp3.1 138μs 198ns 768ns 0.41 0 0 32.3 KB
master RunWaf(args=NestedMap (20)) net472 185μs 53.4ns 200ns 5.15 0.368 0 32.63 KB
master RunWafTwice(args=NestedMap (20)) net6.0 101μs 332ns 1.29μs 0.464 0 0 32.72 KB
master RunWafTwice(args=NestedMap (20)) netcoreapp3.1 137μs 746ns 4.02μs 0.39 0 0 32.82 KB
master RunWafTwice(args=NestedMap (20)) net472 194μs 234ns 906ns 5.22 0.387 0 33.19 KB
master RunWafWithAttack(args=Neste(...)tack) [22]) net6.0 161μs 35.4ns 132ns 0.482 0 0 38.53 KB
master RunWafWithAttack(args=Neste(...)tack) [22]) netcoreapp3.1 195μs 67.8ns 244ns 0.516 0 0 38.6 KB
master RunWafWithAttack(args=Neste(...)tack) [22]) net472 254μs 197ns 762ns 6.21 0.507 0 39.2 KB
#5067 RunWaf(args=NestedMap (10)) net6.0 57.1μs 323ns 2.3μs 0.212 0 0 16.06 KB
#5067 RunWaf(args=NestedMap (10)) netcoreapp3.1 71.1μs 365ns 1.9μs 0.208 0 0 16.06 KB
#5067 RunWaf(args=NestedMap (10)) net472 97.4μs 482ns 2.05μs 2.54 0.0957 0 16.14 KB
#5067 RunWafTwice(args=NestedMap (10)) net6.0 61.3μs 124ns 464ns 0.229 0 0 16.6 KB
#5067 RunWafTwice(args=NestedMap (10)) netcoreapp3.1 75.5μs 239ns 924ns 0.219 0 0 16.58 KB
#5067 RunWafTwice(args=NestedMap (10)) net472 103μs 87.3ns 338ns 2.62 0.103 0 16.69 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [22]) net6.0 114μs 63.5ns 246ns 0.322 0 0 22.41 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [22]) netcoreapp3.1 130μs 524ns 2.03μs 0.252 0 0 22.36 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [22]) net472 160μs 303ns 1.17μs 3.56 0.158 0 22.7 KB
#5067 RunWaf(args=NestedMap (100)) net6.0 108μs 567ns 3μs 0.443 0 0 32.76 KB
#5067 RunWaf(args=NestedMap (100)) netcoreapp3.1 135μs 754ns 4.83μs 0.391 0 0 33.33 KB
#5067 RunWaf(args=NestedMap (100)) net472 185μs 136ns 528ns 5.27 0.37 0 33.67 KB
#5067 RunWafTwice(args=NestedMap (100)) net6.0 104μs 60.6ns 219ns 0.471 0 0 33.3 KB
#5067 RunWafTwice(args=NestedMap (100)) netcoreapp3.1 139μs 761ns 4.75μs 0.451 0 0 33.86 KB
#5067 RunWafTwice(args=NestedMap (100)) net472 195μs 174ns 674ns 5.35 0.389 0 34.22 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [23]) net6.0 165μs 830ns 3.71μs 0.554 0 0 39.1 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [23]) netcoreapp3.1 202μs 361ns 1.35μs 0.502 0 0 39.63 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [23]) net472 252μs 131ns 509ns 6.31 0.505 0 40.23 KB
#5067 RunWaf(args=NestedMap (20)) net6.0 110μs 40.7ns 158ns 0.448 0 0 32.18 KB
#5067 RunWaf(args=NestedMap (20)) netcoreapp3.1 130μs 53.2ns 192ns 0.417 0 0 32.3 KB
#5067 RunWaf(args=NestedMap (20)) net472 184μs 83.3ns 322ns 5.14 0.367 0 32.63 KB
#5067 RunWafTwice(args=NestedMap (20)) net6.0 110μs 125ns 451ns 0.441 0 0 32.72 KB
#5067 RunWafTwice(args=NestedMap (20)) netcoreapp3.1 138μs 329ns 1.27μs 0.391 0 0 32.82 KB
#5067 RunWafTwice(args=NestedMap (20)) net472 196μs 134ns 518ns 5.27 0.391 0 33.19 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [22]) net6.0 167μs 80.1ns 310ns 0.505 0 0 38.53 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [22]) netcoreapp3.1 201μs 763ns 2.95μs 0.488 0 0 38.6 KB
#5067 RunWafWithAttack(args=Neste(...)tack) [22]) net472 258μs 1.05μs 4.08μs 6.14 0.501 0 39.2 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 172μs 258ns 966ns 0.259 0 0 18.25 KB
master SendRequest netcoreapp3.1 194μs 246ns 951ns 0.195 0 0 20.41 KB
master SendRequest net472 0.000163ns 0.000126ns 0.000473ns 0 0 0 0 b
#5067 SendRequest net6.0 173μs 131ns 490ns 0.259 0 0 18.25 KB
#5067 SendRequest netcoreapp3.1 193μs 477ns 1.85μs 0.19 0 0 20.42 KB
#5067 SendRequest net472 0.000122ns 7.97E‑05ns 0.000309ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #5067

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1 41.63 KB 41.88 KB 245 B 0.59%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 555μs 630ns 2.44μs 0.543 0 0 41.79 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 652μs 866ns 3.35μs 0.332 0 0 41.63 KB
master WriteAndFlushEnrichedTraces net472 829μs 3.3μs 12.8μs 8.28 2.48 0.414 53.22 KB
#5067 WriteAndFlushEnrichedTraces net6.0 551μs 174ns 652ns 0.568 0 0 41.59 KB
#5067 WriteAndFlushEnrichedTraces netcoreapp3.1 655μs 324ns 1.21μs 0.331 0 0 41.88 KB
#5067 WriteAndFlushEnrichedTraces net472 844μs 3.52μs 13.6μs 8.28 2.48 0.414 53.24 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.09μs 0.801ns 3.1ns 0.0104 0 0 768 B
master ExecuteNonQuery netcoreapp3.1 1.46μs 0.901ns 3.49ns 0.0103 0 0 768 B
master ExecuteNonQuery net472 1.76μs 1.85ns 7.17ns 0.116 0 0 730 B
#5067 ExecuteNonQuery net6.0 1.15μs 0.501ns 1.94ns 0.0104 0 0 768 B
#5067 ExecuteNonQuery netcoreapp3.1 1.49μs 0.538ns 2.01ns 0.00968 0 0 768 B
#5067 ExecuteNonQuery net472 1.78μs 0.867ns 3.36ns 0.116 0 0 730 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.27μs 1.65ns 5.71ns 0.0129 0 0 936 B
master CallElasticsearch netcoreapp3.1 1.46μs 0.882ns 3.06ns 0.0126 0 0 936 B
master CallElasticsearch net472 2.54μs 1.38ns 5.36ns 0.152 0 0 955 B
master CallElasticsearchAsync net6.0 1.25μs 0.344ns 1.29ns 0.0125 0 0 912 B
master CallElasticsearchAsync netcoreapp3.1 1.62μs 0.739ns 2.77ns 0.013 0 0 984 B
master CallElasticsearchAsync net472 2.68μs 1.35ns 5.22ns 0.16 0 0 1.01 KB
#5067 CallElasticsearch net6.0 1.2μs 0.545ns 2.04ns 0.0133 0 0 936 B
#5067 CallElasticsearch netcoreapp3.1 1.56μs 0.517ns 1.94ns 0.0125 0 0 936 B
#5067 CallElasticsearch net472 2.61μs 0.778ns 2.81ns 0.151 0 0 955 B
#5067 CallElasticsearchAsync net6.0 1.38μs 0.341ns 1.32ns 0.0124 0 0 912 B
#5067 CallElasticsearchAsync netcoreapp3.1 1.62μs 0.707ns 2.65ns 0.0129 0 0 984 B
#5067 CallElasticsearchAsync net472 2.71μs 1.01ns 3.9ns 0.16 0 0 1.01 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.37μs 0.456ns 1.71ns 0.0125 0 0 912 B
master ExecuteAsync netcoreapp3.1 1.61μs 1.05ns 3.94ns 0.012 0 0 912 B
master ExecuteAsync net472 1.81μs 1.12ns 4.32ns 0.138 0 0 875 B
#5067 ExecuteAsync net6.0 1.33μs 0.671ns 2.6ns 0.0126 0 0 912 B
#5067 ExecuteAsync netcoreapp3.1 1.68μs 0.807ns 3.02ns 0.0117 0 0 912 B
#5067 ExecuteAsync net472 1.84μs 1.2ns 4.65ns 0.138 0 0 875 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.21μs 1.2ns 4.51ns 0.0294 0 0 2.1 KB
master SendAsync netcoreapp3.1 5.07μs 25.6ns 114ns 0.0347 0 0 2.63 KB
master SendAsync net472 7.76μs 2.81ns 10.1ns 0.523 0 0 3.31 KB
#5067 SendAsync net6.0 4.17μs 1.95ns 7.56ns 0.0291 0 0 2.1 KB
#5067 SendAsync netcoreapp3.1 4.91μs 1.92ns 6.94ns 0.0343 0 0 2.63 KB
#5067 SendAsync net472 7.82μs 4.39ns 17ns 0.522 0 0 3.31 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Faster 🎉 Fewer allocations 🎉

Faster 🎉 in #5067

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net6.0 1.193 61,300.00 51,400.00 bimodal

Fewer allocations 🎉 in #5067

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 213.75 KB 203.58 KB -10.18 KB -4.76%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 212.7 KB 202.06 KB -10.63 KB -5.00%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 62.24 KB 58.08 KB -4.16 KB -6.68%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 62.9μs 942ns 9.13μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 53.3μs 111ns 673ns 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.7μs 73.8ns 266ns 0 0 0 62.24 KB
master StringConcatAspectBenchmark net6.0 277μs 7.32μs 70.5μs 0 0 0 213.75 KB
master StringConcatAspectBenchmark netcoreapp3.1 289μs 2.76μs 25μs 0 0 0 212.7 KB
master StringConcatAspectBenchmark net472 253μs 4.7μs 45.3μs 0 0 0 221.18 KB
#5067 StringConcatBenchmark net6.0 51.5μs 136ns 510ns 0 0 0 43.44 KB
#5067 StringConcatBenchmark netcoreapp3.1 52.9μs 251ns 974ns 0 0 0 42.64 KB
#5067 StringConcatBenchmark net472 37.9μs 139ns 519ns 0 0 0 58.08 KB
#5067 StringConcatAspectBenchmark net6.0 245μs 6.25μs 59μs 0 0 0 203.58 KB
#5067 StringConcatAspectBenchmark netcoreapp3.1 287μs 2.54μs 22.9μs 0 0 0 202.06 KB
#5067 StringConcatAspectBenchmark net472 240μs 3.23μs 31.5μs 0 0 0 221.18 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.6μs 1.02ns 3.81ns 0.0221 0 0 1.57 KB
master EnrichedLog netcoreapp3.1 2.28μs 2.97ns 10.7ns 0.0205 0 0 1.57 KB
master EnrichedLog net472 2.62μs 2.09ns 8.08ns 0.237 0 0 1.5 KB
#5067 EnrichedLog net6.0 1.61μs 1.39ns 5.21ns 0.0223 0 0 1.57 KB
#5067 EnrichedLog netcoreapp3.1 2.3μs 3.71ns 13.9ns 0.0202 0 0 1.57 KB
#5067 EnrichedLog net472 2.66μs 2.15ns 7.77ns 0.237 0 0 1.5 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 113μs 96.1ns 372ns 0.056 0 0 4.21 KB
master EnrichedLog netcoreapp3.1 118μs 90.4ns 338ns 0.0589 0 0 4.21 KB
master EnrichedLog net472 150μs 173ns 646ns 0.672 0.224 0 4.39 KB
#5067 EnrichedLog net6.0 115μs 103ns 401ns 0.0574 0 0 4.21 KB
#5067 EnrichedLog netcoreapp3.1 117μs 73.3ns 284ns 0 0 0 4.21 KB
#5067 EnrichedLog net472 150μs 198ns 767ns 0.669 0.223 0 4.39 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 2.94μs 1.18ns 4.41ns 0.0299 0 0 2.13 KB
master EnrichedLog netcoreapp3.1 4.11μs 1.32ns 5.12ns 0.0287 0 0 2.13 KB
master EnrichedLog net472 4.84μs 1.9ns 7.37ns 0.307 0 0 1.95 KB
#5067 EnrichedLog net6.0 3.03μs 0.703ns 2.63ns 0.0303 0 0 2.13 KB
#5067 EnrichedLog netcoreapp3.1 4.56μs 1.37ns 5.32ns 0.0292 0 0 2.13 KB
#5067 EnrichedLog net472 4.89μs 1.16ns 4.51ns 0.308 0 0 1.95 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.38μs 0.303ns 1.17ns 0.0153 0 0 1.1 KB
master SendReceive netcoreapp3.1 1.87μs 0.788ns 2.95ns 0.0149 0 0 1.1 KB
master SendReceive net472 2.25μs 3.39ns 13.1ns 0.176 0 0 1.12 KB
#5067 SendReceive net6.0 1.38μs 1.15ns 4.47ns 0.0153 0 0 1.1 KB
#5067 SendReceive netcoreapp3.1 1.79μs 1.25ns 4.85ns 0.0144 0 0 1.1 KB
#5067 SendReceive net472 2.12μs 1.85ns 7.16ns 0.176 0 0 1.12 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.81μs 0.669ns 2.59ns 0.0213 0 0 1.53 KB
master EnrichedLog netcoreapp3.1 3.89μs 1.07ns 4.13ns 0.0215 0 0 1.58 KB
master EnrichedLog net472 4.19μs 2.12ns 8.2ns 0.312 0 0 1.97 KB
#5067 EnrichedLog net6.0 2.8μs 1.67ns 6.04ns 0.021 0 0 1.53 KB
#5067 EnrichedLog netcoreapp3.1 4.04μs 1.59ns 5.52ns 0.0203 0 0 1.58 KB
#5067 EnrichedLog net472 4.3μs 1.36ns 5.28ns 0.311 0 0 1.97 KB
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5067

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 1.137 510.64 448.93

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 511ns 0.229ns 0.858ns 0.00746 0 0 536 B
master StartFinishSpan netcoreapp3.1 711ns 1.79ns 6.95ns 0.00723 0 0 536 B
master StartFinishSpan net472 795ns 0.283ns 1.09ns 0.0851 0 0 538 B
master StartFinishScope net6.0 600ns 0.192ns 0.745ns 0.00922 0 0 656 B
master StartFinishScope netcoreapp3.1 833ns 0.785ns 3.04ns 0.00893 0 0 656 B
master StartFinishScope net472 947ns 0.5ns 1.94ns 0.0979 0 0 618 B
#5067 StartFinishSpan net6.0 449ns 0.1ns 0.376ns 0.00747 0 0 536 B
#5067 StartFinishSpan netcoreapp3.1 734ns 0.466ns 1.8ns 0.00702 0 0 536 B
#5067 StartFinishSpan net472 782ns 0.336ns 1.3ns 0.0854 0 0 538 B
#5067 StartFinishScope net6.0 634ns 0.187ns 0.723ns 0.00921 0 0 656 B
#5067 StartFinishScope netcoreapp3.1 907ns 4.53ns 20.2ns 0.00856 0 0 656 B
#5067 StartFinishScope net472 969ns 0.446ns 1.73ns 0.098 0 0 618 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 718ns 0.318ns 1.23ns 0.00902 0 0 656 B
master RunOnMethodBegin netcoreapp3.1 981ns 3.83ns 14.8ns 0.00877 0 0 656 B
master RunOnMethodBegin net472 1.08μs 0.287ns 1.11ns 0.098 0 0 618 B
#5067 RunOnMethodBegin net6.0 708ns 0.182ns 0.704ns 0.00894 0 0 656 B
#5067 RunOnMethodBegin netcoreapp3.1 1.02μs 1.95ns 7.04ns 0.00861 0 0 656 B
#5067 RunOnMethodBegin net472 1.16μs 0.568ns 2.2ns 0.0976 0 0 618 B

@andrewlock
Copy link
Member

andrewlock commented Jan 17, 2024 •
edited
Loading

Throughput/Crank Report:zap:

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 (5067) (11.283M)   : 0, 11282842
    master (11.045M)   : 0, 11045166
    benchmarks/2.9.0 (11.021M)   : 0, 11020861

    section Automatic
    This PR (5067) (7.685M)   : 0, 7684993
    master (7.612M)   : 0, 7612198
    benchmarks/2.9.0 (8.031M)   : 0, 8030841

    section Trace stats
    This PR (5067) (8.026M)   : 0, 8025929
    master (7.923M)   : 0, 7922699

    section Manual
    This PR (5067) (9.869M)   : 0, 9868918
    master (9.633M)   : 0, 9633067

    section Manual + Automatic
    This PR (5067) (7.224M)   : 0, 7224096
    master (7.227M)   : 0, 7226532

    section Version Conflict
    This PR (5067) (6.540M)   : 0, 6540367
    master (6.546M)   : 0, 6546261
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5067) (9.533M)   : 0, 9532851
    master (9.493M)   : 0, 9492934
    benchmarks/2.9.0 (9.571M)   : 0, 9570999

    section Automatic
    This PR (5067) (6.586M)   : 0, 6586079
    master (6.558M)   : 0, 6557890

    section Trace stats
    This PR (5067) (6.990M)   : 0, 6990113
    master (6.898M)   : 0, 6898082

    section Manual
    This PR (5067) (8.304M)   : 0, 8303935
    master (8.234M)   : 0, 8234237

    section Manual + Automatic
    This PR (5067) (6.180M)   : 0, 6179688
    master (6.273M)   : 0, 6272784

    section Version Conflict
    This PR (5067) (5.524M)   : 0, 5524427
    master (5.625M)   : 0, 5624790
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5067) (10.160M)   : 0, 10160405
    master (10.105M)   : 0, 10104990
    benchmarks/2.9.0 (10.312M)   : 0, 10311545

    section Automatic
    This PR (5067) (7.136M)   : 0, 7136165
    master (6.962M)   : 0, 6961875
    benchmarks/2.9.0 (7.431M)   : 0, 7430755

    section Trace stats
    This PR (5067) (7.511M)   : 0, 7511121
    master (7.331M)   : 0, 7330813

    section Manual
    This PR (5067) (8.846M)   : 0, 8846131
    master (8.663M)   : 0, 8662830

    section Manual + Automatic
    This PR (5067) (6.942M)   : 0, 6942150
    master (6.764M)   : 0, 6764197

    section Version Conflict
    This PR (5067) (6.141M)   : 0, 6141359
    master (6.002M)   : 0, 6001872
Loading 
gantt
    title Throughput Linux x64 (ASM) (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5067) (7.345M)   : 0, 7345328
    master (7.358M)   : 0, 7357726
    benchmarks/2.9.0 (7.826M)   : 0, 7825728

    section No attack
    This PR (5067) (1.753M)   : 0, 1753362
    master (1.768M)   : 0, 1767522
    benchmarks/2.9.0 (3.204M)   : 0, 3203842

    section Attack
    This PR (5067) (1.406M)   : 0, 1405869
    master (1.404M)   : 0, 1404365
    benchmarks/2.9.0 (2.541M)   : 0, 2541039

    section Blocking
    This PR (5067) (3.122M)   : 0, 3122052
    master (3.151M)   : 0, 3150668

    section IAST default
    This PR (5067) (6.469M)   : 0, 6468805
    master (6.544M)   : 0, 6543916

    section IAST full
    This PR (5067) (5.655M)   : 0, 5654674
    master (5.701M)   : 0, 5701059

    section Base vuln
    This PR (5067) (0.976M)   : 0, 976487
    master (0.952M)   : 0, 952021

    section IAST vuln
    This PR (5067) (0.850M)   : 0, 849788
    master (0.863M)   : 0, 862892
Loading

@NachoEchevarria NachoEchevarria force-pushed the nacho/StackTraceLeakVulnerability branch from e668bdd to d30388f Compare January 18, 2024 09:42
@NachoEchevarria NachoEchevarria marked this pull request as ready for review January 19, 2024 09:52
@NachoEchevarria NachoEchevarria requested review from a team as code owners January 19, 2024 09:52
robertpi
robertpi reviewed Jan 23, 2024
View reviewed changes
...rofiler/AutoInstrumentation/StackTraceLeak/DeveloperExceptionPageMiddlewareIntegrationBis.cs Outdated

[Browsable(false)]
[EditorBrowsable(EditorBrowsableState.Never)]
public static class DeveloperExceptionPageMiddlewareIntegrationBis
Copy link
Member

Choose a reason for hiding this comment

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

Seems a little strange to split this across two classes? As OnMethodBegin has a different signature in each case, I think they could go in the same class. Not that it really matters, can be left as is.

Copy link
Member

Choose a reason for hiding this comment

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

Personally I prefer to split like this, as it's clear which of the instrumentations are calling which method 🙂

What I'm not a fan of is the Bis suffix 😄 I have no idea what that means 😅 Personally I would call this one DeveloperExceptionPageMiddlewareIntegration and the earlier one DeveloperExceptionPageMiddlewareIntegration_Pre_3_0_0 to make it explicit, but it's just a nit 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that using another nomenclature would be more clear. Fixed.

robertpi
robertpi approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@robertpi robertpi left a comment

Choose a reason for hiding this comment

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

Nice simple solution, shouldn't add much overhead.

@NachoEchevarria NachoEchevarria force-pushed the nacho/StackTraceLeakVulnerability branch from bc39001 to d72ca50 Compare January 23, 2024 09:47
andrewlock
andrewlock approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@andrewlock andrewlock left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of minor suggestions - the duck typing convention is the main one

...lrProfiler/AutoInstrumentation/StackTraceLeak/DeveloperExceptionPageMiddlewareIntegration.cs Outdated Show resolved Hide resolved
...rofiler/AutoInstrumentation/StackTraceLeak/DeveloperExceptionPageMiddlewareIntegrationBis.cs Outdated

[Browsable(false)]
[EditorBrowsable(EditorBrowsableState.Never)]
public static class DeveloperExceptionPageMiddlewareIntegrationBis
Copy link
Member

Choose a reason for hiding this comment

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

Personally I prefer to split like this, as it's clear which of the instrumentations are calling which method 🙂

What I'm not a fan of is the Bis suffix 😄 I have no idea what that means 😅 Personally I would call this one DeveloperExceptionPageMiddlewareIntegration and the earlier one DeveloperExceptionPageMiddlewareIntegration_Pre_3_0_0 to make it explicit, but it's just a nit 🙂

...rofiler/AutoInstrumentation/StackTraceLeak/DeveloperExceptionPageMiddlewareIntegrationBis.cs Outdated Show resolved Hide resolved
...rofiler/AutoInstrumentation/StackTraceLeak/DeveloperExceptionPageMiddlewareIntegrationBis.cs Outdated
Comment on lines 60 to 65
[DuckCopy]
internal struct ErrorContextStruct
{
[Duck(BindingFlags = DuckAttribute.DefaultFlags | BindingFlags.IgnoreCase)]
public Exception Exception;
}
Copy link
Member

Choose a reason for hiding this comment

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

We don't normally nest the duck types directly in the integrations 🤔 I think I like it 😄

...adog.Trace/ClrProfiler/AutoInstrumentation/StackTraceLeak/StackTraceLeakIntegrationCommon.cs

internal static CallTargetState OnExceptionLeak(IntegrationId integrationId, Exception exception)
{
if (!Tracer.Instance.Settings.IsIntegrationEnabled(integrationId))
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be checking that IAST is enabled too? 🤔 Or do we check that later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's checked later in IASTModule. Still, this piece of code should not be reached if IAST is disabled because of the instrumentation category.

Copy link
Member

Choose a reason for hiding this comment

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

this piece of code should not be reached if IAST is disabled because of the instrumentation category

Good point, I had forgotten that we don't even rejit these if IAST isn't enabled 🙂

.../src/Datadog.Trace/ClrProfiler/AutoInstrumentation/StackTraceLeak/HttpResponseIntegration.cs
Comment on lines +36 to +52
/// <summary>
/// OnMethodBegin callback
/// </summary>
/// <param name="instance">Instance value, aka `this` of the instrumented method.</param>
/// <param name="exception">The exception to be shown.</param>
/// <param name="dontShowSensitiveErrors">The dontShowSensitiveErrors parameter of WriteErrorMessage.</param>
/// <typeparam name="TTarget">Type of the target</typeparam>
/// <returns>Calltarget state value</returns>
internal static CallTargetState OnMethodBegin<TTarget>(TTarget instance, Exception exception, bool dontShowSensitiveErrors)
{
if (HttpRuntime.UsingIntegratedPipeline && !dontShowSensitiveErrors)
{
return StackTraceLeakIntegrationCommon.OnExceptionLeak(IntegrationId.StackTraceLeak, exception);
}

return CallTargetState.GetDefault();
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether UsingIntegrationPipeline is enough here 🤔 In HttpContextHelper.AddHeaderTagsFromHttpResponse() (and other places) we have to add an additional try-catch, because we still run into issues accessing the HttpContext. My gut feeling is that we don't need to do the following, because I don't think you're trying to access the response headers anyway (inside IastModule for example).

Just dropping it here for context, just in case you think it could be an issue 🙂

Suggested change
/// <summary>
/// OnMethodBegin callback
/// </summary>
/// <param name="instance">Instance value, aka `this` of the instrumented method.</param>
/// <param name="exception">The exception to be shown.</param>
/// <param name="dontShowSensitiveErrors">The dontShowSensitiveErrors parameter of WriteErrorMessage.</param>
/// <typeparam name="TTarget">Type of the target</typeparam>
/// <returns>Calltarget state value</returns>
internal static CallTargetState OnMethodBegin<TTarget>(TTarget instance, Exception exception, bool dontShowSensitiveErrors)
{
if (HttpRuntime.UsingIntegratedPipeline && !dontShowSensitiveErrors)
{
return StackTraceLeakIntegrationCommon.OnExceptionLeak(IntegrationId.StackTraceLeak, exception);
}
return CallTargetState.GetDefault();
}
private static readonly IDatadogLogger Log = DatadogLogging.GetLoggerFor(typeof(HttpResponseIntegration));
private static bool _canReadHttpResponseHeaders = true;
internal static CallTargetState OnMethodBegin<TTarget>(TTarget instance, Exception exception, bool dontShowSensitiveErrors)
{
try
{
if (_canReadHttpResponseHeaders && HttpRuntime.UsingIntegratedPipeline && !dontShowSensitiveErrors)
{
return StackTraceLeakIntegrationCommon.OnExceptionLeak(IntegrationId.StackTraceLeak, exception);
}
}
catch (PlatformNotSupportedException ex)
{
// Despite the HttpRuntime.UsingIntegratedPipeline check, we can still fail to access response headers, for example when using Sitefinity: "This operation requires IIS integrated pipeline mode"
Log.Error(ex, "Unable to access response headers performing StackLeak detection. Disabling for the rest of the application lifetime.");
_canReadHttpResponseHeaders = false;
}
return CallTargetState.GetDefault();
}

tracer/src/Datadog.Trace/ClrProfiler/ClrNames.cs
@@ -48,6 +48,7 @@ internal static class ClrNames
public const string Int32Task = "System.Threading.Tasks.Task`1[System.Int32]";

public const string Type = "System.Type";
public const string Exception = "System.Exception";
Copy link
Member

Choose a reason for hiding this comment

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

How do we not have this already 😂

tracer/test/Datadog.Trace.Security.IntegrationTests/IAST/AspNetCore2IastTests.cs
@@ -88,6 +88,7 @@ public class AspNetCore2IastTestsFullSamplingEnabled : AspNetCore2IastTestsFullS
public AspNetCore2IastTestsFullSamplingEnabled(AspNetCoreTestFixture fixture, ITestOutputHelper outputHelper)
: base(fixture, outputHelper, enableIast: true, testName: "AspNetCore2IastTestsEnabled", isIastDeduplicationEnabled: false, vulnerabilitiesPerRequest: 200)
{
SetEnvironmentVariable("ASPNETCORE_ENVIRONMENT", "Development");
Copy link
Member

Choose a reason for hiding this comment

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

Interesting 🤔 Totally understand why you need this, but also, you may end up with issues later for APIs that need it not to be this.

Also, it might be nice to have a test where you don't send this, to confirm there is no stack-leak vulnerability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that if in the future we have apis in our tests that don't need that, we could have two different test classes, one with each value. I did not add tests without the developer mode for CI performance, but I can add one just to make sure.

tracer/test/snapshots/Iast.StackTraceLeak.AspNetCore2.verified.txt Show resolved Hide resolved
@NachoEchevarria NachoEchevarria merged commit a00c0fa into master Jan 24, 2024
57 checks passed
@NachoEchevarria NachoEchevarria deleted the nacho/StackTraceLeakVulnerability branch January 24, 2024 11:51
@NachoEchevarria
Copy link
Contributor Author

Thank you for your feedback and reviews!

@github-actions github-actions bot added this to the vNext milestone Jan 24, 2024
@andrewlock andrewlock mentioned this pull request Feb 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewlock andrewlock andrewlock approved these changes

@robertpi robertpi robertpi approved these changes

Assignees
No one assigned
Labels
area:asm area:asm-iast
Projects
None yet
Milestone
2.47.0
Development

Successfully merging this pull request may close these issues.
3 participants
@NachoEchevarria @andrewlock @robertpi