[ASM] Avoid HttpRequestValidationException when reading body or namevalueCollection values #6185

NachoEchevarria · 2024-10-23T12:29:58Z

Summary of changes

Some HttpRequestValidationExceptions were detected when accessing the property httpRequest.Form or when accessing individual to values within a NamevalueCollection. This is something that can happen when accessing these values with the validation mechanisms enabled within the request. It only applies to .net framework.

In the past, similar exceptions were addressed by using the class ReuqestDataHelper. Some new methods have been added to handle this kind of situations.

Reason for change

Implementation details

Test coverage

Other details

andrewlock · 2024-10-23T13:19:39Z

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

    section CallTarget+Inlining+NGEN
    This PR (6185) - mean (1,108ms)  : 1088, 1128
     .   : milestone, 1108,
    master - mean (1,110ms)  : 1092, 1128
     .   : milestone, 1110,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6185) - mean (108ms)  : 106, 111
     .   : milestone, 108,
    master - mean (108ms)  : 106, 111
     .   : milestone, 108,

    section CallTarget+Inlining+NGEN
    This PR (6185) - mean (772ms)  : 759, 784
     .   : milestone, 772,
    master - mean (774ms)  : 762, 786
     .   : milestone, 774,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6185) - mean (92ms)  : 90, 94
     .   : milestone, 92,
    master - mean (92ms)  : 90, 94
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (6185) - mean (729ms)  : 713, 746
     .   : milestone, 729,
    master - mean (730ms)  : 711, 750
     .   : milestone, 730,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6185) - mean (190ms)  : 186, 195
     .   : milestone, 190,
    master - mean (191ms)  : 188, 194
     .   : milestone, 191,

    section CallTarget+Inlining+NGEN
    This PR (6185) - mean (1,221ms)  : 1198, 1245
     .   : milestone, 1221,
    master - mean (1,225ms)  : 1200, 1250
     .   : milestone, 1225,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6185) - mean (277ms)  : 272, 281
     .   : milestone, 277,
    master - mean (277ms)  : 273, 281
     .   : milestone, 277,

    section CallTarget+Inlining+NGEN
    This PR (6185) - mean (950ms)  : 931, 970
     .   : milestone, 950,
    master - mean (954ms)  : 937, 972
     .   : milestone, 954,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6185) - mean (264ms)  : 260, 268
     .   : milestone, 264,
    master - mean (265ms)  : 261, 270
     .   : milestone, 265,

    section CallTarget+Inlining+NGEN
    This PR (6185) - mean (935ms)  : 912, 958
     .   : milestone, 935,
    master - mean (936ms)  : 919, 953
     .   : milestone, 936,

datadog-ddstaging · 2024-10-23T13:42:16Z

Datadog Report

Branch report: nacho/QueryStringGetValuesException
Commit report: ce1c879
Test service: dd-trace-dotnet

✅ 0 Failed, 368513 Passed, 2065 Skipped, 15h 37m 14.78s Total Time
❄️ 1 New Flaky

New Flaky Tests (1)

AfterInitialSuccessThenMultipleFailures_SinkIsTemporarilyDisabled - Datadog.Trace.Tests.Logging.DirectSubmission.Sink.PeriodicBatching.BatchingSinkTests - Last Failure
Expand for error
```
 Expected sink.Batches.Count to be 11, but found 12 (difference of 1).
```

andrewlock · 2024-10-23T14:41:26Z

Benchmarks Report for appsec 🐌

Benchmarks for #6185 compared to master:

1 benchmarks are faster, with geometric mean 1.314
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 - Faster 🎉 Same allocations ✔️

Faster 🎉 in #6185

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net472	1.314	216.82	165.00

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	72.8μs	94.1ns	339ns	0.0787	0	6 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	64.4μs	93.4ns	362ns	0.0962	0	6.95 KB
master	`AllCycleSimpleBody`	net472	49.2μs	79.2ns	307ns	1.32	0	8.34 KB
master	`AllCycleMoreComplexBody`	net6.0	79.3μs	94.4ns	353ns	0.119	0	9.51 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	70.7μs	80.1ns	310ns	0.141	0	10.37 KB
master	`AllCycleMoreComplexBody`	net472	56.9μs	104ns	401ns	1.87	0.0283	11.85 KB
master	`ObjectExtractorSimpleBody`	net6.0	142ns	0.142ns	0.53ns	0.00395	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	207ns	0.119ns	0.446ns	0.00374	0	272 B
master	`ObjectExtractorSimpleBody`	net472	217ns	0.104ns	0.402ns	0.0445	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	3.04μs	2.29ns	8.58ns	0.0533	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.94μs	4ns	15.5ns	0.0491	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	4.3μs	1.41ns	5.28ns	0.602	0.00645	3.8 KB
#6185	`AllCycleSimpleBody`	net6.0	72.5μs	81.8ns	317ns	0.0728	0	6.01 KB
#6185	`AllCycleSimpleBody`	netcoreapp3.1	63μs	44.4ns	166ns	0.0944	0	6.95 KB
#6185	`AllCycleSimpleBody`	net472	48.4μs	51.6ns	200ns	1.31	0	8.33 KB
#6185	`AllCycleMoreComplexBody`	net6.0	78.1μs	46.6ns	175ns	0.117	0	9.51 KB
#6185	`AllCycleMoreComplexBody`	netcoreapp3.1	70.3μs	63.5ns	237ns	0.139	0	10.36 KB
#6185	`AllCycleMoreComplexBody`	net472	56.4μs	35.7ns	138ns	1.86	0.0282	11.85 KB
#6185	`ObjectExtractorSimpleBody`	net6.0	141ns	0.231ns	0.895ns	0.00391	0	280 B
#6185	`ObjectExtractorSimpleBody`	netcoreapp3.1	211ns	0.322ns	1.25ns	0.0037	0	272 B
#6185	`ObjectExtractorSimpleBody`	net472	165ns	0.176ns	0.658ns	0.0446	0	281 B
#6185	`ObjectExtractorMoreComplexBody`	net6.0	3.08μs	3.64ns	14.1ns	0.0536	0	3.78 KB
#6185	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.99μs	2.73ns	10.6ns	0.0498	0	3.69 KB
#6185	`ObjectExtractorMoreComplexBody`	net472	4.64μs	2.4ns	9.29ns	0.601	0.00694	3.8 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EncodeArgs`	net6.0	38.6μs	32.2ns	125ns	0.442	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	54.1μs	22.8ns	88.2ns	0.433	0	32.4 KB
master	`EncodeArgs`	net472	66μs	42.5ns	165ns	5.15	0.066	32.5 KB
master	`EncodeLegacyArgs`	net6.0	76.5μs	43.4ns	163ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	104μs	78.2ns	303ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	153μs	105ns	407ns	0.304	0	2.15 KB
#6185	`EncodeArgs`	net6.0	38.5μs	33.3ns	129ns	0.46	0	32.4 KB
#6185	`EncodeArgs`	netcoreapp3.1	54.2μs	30.2ns	113ns	0.431	0	32.4 KB
#6185	`EncodeArgs`	net472	67.3μs	29.9ns	112ns	5.16	0.067	32.5 KB
#6185	`EncodeLegacyArgs`	net6.0	74.2μs	48.9ns	169ns	0	0	2.14 KB
#6185	`EncodeLegacyArgs`	netcoreapp3.1	107μs	93.1ns	349ns	0	0	2.14 KB
#6185	`EncodeLegacyArgs`	net472	156μs	81.2ns	314ns	0.313	0	2.15 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunWafRealisticBenchmark`	net6.0	184μs	153ns	550ns	0	2.44 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	196μs	346ns	1.34μs	0	2.39 KB
master	`RunWafRealisticBenchmark`	net472	211μs	212ns	821ns	0.315	2.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	127μs	223ns	862ns	0	1.47 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	130μs	112ns	419ns	0	1.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	144μs	55.1ns	213ns	0.209	1.49 KB
#6185	`RunWafRealisticBenchmark`	net6.0	184μs	68ns	263ns	0	2.44 KB
#6185	`RunWafRealisticBenchmark`	netcoreapp3.1	198μs	264ns	1.02μs	0	2.39 KB
#6185	`RunWafRealisticBenchmark`	net472	209μs	89.8ns	348ns	0.311	2.46 KB
#6185	`RunWafRealisticBenchmarkWithAttack`	net6.0	122μs	91.6ns	355ns	0	1.47 KB
#6185	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	131μs	145ns	561ns	0	1.46 KB
#6185	`RunWafRealisticBenchmarkWithAttack`	net472	139μs	116ns	449ns	0.208	1.49 KB

Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ Fewer allocations 🎉

Fewer allocations 🎉 in #6185

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0	263.22 KB	255.4 KB	-7.82 KB	-2.97%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	62.21 KB	59.04 KB	-3.17 KB	-5.09%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`StringConcatBenchmark`	net6.0	61.8μs	779ns	7.63μs	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	59.3μs	676ns	6.59μs	42.64 KB
master	`StringConcatBenchmark`	net472	38.4μs	134ns	500ns	62.21 KB
master	`StringConcatAspectBenchmark`	net6.0	303μs	1.48μs	7.95μs	263.22 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	340μs	1.85μs	10.9μs	254.29 KB
master	`StringConcatAspectBenchmark`	net472	278μs	5.27μs	51.4μs	278.53 KB
#6185	`StringConcatBenchmark`	net6.0	62.4μs	914ns	8.95μs	43.44 KB
#6185	`StringConcatBenchmark`	netcoreapp3.1	53.7μs	265ns	1.06μs	42.64 KB
#6185	`StringConcatBenchmark`	net472	38.3μs	146ns	547ns	59.04 KB
#6185	`StringConcatAspectBenchmark`	net6.0	298μs	4.39μs	41.4μs	255.4 KB
#6185	`StringConcatAspectBenchmark`	netcoreapp3.1	348μs	1.95μs	14.8μs	255.55 KB
#6185	`StringConcatAspectBenchmark`	net472	266μs	4.9μs	46.8μs	278.53 KB

andrewlock · 2024-10-23T15:25:39Z

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 (6185) (11.261M)   : 0, 11260742
    master (11.034M)   : 0, 11033944
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6185) (7.219M)   : 0, 7219043
    master (7.254M)   : 0, 7253523
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.563M)   : 0, 7562798

    section Manual
    master (11.077M)   : 0, 11077275

    section Manual + Automatic
    This PR (6185) (6.619M)   : 0, 6619036
    master (6.721M)   : 0, 6721446

    section DD_TRACE_ENABLED=0
    master (10.177M)   : 0, 10177332

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6185) (9.623M)   : 0, 9622709
    master (9.472M)   : 0, 9472389
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6185) (6.347M)   : 0, 6347004
    master (6.406M)   : 0, 6405513

    section Trace stats
    master (6.754M)   : 0, 6754288

    section Manual
    master (9.485M)   : 0, 9484735

    section Manual + Automatic
    This PR (6185) (6.109M)   : 0, 6108827
    master (5.838M)   : 0, 5838489

    section DD_TRACE_ENABLED=0
    master (8.937M)   : 0, 8936952

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6185) (9.312M)   : 0, 9312374
    master (9.618M)   : 0, 9618393
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6185) (6.207M)   : 0, 6206600
    master (6.392M)   : 0, 6392314
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (6.974M)   : 0, 6973795

    section Manual
    master (9.798M)   : 0, 9797512

    section Manual + Automatic
    This PR (6185) (5.912M)   : 0, 5912348
    master (6.081M)   : 0, 6080661

    section DD_TRACE_ENABLED=0
    master (9.137M)   : 0, 9136586

andrewlock · 2024-10-23T21:38:44Z

Benchmarks Report for tracer 🐌

Benchmarks for #6185 compared to master:

3 benchmarks are slower, with geometric mean 1.130
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	8.09μs	45.9ns	335ns	0.0203	0.00811	0	5.6 KB
master	`StartStopWithChild`	netcoreapp3.1	10.1μs	52.3ns	267ns	0.0243	0.0097	0	5.81 KB
master	`StartStopWithChild`	net472	16.2μs	42.6ns	165ns	1.05	0.316	0.0973	6.2 KB
#6185	`StartStopWithChild`	net6.0	8.08μs	44ns	253ns	0.0157	0.00785	0	5.61 KB
#6185	`StartStopWithChild`	netcoreapp3.1	9.77μs	52.4ns	267ns	0.019	0.00951	0	5.8 KB
#6185	`StartStopWithChild`	net472	16.5μs	53.9ns	209ns	1.04	0.301	0.0835	6.21 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	482μs	219ns	788ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	637μs	609ns	2.28μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	840μs	456ns	1.77μs	0.417	3.3 KB
#6185	`WriteAndFlushEnrichedTraces`	net6.0	466μs	185ns	667ns	0	2.7 KB
#6185	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	660μs	364ns	1.41μs	0	2.7 KB
#6185	`WriteAndFlushEnrichedTraces`	net472	834μs	623ns	2.33μs	0.417	3.3 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net6.0	224μs	2.55μs	25.3μs	0.192	18.73 KB
master	`SendRequest`	netcoreapp3.1	232μs	1.41μs	14μs	0.231	20.89 KB
master	`SendRequest`	net472	0.000898ns	0.000408ns	0.00158ns	0	0 b
#6185	`SendRequest`	net6.0	194μs	1.09μs	7.68μs	0.183	18.73 KB
#6185	`SendRequest`	netcoreapp3.1	224μs	1.26μs	8.32μs	0.208	20.89 KB
#6185	`SendRequest`	net472	0.00123ns	0.000514ns	0.00192ns	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	546μs	2.57μs	10.6μs	0.548	0	0	41.55 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	676μs	3.63μs	21.2μs	0.34	0	0	41.67 KB
master	`WriteAndFlushEnrichedTraces`	net472	870μs	2.82μs	10.9μs	8.3	2.62	0.437	53.37 KB
#6185	`WriteAndFlushEnrichedTraces`	net6.0	577μs	2.79μs	12.1μs	0.543	0	0	41.65 KB
#6185	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	676μs	3.31μs	14.4μs	0.324	0	0	41.82 KB
#6185	`WriteAndFlushEnrichedTraces`	net472	872μs	4.22μs	16.9μs	8.08	2.55	0.425	53.32 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net6.0	1.33μs	1.07ns	3.99ns	0.0146	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.72μs	1.06ns	3.98ns	0.014	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.08μs	2.1ns	8.15ns	0.156	0.00104	987 B
#6185	`ExecuteNonQuery`	net6.0	1.42μs	1.16ns	4.34ns	0.0141	0	1.02 KB
#6185	`ExecuteNonQuery`	netcoreapp3.1	1.73μs	0.896ns	3.47ns	0.0138	0	1.02 KB
#6185	`ExecuteNonQuery`	net472	2.08μs	1.9ns	7.13ns	0.156	0.00105	987 B

Benchmarks.Trace.ElasticsearchBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6185

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑net6.0	1.132	1,146.17	1,297.18

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net6.0	1.15μs	0.561ns	2.17ns	0.0138	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.48μs	0.318ns	1.19ns	0.0134	976 B
master	`CallElasticsearch`	net472	2.48μs	2.3ns	8.91ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.36μs	0.4ns	1.55ns	0.0135	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.72μs	0.348ns	1.3ns	0.0139	1.02 KB
master	`CallElasticsearchAsync`	net472	2.67μs	0.952ns	3.56ns	0.167	1.05 KB
#6185	`CallElasticsearch`	net6.0	1.31μs	3.85ns	14.9ns	0.0137	976 B
#6185	`CallElasticsearch`	netcoreapp3.1	1.62μs	0.569ns	2.2ns	0.0131	976 B
#6185	`CallElasticsearch`	net472	2.52μs	0.836ns	3.13ns	0.157	995 B
#6185	`CallElasticsearchAsync`	net6.0	1.24μs	0.45ns	1.68ns	0.013	952 B
#6185	`CallElasticsearchAsync`	netcoreapp3.1	1.67μs	0.958ns	3.71ns	0.0134	1.02 KB
#6185	`CallElasticsearchAsync`	net472	2.71μs	0.957ns	3.45ns	0.167	1.05 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net6.0	1.25μs	0.663ns	2.48ns	0.0131	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.68μs	1.45ns	5.61ns	0.0125	952 B
master	`ExecuteAsync`	net472	1.95μs	0.577ns	2.16ns	0.145	915 B
#6185	`ExecuteAsync`	net6.0	1.25μs	0.622ns	2.41ns	0.0129	952 B
#6185	`ExecuteAsync`	netcoreapp3.1	1.61μs	0.823ns	3.19ns	0.0129	952 B
#6185	`ExecuteAsync`	net472	1.76μs	0.628ns	2.35ns	0.145	915 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net6.0	4.33μs	1.11ns	4.16ns	0.0325	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.26μs	1.74ns	6.74ns	0.0367	2.85 KB
master	`SendAsync`	net472	7.63μs	2.85ns	11ns	0.493	3.12 KB
#6185	`SendAsync`	net6.0	4.29μs	2.6ns	10.1ns	0.0323	2.31 KB
#6185	`SendAsync`	netcoreapp3.1	5.28μs	3.03ns	11.7ns	0.0369	2.85 KB
#6185	`SendAsync`	net472	7.57μs	1.98ns	7.41ns	0.493	3.12 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.49μs	0.827ns	3.09ns	0.0233	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.19μs	1ns	3.75ns	0.0219	1.64 KB
master	`EnrichedLog`	net472	2.61μs	0.78ns	2.81ns	0.249	1.57 KB
#6185	`EnrichedLog`	net6.0	1.62μs	1.04ns	4.04ns	0.0225	1.64 KB
#6185	`EnrichedLog`	netcoreapp3.1	2.28μs	1.64ns	6.34ns	0.0215	1.64 KB
#6185	`EnrichedLog`	net472	2.56μs	1.07ns	4.16ns	0.249	1.57 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net6.0	116μs	184ns	711ns	0.0581	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	123μs	285ns	1.1μs	0.061	0	4.28 KB
master	`EnrichedLog`	net472	152μs	151ns	584ns	0.683	0.228	4.46 KB
#6185	`EnrichedLog`	net6.0	118μs	182ns	707ns	0.0588	0	4.28 KB
#6185	`EnrichedLog`	netcoreapp3.1	120μs	218ns	816ns	0	0	4.28 KB
#6185	`EnrichedLog`	net472	151μs	339ns	1.31μs	0.679	0.226	4.46 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.99μs	1.22ns	4.56ns	0.0301	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.04μs	2.24ns	8.4ns	0.0286	2.2 KB
master	`EnrichedLog`	net472	4.91μs	1.34ns	4.82ns	0.321	2.02 KB
#6185	`EnrichedLog`	net6.0	2.91μs	0.722ns	2.8ns	0.0308	2.2 KB
#6185	`EnrichedLog`	netcoreapp3.1	4.17μs	1.56ns	5.84ns	0.0293	2.2 KB
#6185	`EnrichedLog`	net472	4.9μs	1.36ns	5.26ns	0.32	2.02 KB

Benchmarks.Trace.RedisBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6185

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.RedisBenchmark.SendReceive‑net6.0	1.122	1,243.23	1,395.47

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net6.0	1.24μs	0.617ns	2.23ns	0.0162	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.77μs	1.63ns	6.11ns	0.0152	1.14 KB
master	`SendReceive`	net472	2.05μs	1.5ns	5.8ns	0.183	1.16 KB
#6185	`SendReceive`	net6.0	1.4μs	0.722ns	2.79ns	0.016	1.14 KB
#6185	`SendReceive`	netcoreapp3.1	1.82μs	1.08ns	4.03ns	0.0155	1.14 KB
#6185	`SendReceive`	net472	2.17μs	1.16ns	4.34ns	0.183	1.16 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	2.73μs	0.629ns	2.35ns	0.0218	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.91μs	1.07ns	4.16ns	0.0215	1.65 KB
master	`EnrichedLog`	net472	4.32μs	2.78ns	10.8ns	0.323	2.04 KB
#6185	`EnrichedLog`	net6.0	2.88μs	3.54ns	12.3ns	0.0216	1.6 KB
#6185	`EnrichedLog`	netcoreapp3.1	3.87μs	1.74ns	6.28ns	0.0213	1.65 KB
#6185	`EnrichedLog`	net472	4.39μs	2.56ns	9.93ns	0.323	2.04 KB

Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6185

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0	1.136	480.51	545.89

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	403ns	0.233ns	0.874ns	0.00803	576 B
master	`StartFinishSpan`	netcoreapp3.1	562ns	0.629ns	2.35ns	0.00761	576 B
master	`StartFinishSpan`	net472	711ns	0.692ns	2.68ns	0.0915	578 B
master	`StartFinishScope`	net6.0	481ns	0.512ns	1.98ns	0.00982	696 B
master	`StartFinishScope`	netcoreapp3.1	700ns	0.436ns	1.69ns	0.00959	696 B
master	`StartFinishScope`	net472	887ns	0.689ns	2.67ns	0.104	658 B
#6185	`StartFinishSpan`	net6.0	393ns	0.287ns	1.11ns	0.00806	576 B
#6185	`StartFinishSpan`	netcoreapp3.1	622ns	0.385ns	1.49ns	0.00773	576 B
#6185	`StartFinishSpan`	net472	716ns	0.916ns	3.55ns	0.0917	578 B
#6185	`StartFinishScope`	net6.0	546ns	0.371ns	1.44ns	0.00962	696 B
#6185	`StartFinishScope`	netcoreapp3.1	664ns	0.713ns	2.76ns	0.00929	696 B
#6185	`StartFinishScope`	net472	885ns	0.738ns	2.86ns	0.104	658 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	620ns	0.759ns	2.94ns	0.00966	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	940ns	0.844ns	3.27ns	0.00942	696 B
master	`RunOnMethodBegin`	net472	1.14μs	1.77ns	6.84ns	0.104	658 B
#6185	`RunOnMethodBegin`	net6.0	665ns	0.437ns	1.69ns	0.00968	696 B
#6185	`RunOnMethodBegin`	netcoreapp3.1	902ns	0.858ns	3.32ns	0.00943	696 B
#6185	`RunOnMethodBegin`	net472	1.19μs	1.07ns	4.14ns	0.104	658 B

andrewlock · 2024-11-07T12:33:48Z

tracer/src/Datadog.Trace/AppSec/Coordinator/SecurityCoordinator.Framework.cs

+            return null;
+        }
+
+        var formData = new Dictionary<string, object>(form.Keys.Count);


I wonder, does the above protection definitely protect you? 🤔 i.e. this code suggests you get an exception when, you first access _httpTransport.Context.Request.Form (hence the protection above)?

If so, then it seems unlikely that we need the protections below any more? And if we do then it seems very likely that form.Keys would also throw, no 🤔

I think it would be nice if we had some tests (integration or unit) that replicates the verification that aspnet does so we can know we're actually protecting ourselves correctly 🙂

I was basically checking the source code to find places where the exceptions could potentially be thrown.

HttpRequest.Form (Get) calls ValidateHttpValueCollection. Same for GetValues and Get. Keys does not seem to check that.

I tried to reproduce the case where getting the form does not throw exception but later, getting the values does but did not get it. But the stack in some errors detected in customers shows that this can happen. Anyway, I will add some unit tests trying to cover all the cases.

I have added some tests. Basically, when normal validation is enabled, accessing request.Form would trigger the exception.
HttpRequest also allows to perform what they call "granular validation", which is different than the normal mode. This mode translates validation to the HttpCollections fields of the request by adding a callback to each of them that is called when their values are accessed, instead of being done at a httpRequest level when accessing the property Form.
This mode seems to be enabled by the framework and I could not find an easy way to enable it without reflection. The involved methods are internal and sometimes use classes that cannot be accessed out of the scope of the assembly without reflection.
In summary: a query with regular validation will throw an exception when accessing Form. A query with granular validation would not throw an exception when accessing Form but it will throw it when accessing GetValues or Get. The keys property never throws any exception.
The granular validation mode would explain why we have some exceptions that where thrown when calling GetValues but did not thrown it earlier, when accessing Form.

https://app.datadoghq.com/error-tracking?query=service%3Ainstrumentation-telemetry-data%20%2Aappsec%2A%20%40lib_language%3Adotnet&fromUser=false&issueId=061ff9ca-0ddb-11ef-bfc4-da7ad0900002&refresh_mode=sliding&source=all&from_ts=1730449059747&to_ts=1731053859747&live=true

:TIL: Interesting, thanks for the explanation! It might be worth adding some of that to the code itself 🙂 Thanks!

Done! Thanks!

andrewlock

Nice, thank you!

NachoEchevarria · 2024-11-11T13:30:31Z

Thanks for you feedback and reviews!

NachoEchevarria force-pushed the nacho/QueryStringGetValuesException branch from 4336d4b to 52d9ad6 Compare October 23, 2024 12:42

NachoEchevarria added 4 commits October 31, 2024 15:22

Protect against exception

542e93c

Protect more areas of the code

57adccf

correct comments

e032154

correct log message

23de289

NachoEchevarria force-pushed the nacho/QueryStringGetValuesException branch from c96ebc6 to 23de289 Compare October 31, 2024 14:25

Fix

e11e0bf

NachoEchevarria changed the title ~~Nacho/query string get values exception~~ [ASM] Avoid exceptions Oct 31, 2024

NachoEchevarria changed the title ~~[ASM] Avoid exceptions~~ [ASM] Avoid HttpRequestValidationException when reading body or namevalueCollection values Oct 31, 2024

Merge branch 'master' into nacho/QueryStringGetValuesException

d910cb7

github-actions bot added the area:asm label Nov 5, 2024

Fix test

0b35bf1

NachoEchevarria marked this pull request as ready for review November 7, 2024 11:37

NachoEchevarria requested review from a team as code owners November 7, 2024 11:37

e-n-0 approved these changes Nov 7, 2024

View reviewed changes

andrewlock reviewed Nov 7, 2024

View reviewed changes

NachoEchevarria and others added 4 commits November 7, 2024 18:39

Add unit test

17453fe

Merge branch 'master' into nacho/QueryStringGetValuesException

2534da1

Add granular validation unit tests

d0127fb

add key test

dc1a4e1

NachoEchevarria requested a review from andrewlock November 8, 2024 14:09

andrewlock approved these changes Nov 8, 2024

View reviewed changes

NachoEchevarria added 2 commits November 11, 2024 11:44

Add comment

e7a3fbb

Merge branch 'master' into nacho/QueryStringGetValuesException

ce1c879

NachoEchevarria merged commit 3079346 into master Nov 11, 2024
78 checks passed

NachoEchevarria deleted the nacho/QueryStringGetValuesException branch November 11, 2024 13:30

github-actions bot added this to the vNext-v3 milestone Nov 11, 2024

andrewlock added the identified-by:telemetry label Nov 18, 2024

[ASM] Avoid HttpRequestValidationException when reading body or namevalueCollection values #6185

[ASM] Avoid HttpRequestValidationException when reading body or namevalueCollection values #6185

Conversation

NachoEchevarria commented Oct 23, 2024 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

andrewlock commented Oct 23, 2024 • edited Loading

Execution-Time Benchmarks Report ⏱️

datadog-ddstaging bot commented Oct 23, 2024 • edited Loading

Datadog Report

New Flaky Tests (1)

andrewlock commented Oct 23, 2024 • edited Loading

Benchmarks Report for appsec 🐌

Benchmark details

Faster 🎉 in #6185

Raw results

Raw results

Raw results

Fewer allocations 🎉 in #6185

Raw results

andrewlock commented Oct 23, 2024 • edited Loading

Throughput/Crank Report ⚡

andrewlock commented Oct 23, 2024 • edited Loading

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6185

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #6185

Raw results

Raw results

Slower ⚠️ in #6185

Raw results

Raw results

andrewlock Nov 7, 2024

Choose a reason for hiding this comment

NachoEchevarria Nov 7, 2024

Choose a reason for hiding this comment

NachoEchevarria Nov 8, 2024

Choose a reason for hiding this comment

andrewlock Nov 8, 2024

Choose a reason for hiding this comment

NachoEchevarria Nov 11, 2024

Choose a reason for hiding this comment

andrewlock left a comment

Choose a reason for hiding this comment

NachoEchevarria commented Nov 11, 2024

NachoEchevarria commented Oct 23, 2024 •

edited

Loading

andrewlock commented Oct 23, 2024 •

edited

Loading

datadog-ddstaging bot commented Oct 23, 2024 •

edited

Loading

andrewlock commented Oct 23, 2024 •

edited

Loading

andrewlock commented Oct 23, 2024 •

edited

Loading

andrewlock commented Oct 23, 2024 •

edited

Loading