Skip to content

[ASM] - Fix HttpRequestValidationException Error#5221

Merged
NachoEchevarria merged 8 commits into
masterfrom
nacho/IastQueryStringFix
Feb 29, 2024
Merged

[ASM] - Fix HttpRequestValidationException Error#5221
NachoEchevarria merged 8 commits into
masterfrom
nacho/IastQueryStringFix

Conversation

@NachoEchevarria

@NachoEchevarria NachoEchevarria commented Feb 21, 2024

Copy link
Copy Markdown
Collaborator

Summary of changes

The following PR is a fix for this exception:

System.Web.HttpRequestValidationException: Datadog ASP.NET HttpModule instrumentation error
System.Web.HttpRequestValidationException
at System.Web.HttpRequest.ValidateString(String value, String collectionKey, RequestValidationSource requestCollection)
at System.Web.HttpRequest.ValidateHttpValueCollection(HttpValueCollection collection, RequestValidationSource requestCollection)
at System.Web.HttpRequest.get_QueryString()
at Datadog.Trace.Iast.IastRequestContext.AddRequestData(HttpRequest request)
at Datadog.Trace.AspNet.TracingHttpModule.OnBeginRequest(Object sender, EventArgs eventArgs)

This is a net framework exception, is not thrown in netcore.

Reason for change

The exception was thrown and logged.

Implementation details

Test coverage

Other details

@github-actions github-actions Bot added the area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) label Feb 21, 2024
@datadog-ddstaging

datadog-ddstaging Bot commented Feb 21, 2024 •
edited
Loading

Copy link
Copy Markdown

Datadog Report

Branch report: nacho/IastQueryStringFix
Commit report: 8fe437e
Test service: dd-trace-dotnet

✅ 0 Failed, 306279 Passed, 1553 Skipped, 41m 21.64s Wall Time
❄️ 2 New Flaky

New Flaky Tests (2)

  • NoExceptions - Datadog.Trace.ClrProfiler.IntegrationTests.SmokeTests.StackExchangeRedisAssemblyConflictSdkProjectSmokeTest - Last Failure

    Expand for error 
     The smoke test is running for too long or was lost.

  • TelemetryControllerDumpsAllTelemetryToFile - Datadog.Trace.Tests.Telemetry.TelemetryControllerTests - Last Failure

    Expand for error 
     Expected products not to be <null>.

@andrewlock

andrewlock commented Feb 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 (5221) - mean (75ms)  : 67, 83
     .   : milestone, 75,
    master - mean (75ms)  : 64, 86
     .   : milestone, 75,

    section CallTarget+Inlining+NGEN
    This PR (5221) - mean (986ms)  : 959, 1013
     .   : milestone, 986,
    master - mean (983ms)  : 964, 1002
     .   : milestone, 983,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5221) - mean (111ms)  : 106, 117
     .   : milestone, 111,
    master - mean (111ms)  : 106, 115
     .   : milestone, 111,

    section CallTarget+Inlining+NGEN
    This PR (5221) - mean (716ms)  : 689, 743
     .   : milestone, 716,
    master - mean (716ms)  : 694, 738
     .   : milestone, 716,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5221) - mean (94ms)  : 91, 97
     .   : milestone, 94,
    master - mean (95ms)  : 91, 99
     .   : milestone, 95,

    section CallTarget+Inlining+NGEN
    This PR (5221) - mean (673ms)  : 644, 703
     .   : milestone, 673,
    master - mean (666ms)  : 648, 685
     .   : milestone, 666,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5221) - mean (188ms)  : 185, 192
     .   : milestone, 188,
    master - mean (188ms)  : 185, 191
     .   : milestone, 188,

    section CallTarget+Inlining+NGEN
    This PR (5221) - mean (1,064ms)  : 1039, 1089
     .   : milestone, 1064,
    master - mean (1,055ms)  : 1027, 1084
     .   : milestone, 1055,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5221) - mean (271ms)  : 267, 275
     .   : milestone, 271,
    master - mean (270ms)  : 263, 276
     .   : milestone, 270,

    section CallTarget+Inlining+NGEN
    This PR (5221) - mean (1,065ms)  : 1046, 1083
     .   : milestone, 1065,
    master - mean (1,056ms)  : 1028, 1084
     .   : milestone, 1056,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5221) - mean (260ms)  : 255, 264
     .   : milestone, 260,
    master - mean (259ms)  : 252, 266
     .   : milestone, 259,

    section CallTarget+Inlining+NGEN
    This PR (5221) - mean (1,002ms)  : 978, 1026
     .   : milestone, 1002,
    master - mean (996ms)  : 972, 1020
     .   : milestone, 996,
Loading

@NachoEchevarria NachoEchevarria changed the title IAST - Fix IAST - Fix HttpRequestValidationException Error Feb 21, 2024
@NachoEchevarria
NachoEchevarria marked this pull request as ready for review February 21, 2024 12:19
@NachoEchevarria
NachoEchevarria requested a review from a team as a code owner February 21, 2024 12:19
andrewlock
andrewlock reviewed Feb 21, 2024
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for flagging this, I didn't know that was a thing! Do you know what the querystring was that triggered this?

Given that this affects a lot more than just Iast, I think we should:

  • Extract this to a helper method
  • Update all the places we call QueryString in .NET FX to use it (tracing/ASM etc)
  • (If possible) Add a unit test for the faulty querystring, mostly to confirm that this is definitely where the exception is thrown (i.e. not on the call to AllKeys or ToString or anything)

Comment thread tracer/src/Datadog.Trace/Iast/IastRequestContext.cs Outdated
Comment on lines +209 to +217
try
{
// we can get an exception if the query string fails validation
queryString = request.QueryString;
}
catch (HttpRequestValidationException)
{
Log.Debug("Error reading request QueryString.");
}

@andrewlock andrewlock Feb 21, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yikes, really?! We should probably extract this to a helper method and make sure we use it everywhere, include in APM too!🙂

@NachoEchevarria NachoEchevarria Feb 21, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@NachoEchevarria

NachoEchevarria commented Feb 21, 2024

Copy link
Copy Markdown
Collaborator Author

Thanks for flagging this, I didn't know that was a thing! Do you know what the querystring was that triggered this?

Given that this affects a lot more than just Iast, I think we should:

  • Extract this to a helper method
  • Update all the places we call QueryString in .NET FX to use it (tracing/ASM etc)
  • (If possible) Add a unit test for the faulty querystring, mostly to confirm that this is definitely where the exception is thrown (i.e. not on the call to AllKeys or ToString or anything)

I don't have the specific querystring, but the validation ends up in a method named CrossSiteScriptingValidation.IsDangerousString(string s, out int matchIndex) The code seems to be a protection against XSS. I will try to reproduce it in a unit test.

@NachoEchevarria
NachoEchevarria requested a review from a team as a code owner February 21, 2024 16:40
@andrewlock

andrewlock commented Feb 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #5221 compared to master:

  • 3 benchmarks are faster, with geometric mean 1.138
  • 2 benchmarks are slower, with geometric mean 1.179
  • 2 benchmarks have fewer allocations
  • 4 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.64μs 48.7ns 316ns 0.0221 0.00883 0 7.49 KB
master StartStopWithChild netcoreapp3.1 10.7μs 55.8ns 290ns 0.0257 0.0103 0 7.58 KB
master StartStopWithChild net472 17.3μs 60.2ns 233ns 1.34 0.353 0.129 7.97 KB
#5221 StartStopWithChild net6.0 8.65μs 44.8ns 283ns 0.0217 0.00866 0 7.49 KB
#5221 StartStopWithChild netcoreapp3.1 10.6μs 54.3ns 317ns 0.0257 0.0103 0 7.59 KB
#5221 StartStopWithChild net472 16.8μs 61.8ns 239ns 1.35 0.355 0.124 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 446μs 396ns 1.43μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 619μs 415ns 1.61μs 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 812μs 187ns 675ns 0.406 0 0 3.3 KB
#5221 WriteAndFlushEnrichedTraces net6.0 477μs 354ns 1.33μs 0 0 0 2.7 KB
#5221 WriteAndFlushEnrichedTraces netcoreapp3.1 632μs 338ns 1.26μs 0 0 0 2.7 KB
#5221 WriteAndFlushEnrichedTraces net472 802μs 273ns 1.06μs 0.401 0 0 3.3 KB
Benchmarks.Trace.Asm.AppSecBodyBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5221

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1 1.201 205.56 246.95

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net6.0 40.6μs 23.9ns 89.5ns 0.0201 0 0 2.36 KB
master AllCycleSimpleBody netcoreapp3.1 43.3μs 51.4ns 192ns 0.0216 0 0 2.34 KB
master AllCycleSimpleBody net472 47.2μs 31.5ns 114ns 0.377 0 0 2.41 KB
master AllCycleMoreComplexBody net6.0 214μs 184ns 689ns 0.107 0 0 9.84 KB
master AllCycleMoreComplexBody netcoreapp3.1 228μs 74.9ns 260ns 0.113 0 0 9.73 KB
master AllCycleMoreComplexBody net472 243μs 113ns 424ns 1.57 0 0 9.91 KB
master ObjectExtractorSimpleBody net6.0 156ns 0.0947ns 0.367ns 0.00394 0 0 280 B
master ObjectExtractorSimpleBody netcoreapp3.1 206ns 0.211ns 0.819ns 0.00372 0 0 272 B
master ObjectExtractorSimpleBody net472 179ns 0.0672ns 0.242ns 0.0446 0 0 281 B
master ObjectExtractorMoreComplexBody net6.0 3.06μs 1.14ns 4.13ns 0.0535 0 0 3.78 KB
master ObjectExtractorMoreComplexBody netcoreapp3.1 4.02μs 2.93ns 11.4ns 0.0504 0 0 3.69 KB
master ObjectExtractorMoreComplexBody net472 3.81μs 3.01ns 11.6ns 0.601 0.00576 0 3.8 KB
#5221 AllCycleSimpleBody net6.0 41μs 19.3ns 72.3ns 0.0203 0 0 2.36 KB
#5221 AllCycleSimpleBody netcoreapp3.1 44.1μs 55.8ns 216ns 0.0223 0 0 2.34 KB
#5221 AllCycleSimpleBody net472 48μs 20.7ns 80.3ns 0.382 0 0 2.41 KB
#5221 AllCycleMoreComplexBody net6.0 216μs 341ns 1.32μs 0.107 0 0 9.84 KB
#5221 AllCycleMoreComplexBody netcoreapp3.1 228μs 217ns 812ns 0.113 0 0 9.73 KB
#5221 AllCycleMoreComplexBody net472 242μs 83.5ns 323ns 1.57 0 0 9.91 KB
#5221 ObjectExtractorSimpleBody net6.0 141ns 0.0961ns 0.359ns 0.00394 0 0 280 B
#5221 ObjectExtractorSimpleBody netcoreapp3.1 247ns 0.219ns 0.791ns 0.00359 0 0 272 B
#5221 ObjectExtractorSimpleBody net472 171ns 0.0786ns 0.304ns 0.0446 0 0 281 B
#5221 ObjectExtractorMoreComplexBody net6.0 2.96μs 1.21ns 4.51ns 0.0532 0 0 3.78 KB
#5221 ObjectExtractorMoreComplexBody netcoreapp3.1 3.96μs 1.84ns 7.14ns 0.0495 0 0 3.69 KB
#5221 ObjectExtractorMoreComplexBody net472 3.8μs 3.19ns 12.4ns 0.603 0.00568 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 52.8μs 64.8ns 243ns 0.421 0 0 30.94 KB
master EncodeArgs netcoreapp3.1 68.9μs 102ns 381ns 0.412 0 0 31.47 KB
master EncodeArgs net472 84.4μs 37.9ns 147ns 5.1 0.0843 0 32.27 KB
master EncodeLegacyArgs net6.0 123μs 200ns 747ns 0.426 0 0 33.89 KB
master EncodeLegacyArgs netcoreapp3.1 157μs 479ns 1.86μs 0.462 0 0 34.09 KB
master EncodeLegacyArgs net472 211μs 93ns 348ns 5.48 0.422 0 35 KB
#5221 EncodeArgs net6.0 53.5μs 60ns 225ns 0.426 0 0 30.94 KB
#5221 EncodeArgs netcoreapp3.1 69.6μs 68.2ns 255ns 0.417 0 0 31.47 KB
#5221 EncodeArgs net472 85.2μs 52.2ns 202ns 5.12 0.0853 0 32.27 KB
#5221 EncodeLegacyArgs net6.0 122μs 260ns 973ns 0.474 0 0 33.89 KB
#5221 EncodeLegacyArgs netcoreapp3.1 156μs 297ns 1.07μs 0.466 0 0 34.09 KB
#5221 EncodeLegacyArgs net472 215μs 119ns 445ns 5.49 0.43 0 35 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 188μs 122ns 473ns 0.094 0 0 6.51 KB
master RunWafRealisticBenchmark netcoreapp3.1 205μs 377ns 1.41μs 0 0 0 6.49 KB
master RunWafRealisticBenchmark net472 224μs 255ns 987ns 0.994 0 0 6.59 KB
master RunWafRealisticBenchmarkWithAttack net6.0 124μs 68.8ns 267ns 0.0619 0 0 4.15 KB
master RunWafRealisticBenchmarkWithAttack netcoreapp3.1 134μs 211ns 817ns 0 0 0 4.14 KB
master RunWafRealisticBenchmarkWithAttack net472 145μs 62.1ns 240ns 0.653 0 0 4.19 KB
#5221 RunWafRealisticBenchmark net6.0 189μs 220ns 852ns 0.094 0 0 6.51 KB
#5221 RunWafRealisticBenchmark netcoreapp3.1 205μs 404ns 1.57μs 0 0 0 6.49 KB
#5221 RunWafRealisticBenchmark net472 223μs 94.9ns 355ns 0.999 0 0 6.59 KB
#5221 RunWafRealisticBenchmarkWithAttack net6.0 127μs 106ns 410ns 0.0624 0 0 4.15 KB
#5221 RunWafRealisticBenchmarkWithAttack netcoreapp3.1 135μs 173ns 669ns 0 0 0 4.14 KB
#5221 RunWafRealisticBenchmarkWithAttack net472 147μs 92.1ns 357ns 0.659 0 0 4.19 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 171μs 188ns 729ns 0.255 0 0 18.26 KB
master SendRequest netcoreapp3.1 193μs 300ns 1.16μs 0.192 0 0 20.42 KB
master SendRequest net472 0.000105ns 7.01E‑05ns 0.000272ns 0 0 0 0 b
#5221 SendRequest net6.0 172μs 234ns 905ns 0.256 0 0 18.26 KB
#5221 SendRequest netcoreapp3.1 194μs 295ns 1.14μs 0.193 0 0 20.42 KB
#5221 SendRequest net472 0.000933ns 0.000344ns 0.00124ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #5221

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑net6.0 41.38 KB 41.83 KB 451 B 1.09%

Fewer allocations 🎉 in #5221

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1 41.93 KB 41.57 KB -356 B -0.85%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 532μs 607ns 2.1μs 0.558 0 0 41.38 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 654μs 1.3μs 5.03μs 0.331 0 0 41.93 KB
master WriteAndFlushEnrichedTraces net472 847μs 4μs 17μs 8.33 2.5 0.417 53.23 KB
#5221 WriteAndFlushEnrichedTraces net6.0 563μs 935ns 3.62μs 0.561 0 0 41.83 KB
#5221 WriteAndFlushEnrichedTraces netcoreapp3.1 663μs 1.85μs 7.18μs 0.338 0 0 41.57 KB
#5221 WriteAndFlushEnrichedTraces net472 879μs 4.1μs 16.4μs 8.13 2.57 0.428 53.25 KB
Benchmarks.Trace.DbCommandBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #5221

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑net6.0 1.157 975.81 1,129.42

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 976ns 0.496ns 1.92ns 0.0107 0 0 776 B
master ExecuteNonQuery netcoreapp3.1 1.53μs 0.743ns 2.78ns 0.00997 0 0 776 B
master ExecuteNonQuery net472 1.75μs 1.03ns 3.57ns 0.117 0 0 738 B
#5221 ExecuteNonQuery net6.0 1.13μs 0.576ns 2.23ns 0.0105 0 0 776 B
#5221 ExecuteNonQuery netcoreapp3.1 1.49μs 0.687ns 2.57ns 0.0104 0 0 776 B
#5221 ExecuteNonQuery net472 1.7μs 0.42ns 1.63ns 0.116 0 0 738 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.22μs 0.754ns 2.82ns 0.0134 0 0 944 B
master CallElasticsearch netcoreapp3.1 1.52μs 0.641ns 2.4ns 0.0122 0 0 944 B
master CallElasticsearch net472 2.55μs 0.797ns 3.09ns 0.152 0 0 963 B
master CallElasticsearchAsync net6.0 1.4μs 0.853ns 3.19ns 0.0126 0 0 920 B
master CallElasticsearchAsync netcoreapp3.1 1.6μs 0.61ns 2.28ns 0.013 0 0 992 B
master CallElasticsearchAsync net472 2.62μs 1.94ns 7.25ns 0.161 0 0 1.02 KB
#5221 CallElasticsearch net6.0 1.21μs 0.678ns 2.63ns 0.0134 0 0 944 B
#5221 CallElasticsearch netcoreapp3.1 1.52μs 0.62ns 2.32ns 0.0128 0 0 944 B
#5221 CallElasticsearch net472 2.5μs 1.82ns 7.05ns 0.152 0 0 963 B
#5221 CallElasticsearchAsync net6.0 1.3μs 0.569ns 2.05ns 0.0131 0 0 920 B
#5221 CallElasticsearchAsync netcoreapp3.1 1.58μs 1.34ns 4.83ns 0.0133 0 0 992 B
#5221 CallElasticsearchAsync net472 2.62μs 1.43ns 5.52ns 0.161 0 0 1.02 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.27μs 0.362ns 1.4ns 0.0127 0 0 920 B
master ExecuteAsync netcoreapp3.1 1.63μs 0.539ns 2.02ns 0.0123 0 0 920 B
master ExecuteAsync net472 1.76μs 1.04ns 3.88ns 0.14 0 0 883 B
#5221 ExecuteAsync net6.0 1.29μs 0.776ns 2.9ns 0.013 0 0 920 B
#5221 ExecuteAsync netcoreapp3.1 1.68μs 0.784ns 2.93ns 0.0127 0 0 920 B
#5221 ExecuteAsync net472 1.8μs 1.18ns 4.43ns 0.14 0.000896 0 883 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.2μs 0.935ns 3.5ns 0.0293 0 0 2.1 KB
master SendAsync netcoreapp3.1 4.97μs 1.69ns 6.09ns 0.0347 0 0 2.64 KB
master SendAsync net472 7.81μs 2.52ns 9.75ns 0.524 0 0 3.31 KB
#5221 SendAsync net6.0 4.17μs 2.03ns 7.59ns 0.0292 0 0 2.1 KB
#5221 SendAsync netcoreapp3.1 4.78μs 1.44ns 5.59ns 0.0355 0 0 2.64 KB
#5221 SendAsync net472 7.62μs 2.46ns 9.22ns 0.524 0 0 3.31 KB
Benchmarks.Trace.Iast.StringAspectsBenchmark - Faster 🎉 More allocations ⚠️

Faster 🎉 in #5221

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑netcoreapp3.1 1.159 60,650.00 52,350.00 bimodal

More allocations ⚠️ in #5221

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 202.84 KB 211.59 KB 8.75 KB 4.31%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net472 221.18 KB 229.38 KB 8.19 KB 3.70%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 55.75 KB 57.34 KB 1.59 KB 2.86%

Fewer allocations 🎉 in #5221

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0 214.65 KB 203.17 KB -11.48 KB -5.35%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StringConcatBenchmark net6.0 51.3μs 224ns 1.03μs 0 0 0 43.44 KB
master StringConcatBenchmark netcoreapp3.1 60.8μs 731ns 7.17μs 0 0 0 42.64 KB
master StringConcatBenchmark net472 37.8μs 78.3ns 282ns 0 0 0 55.75 KB
master StringConcatAspectBenchmark net6.0 278μs 1.53μs 10μs 0 0 0 214.65 KB
master StringConcatAspectBenchmark netcoreapp3.1 282μs 1.54μs 9.27μs 0 0 0 202.84 KB
master StringConcatAspectBenchmark net472 230μs 2.78μs 26.1μs 0 0 0 221.18 KB
#5221 StringConcatBenchmark net6.0 56.8μs 686ns 6.83μs 0 0 0 43.44 KB
#5221 StringConcatBenchmark netcoreapp3.1 52.3μs 89.6ns 323ns 0 0 0 42.64 KB
#5221 StringConcatBenchmark net472 37.9μs 39.2ns 136ns 0 0 0 57.34 KB
#5221 StringConcatAspectBenchmark net6.0 263μs 1.08μs 4.69μs 0 0 0 203.17 KB
#5221 StringConcatAspectBenchmark netcoreapp3.1 275μs 1.48μs 7.96μs 0 0 0 211.59 KB
#5221 StringConcatAspectBenchmark net472 226μs 2.13μs 19.9μs 0 0 0 229.38 KB
Benchmarks.Trace.ILoggerBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5221

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑net6.0 1.129 1,561.77 1,383.87

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 1.56μs 0.597ns 2.23ns 0.0219 0 0 1.58 KB
master EnrichedLog netcoreapp3.1 2.17μs 0.824ns 2.85ns 0.0211 0 0 1.58 KB
master EnrichedLog net472 2.61μs 1.48ns 5.54ns 0.239 0 0 1.51 KB
#5221 EnrichedLog net6.0 1.38μs 0.65ns 2.52ns 0.0221 0 0 1.58 KB
#5221 EnrichedLog netcoreapp3.1 2.17μs 0.837ns 3.13ns 0.0215 0 0 1.58 KB
#5221 EnrichedLog net472 2.5μs 1.27ns 4.57ns 0.239 0 0 1.51 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 114μs 66ns 256ns 0.0568 0 0 4.22 KB
master EnrichedLog netcoreapp3.1 118μs 166ns 620ns 0 0 0 4.22 KB
master EnrichedLog net472 147μs 77.7ns 291ns 0.661 0.22 0 4.4 KB
#5221 EnrichedLog net6.0 112μs 103ns 385ns 0.0557 0 0 4.22 KB
#5221 EnrichedLog netcoreapp3.1 117μs 86.1ns 333ns 0.059 0 0 4.22 KB
#5221 EnrichedLog net472 147μs 216ns 838ns 0.668 0.223 0 4.4 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.06μs 2.13ns 8.23ns 0.0289 0 0 2.14 KB
master EnrichedLog netcoreapp3.1 4.12μs 2.69ns 10.4ns 0.0289 0 0 2.14 KB
master EnrichedLog net472 4.93μs 1.17ns 4.54ns 0.31 0 0 1.95 KB
#5221 EnrichedLog net6.0 2.98μs 1.22ns 4.72ns 0.03 0 0 2.14 KB
#5221 EnrichedLog netcoreapp3.1 4.15μs 2.43ns 9.39ns 0.029 0 0 2.14 KB
#5221 EnrichedLog net472 4.74μs 2.17ns 8.4ns 0.309 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.39μs 0.434ns 1.68ns 0.0153 0 0 1.11 KB
master SendReceive netcoreapp3.1 1.74μs 1.11ns 4.29ns 0.0147 0 0 1.11 KB
master SendReceive net472 2.13μs 0.61ns 2.2ns 0.178 0 0 1.12 KB
#5221 SendReceive net6.0 1.43μs 0.573ns 2.22ns 0.0157 0 0 1.11 KB
#5221 SendReceive netcoreapp3.1 1.81μs 1.4ns 5.24ns 0.0144 0 0 1.11 KB
#5221 SendReceive net472 2.25μs 0.455ns 1.58ns 0.178 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.86μs 1.11ns 4.15ns 0.0216 0 0 1.54 KB
master EnrichedLog netcoreapp3.1 4.02μs 2.96ns 11.5ns 0.0201 0 0 1.58 KB
master EnrichedLog net472 4.13μs 1.7ns 6.59ns 0.313 0 0 1.97 KB
#5221 EnrichedLog net6.0 2.79μs 0.872ns 3.38ns 0.021 0 0 1.54 KB
#5221 EnrichedLog netcoreapp3.1 3.93μs 1.76ns 6.8ns 0.0213 0 0 1.58 KB
#5221 EnrichedLog net472 4.35μs 2.32ns 8.99ns 0.312 0 0 1.97 KB
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5221

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1 1.127 816.26 724.54

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 438ns 0.808ns 3.13ns 0.00756 0 0 544 B
master StartFinishSpan netcoreapp3.1 817ns 1.28ns 4.94ns 0.00734 0 0 544 B
master StartFinishSpan net472 807ns 1.51ns 5.84ns 0.0864 0 0 546 B
master StartFinishScope net6.0 562ns 0.912ns 3.53ns 0.00942 0 0 664 B
master StartFinishScope netcoreapp3.1 781ns 1.59ns 6.16ns 0.00882 0 0 664 B
master StartFinishScope net472 941ns 2.27ns 8.79ns 0.0994 0 0 626 B
#5221 StartFinishSpan net6.0 447ns 0.724ns 2.81ns 0.00754 0 0 544 B
#5221 StartFinishSpan netcoreapp3.1 724ns 0.709ns 2.65ns 0.00727 0 0 544 B
#5221 StartFinishSpan net472 771ns 1.46ns 5.65ns 0.0866 0 0 546 B
#5221 StartFinishScope net6.0 613ns 0.633ns 2.45ns 0.00943 0 0 664 B
#5221 StartFinishScope netcoreapp3.1 832ns 1.82ns 7.04ns 0.00901 0 0 664 B
#5221 StartFinishScope net472 901ns 2.98ns 11.5ns 0.0991 0 0 626 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 586ns 1.14ns 4.4ns 0.00929 0 0 664 B
master RunOnMethodBegin netcoreapp3.1 913ns 3.54ns 13.7ns 0.00882 0 0 664 B
master RunOnMethodBegin net472 1.11μs 2.02ns 7.81ns 0.0991 0 0 626 B
#5221 RunOnMethodBegin net6.0 652ns 0.957ns 3.71ns 0.0092 0 0 664 B
#5221 RunOnMethodBegin netcoreapp3.1 986ns 2.1ns 8.14ns 0.00877 0 0 664 B
#5221 RunOnMethodBegin net472 1.1μs 2.03ns 7.86ns 0.0992 0 0 626 B

@andrewlock

andrewlock commented Feb 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 (5221) (10.720M)   : 0, 10720498
    master (10.916M)   : 0, 10915647
    benchmarks/2.9.0 (11.425M)   : 0, 11424847

    section Automatic
    This PR (5221) (7.379M)   : 0, 7379121
    master (7.507M)   : 0, 7506546
    benchmarks/2.9.0 (8.301M)   : 0, 8300659

    section Trace stats
    This PR (5221) (7.677M)   : 0, 7677446
    master (7.852M)   : 0, 7852061

    section Manual
    This PR (5221) (9.155M)   : 0, 9154740
    master (9.470M)   : 0, 9470416

    section Manual + Automatic
    This PR (5221) (6.991M)   : 0, 6990618
    master (7.139M)   : 0, 7138949

    section Version Conflict
    This PR (5221) (6.318M)   : 0, 6318202
    master (6.402M)   : 0, 6402182
Loading 
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5221) (9.636M)   : 0, 9635992
    master (9.641M)   : 0, 9641059
    benchmarks/2.9.0 (9.641M)   : 0, 9641077

    section Automatic
    This PR (5221) (6.674M)   : 0, 6673801
    master (6.576M)   : 0, 6576031

    section Trace stats
    This PR (5221) (6.917M)   : 0, 6917144
    master (6.884M)   : 0, 6883620

    section Manual
    This PR (5221) (8.084M)   : 0, 8083580
    master (8.383M)   : 0, 8382832

    section Manual + Automatic
    This PR (5221) (6.171M)   : 0, 6170584
    master (6.133M)   : 0, 6132725

    section Version Conflict
    This PR (5221) (5.642M)   : 0, 5642317
    master (5.798M)   : 0, 5798106
Loading 
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5221) (10.026M)   : 0, 10025612
    master (10.207M)   : 0, 10206635
    benchmarks/2.9.0 (9.812M)   : 0, 9811640

    section Automatic
    This PR (5221) (6.955M)   : crit ,0, 6954508
    master (7.341M)   : 0, 7341487
    benchmarks/2.9.0 (7.410M)   : 0, 7410467

    section Trace stats
    This PR (5221) (7.211M)   : crit ,0, 7210755
    master (7.682M)   : 0, 7681925

    section Manual
    This PR (5221) (8.846M)   : 0, 8846345
    master (9.230M)   : 0, 9229601

    section Manual + Automatic
    This PR (5221) (6.557M)   : crit ,0, 6556789
    master (7.027M)   : 0, 7027451

    section Version Conflict
    This PR (5221) (5.896M)   : crit ,0, 5895509
    master (6.367M)   : 0, 6367128
Loading 
gantt
    title Throughput Linux x64 (ASM) (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5221) (7.454M)   : 0, 7453984
    master (7.497M)   : 0, 7497198
    benchmarks/2.9.0 (7.805M)   : 0, 7805433

    section No attack
    This PR (5221) (1.829M)   : 0, 1829148
    master (1.845M)   : 0, 1844618
    benchmarks/2.9.0 (3.184M)   : 0, 3183762

    section Attack
    This PR (5221) (1.453M)   : 0, 1452685
    master (1.448M)   : 0, 1447941
    benchmarks/2.9.0 (2.497M)   : 0, 2497002

    section Blocking
    This PR (5221) (3.132M)   : 0, 3132269
    master (3.161M)   : 0, 3160570

    section IAST default
    This PR (5221) (6.460M)   : 0, 6459533
    master (6.463M)   : 0, 6462789

    section IAST full
    This PR (5221) (5.508M)   : 0, 5507839
    master (5.640M)   : 0, 5639988

    section Base vuln
    This PR (5221) (0.964M)   : 0, 964127
    master (0.914M)   : 0, 913733

    section IAST vuln
    This PR (5221) (0.862M)   : 0, 862205
    master (0.871M)   : 0, 870835
Loading

@NachoEchevarria NachoEchevarria changed the title IAST - Fix HttpRequestValidationException Error [ASM] - Fix HttpRequestValidationException Error Feb 22, 2024
andrewlock
andrewlock approved these changes Feb 28, 2024
View reviewed changes

@andrewlock andrewlock left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for hunting down a repro!

Comment thread tracer/src/Datadog.Trace/Util/QueryStringHelper.cs
Comment on lines +37 to +40
internal static QueryString GetQueryString(HttpRequest request)
{
return request.QueryString;
}

@andrewlock andrewlock Feb 28, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was undecided whether it's worth keeping this, but I think the consistency makes sense 👍

Comment thread tracer/test/Datadog.Trace.Tests/Util/QueryStringHelperTests.cs
[Fact]
public void GivenADangerousQueryString_WhenGetQueryString_HelperAvoidsException()
{
var request = new HttpRequest("file", "http://localhost/benchmarks", "data=<script>alert(1)</script>");

@andrewlock andrewlock Feb 28, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome, thanks for repro-ing this! It might be worth trying to create the same thing for .NET Core, just to prove it's fine 🙂

@NachoEchevarria
NachoEchevarria merged commit e69a035 into master Feb 29, 2024
@NachoEchevarria
NachoEchevarria deleted the nacho/IastQueryStringFix branch February 29, 2024 14:42
@github-actions github-actions Bot added this to the vNext milestone Feb 29, 2024
@NachoEchevarria NachoEchevarria mentioned this pull request Jun 7, 2024
Merged
@NachoEchevarria NachoEchevarria mentioned this pull request Aug 26, 2024
Merged
NachoEchevarria added a commit that referenced this pull request Sep 3, 2024
@NachoEchevarria
[ASM] Avoid unhandled HttpRequestValidationExceptions (#5943)
053190c 
## Summary of changes

We are logging some not handled HttpRequestValidationException errors

[In the past](#5221), we
protected the reading of the query string to avoid such issues, but now
they seem to be caused by the values of headers and cookies.

This error occurs in .Net framework. If the method request.ValidateInput
is called, the request will launch an exception if the cookies, url or
header values are dangerous.

In the past, we created a class called QueryStringValidator. In this PR,
the features of this class are extended to cookies and headers and urls.
The class has been renamed to RequestDataValidator.

## Reason for change

## Implementation details

## Test coverage

Some unit tests have been added.

## Other details
<!-- Fixes #{issue} -->

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robertpi robertpi robertpi approved these changes
@andrewlock andrewlock andrewlock approved these changes

Assignees

No one assigned

Labels

area:asm area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations)

Projects

None yet

Milestone

2.49.0

Development

Successfully merging this pull request may close these issues.

3 participants

@NachoEchevarria @andrewlock @robertpi