[ASM] Avoid unhandled HttpRequestValidationExceptions by NachoEchevarria · Pull Request #5943 · DataDog/dd-trace-dotnet

NachoEchevarria · 2024-08-26T08:37:39Z

Summary of changes

We are logging some not handled HttpRequestValidationException errors

In the past, 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

datadog-ddstaging · 2024-08-26T08:59:21Z

Datadog Report

Branch report: nacho/AvoidValidationExceptions
Commit report: ddd6235
Test service: dd-trace-dotnet

❌ 1 Failed (1 Known Flaky), 364634 Passed, 2054 Skipped, 15h 33m 27.49s Total Time

❌ Failed Tests (1)

Profiler_liveheap_cpu_walltime - scenarios - ❄️ Known flaky - Details

Expand for error

 
  ########### Finishing run at 8/30/2024 12:05:24 PM262eak 25 times with 1 thread(s).shared\bin\monitoring-home\win-x86\Datadog.Profiler.Native.dlltation 5=compute fibonacci 6=n sleeping threads 7=async calls 8=iterator calls 9=allocate array of Generic<int>> 10=threads competing for a lock 11=lunix signal handler 12=trigger garbage collections 13=memory leak 14=short lived threads] [--param <any number to pass to the scenario - used for contention duration for example>] [--timeout <duration in seconds> | --run-infinitely]uteWithThreadLocal(System.Threading.Tasks.Task ByRef, System.Threading.Thread)
    at System.Threading.Tasks.Task.ExecuteEntryUnsafe(System.Threading.Thread)
    at System.Threading.Tasks.ThreadPoolTaskScheduler+<>c.<.cctor>b__10_0(System.Object)
    at System.Threading.Thread.StartCallback()

andrewlock · 2024-08-26T09:00:45Z

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).

andrewlock · 2024-08-26T09:34:36Z

Benchmarks Report for appsec 🐌

Benchmarks for #5943 compared to master:

All benchmarks have the same speed
1 benchmarks have fewer allocations
2 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.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	73.3μs	82ns	318ns	0.0729	0	6 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	62.7μs	68.4ns	265ns	0.0629	0	6.95 KB
master	`AllCycleSimpleBody`	net472	48.8μs	69.8ns	271ns	1.32	0	8.34 KB
master	`AllCycleMoreComplexBody`	net6.0	79.6μs	119ns	443ns	0.12	0	9.51 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	69.6μs	81.5ns	316ns	0.14	0	10.37 KB
master	`AllCycleMoreComplexBody`	net472	55.9μs	60.3ns	226ns	1.88	0.028	11.85 KB
master	`ObjectExtractorSimpleBody`	net6.0	141ns	0.243ns	0.939ns	0.00393	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	217ns	0.162ns	0.607ns	0.00374	0	272 B
master	`ObjectExtractorSimpleBody`	net472	165ns	0.113ns	0.437ns	0.0446	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	3.03μs	1.14ns	4.41ns	0.0532	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.97μs	1.5ns	5.6ns	0.0498	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	3.84μs	2.57ns	9.96ns	0.602	0.00577	3.8 KB
#5943	`AllCycleSimpleBody`	net6.0	74.1μs	104ns	404ns	0.0741	0	6.01 KB
#5943	`AllCycleSimpleBody`	netcoreapp3.1	62.4μs	57ns	213ns	0.0938	0	6.95 KB
#5943	`AllCycleSimpleBody`	net472	49.1μs	68.9ns	267ns	1.32	0	8.34 KB
#5943	`AllCycleMoreComplexBody`	net6.0	79.2μs	73.5ns	275ns	0.119	0	9.51 KB
#5943	`AllCycleMoreComplexBody`	netcoreapp3.1	68.8μs	109ns	423ns	0.137	0	10.36 KB
#5943	`AllCycleMoreComplexBody`	net472	56μs	47.1ns	170ns	1.88	0.028	11.85 KB
#5943	`ObjectExtractorSimpleBody`	net6.0	149ns	0.192ns	0.745ns	0.00394	0	280 B
#5943	`ObjectExtractorSimpleBody`	netcoreapp3.1	202ns	0.18ns	0.698ns	0.00376	0	272 B
#5943	`ObjectExtractorSimpleBody`	net472	168ns	0.169ns	0.656ns	0.0446	0	281 B
#5943	`ObjectExtractorMoreComplexBody`	net6.0	3.17μs	2.95ns	11ns	0.0528	0	3.78 KB
#5943	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	4.03μs	2.12ns	8.21ns	0.0507	0	3.69 KB
#5943	`ObjectExtractorMoreComplexBody`	net472	3.78μs	1.78ns	6.9ns	0.603	0.00567	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	37.3μs	19.2ns	71.9ns	0.446	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	55.3μs	31.9ns	115ns	0.44	0	32.4 KB
master	`EncodeArgs`	net472	65.2μs	41.3ns	155ns	5.15	0.0652	32.5 KB
master	`EncodeLegacyArgs`	net6.0	71.8μs	20.6ns	77.2ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	105μs	101ns	392ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	151μs	85.1ns	318ns	0.303	0	2.15 KB
#5943	`EncodeArgs`	net6.0	36.9μs	26.4ns	102ns	0.457	0	32.4 KB
#5943	`EncodeArgs`	netcoreapp3.1	54.6μs	25.6ns	99.2ns	0.433	0	32.4 KB
#5943	`EncodeArgs`	net472	65.6μs	30.4ns	110ns	5.14	0.0663	32.5 KB
#5943	`EncodeLegacyArgs`	net6.0	73.7μs	32.2ns	121ns	0	0	2.14 KB
#5943	`EncodeLegacyArgs`	netcoreapp3.1	105μs	154ns	598ns	0	0	2.14 KB
#5943	`EncodeLegacyArgs`	net472	150μs	128ns	494ns	0.301	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	185μs	68.4ns	265ns	0	2.42 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	195μs	158ns	612ns	0	2.37 KB
master	`RunWafRealisticBenchmark`	net472	208μs	97.6ns	352ns	0.311	2.43 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	123μs	76.7ns	297ns	0	1.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	131μs	51.4ns	178ns	0	1.45 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	138μs	37.4ns	145ns	0.207	1.48 KB
#5943	`RunWafRealisticBenchmark`	net6.0	183μs	110ns	426ns	0	2.42 KB
#5943	`RunWafRealisticBenchmark`	netcoreapp3.1	198μs	226ns	875ns	0	2.37 KB
#5943	`RunWafRealisticBenchmark`	net472	209μs	48.1ns	186ns	0.314	2.43 KB
#5943	`RunWafRealisticBenchmarkWithAttack`	net6.0	121μs	38.9ns	151ns	0	1.46 KB
#5943	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	129μs	128ns	478ns	0	1.45 KB
#5943	`RunWafRealisticBenchmarkWithAttack`	net472	139μs	35ns	136ns	0.209	1.48 KB

Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #5943

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1	251.8 KB	264.26 KB	12.46 KB	4.95%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0	253.15 KB	254.56 KB	1.41 KB	0.56%

Fewer allocations 🎉 in #5943

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	59.55 KB	58.9 KB	-656 B	-1.10%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`StringConcatBenchmark`	net6.0	60μs	802ns	7.94μs	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	54.3μs	271ns	1.15μs	42.64 KB
master	`StringConcatBenchmark`	net472	37.6μs	96.7ns	362ns	59.55 KB
master	`StringConcatAspectBenchmark`	net6.0	306μs	1.72μs	11.2μs	253.15 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	333μs	1.87μs	11.7μs	251.8 KB
master	`StringConcatAspectBenchmark`	net472	290μs	6.63μs	64.3μs	278.53 KB
#5943	`StringConcatBenchmark`	net6.0	60μs	844ns	8.39μs	43.44 KB
#5943	`StringConcatBenchmark`	netcoreapp3.1	53.2μs	285ns	1.46μs	42.64 KB
#5943	`StringConcatBenchmark`	net472	37.3μs	178ns	732ns	58.9 KB
#5943	`StringConcatAspectBenchmark`	net6.0	307μs	1.7μs	10.8μs	254.56 KB
#5943	`StringConcatAspectBenchmark`	netcoreapp3.1	345μs	1.9μs	11.7μs	264.26 KB
#5943	`StringConcatAspectBenchmark`	net472	282μs	5.76μs	55.9μs	278.53 KB

andrewlock · 2024-08-26T09:34:37Z

Benchmarks Report for tracer 🐌

Benchmarks for #5943 compared to master:

3 benchmarks are faster, with geometric mean 1.157
1 benchmarks are slower, with geometric mean 1.153
All benchmarks have the same allocations

The following thresholds were used for comparing the benchmark speeds:

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

Allocation changes below 0.5% are ignored.

Benchmark details

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Gen 2	Allocated
master	`StartStopWithChild`	net6.0	7.76μs	43.7ns	303ns	0.0148	0.00738	0	5.43 KB
master	`StartStopWithChild`	netcoreapp3.1	9.78μs	55.1ns	389ns	0.0241	0.00963	0	5.61 KB
master	`StartStopWithChild`	net472	15.8μs	46.9ns	175ns	1.02	0.302	0.103	6.07 KB
#5943	`StartStopWithChild`	net6.0	7.74μs	43.5ns	298ns	0.0153	0.00765	0	5.43 KB
#5943	`StartStopWithChild`	netcoreapp3.1	9.95μs	53.9ns	315ns	0.0193	0.00964	0	5.61 KB
#5943	`StartStopWithChild`	net472	16.1μs	60.8ns	235ns	1.04	0.329	0.0962	6.07 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	476μs	256ns	990ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	645μs	131ns	473ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	839μs	659ns	2.55μs	0.417	3.3 KB
#5943	`WriteAndFlushEnrichedTraces`	net6.0	469μs	224ns	839ns	0	2.7 KB
#5943	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	638μs	269ns	1.04μs	0	2.7 KB
#5943	`WriteAndFlushEnrichedTraces`	net472	834μs	516ns	1.93μ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	187μs	884ns	3.54μs	0.183	18.45 KB
master	`SendRequest`	netcoreapp3.1	207μs	1.1μs	5.91μs	0.209	20.61 KB
master	`SendRequest`	net472	0.0059ns	0.00211ns	0.00818ns	0	0 b
#5943	`SendRequest`	net6.0	180μs	880ns	3.73μs	0.177	18.45 KB
#5943	`SendRequest`	netcoreapp3.1	201μs	704ns	2.44μs	0.205	20.61 KB
#5943	`SendRequest`	net472	0.00212ns	0.000984ns	0.00368ns	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	543μs	1.82μs	6.83μs	0.546	0	0	41.52 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	677μs	3.19μs	14.6μs	0.329	0	0	41.73 KB
master	`WriteAndFlushEnrichedTraces`	net472	875μs	3.56μs	13.8μs	8.13	2.57	0.428	53.27 KB
#5943	`WriteAndFlushEnrichedTraces`	net6.0	567μs	1.56μs	5.84μs	0.561	0	0	41.38 KB
#5943	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	669μs	1.02μs	3.83μs	0.329	0	0	41.79 KB
#5943	`WriteAndFlushEnrichedTraces`	net472	872μs	3.97μs	15.4μs	8.33	2.5	0.417	53.28 KB

Benchmarks.Trace.DbCommandBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5943

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.DbCommandBenchmark.ExecuteNonQuery‑net6.0	1.149	1,345.72	1,170.71

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteNonQuery`	net6.0	1.35μs	1.31ns	5.06ns	0.0142	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.72μs	1.68ns	6.5ns	0.0138	1.02 KB
master	`ExecuteNonQuery`	net472	2.02μs	0.813ns	2.93ns	0.157	987 B
#5943	`ExecuteNonQuery`	net6.0	1.17μs	0.694ns	2.69ns	0.0147	1.02 KB
#5943	`ExecuteNonQuery`	netcoreapp3.1	1.71μs	1.4ns	5.43ns	0.0137	1.02 KB
#5943	`ExecuteNonQuery`	net472	1.96μs	2.16ns	8.09ns	0.156	987 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net6.0	1.19μs	0.497ns	1.86ns	0.0137	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.46μs	0.583ns	2.1ns	0.0132	976 B
master	`CallElasticsearch`	net472	2.47μs	1.59ns	5.97ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.35μs	1.64ns	6.37ns	0.0135	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	0.594ns	2.3ns	0.0142	1.02 KB
master	`CallElasticsearchAsync`	net472	2.51μs	1.25ns	4.67ns	0.166	1.05 KB
#5943	`CallElasticsearch`	net6.0	1.14μs	0.514ns	1.99ns	0.0137	976 B
#5943	`CallElasticsearch`	netcoreapp3.1	1.53μs	2.05ns	7.67ns	0.0131	976 B
#5943	`CallElasticsearch`	net472	2.43μs	2.07ns	8.03ns	0.158	995 B
#5943	`CallElasticsearchAsync`	net6.0	1.37μs	0.899ns	3.36ns	0.0132	952 B
#5943	`CallElasticsearchAsync`	netcoreapp3.1	1.74μs	0.762ns	2.95ns	0.0132	1.02 KB
#5943	`CallElasticsearchAsync`	net472	2.57μs	2.07ns	8.02ns	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.34μs	0.561ns	2.1ns	0.013	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.57μs	0.783ns	2.82ns	0.0127	952 B
master	`ExecuteAsync`	net472	1.73μs	0.546ns	2.04ns	0.145	915 B
#5943	`ExecuteAsync`	net6.0	1.37μs	1.12ns	4.33ns	0.0133	952 B
#5943	`ExecuteAsync`	netcoreapp3.1	1.66μs	2.08ns	8.07ns	0.0123	952 B
#5943	`ExecuteAsync`	net472	1.71μs	0.853ns	3.31ns	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.14μs	0.957ns	3.58ns	0.0311	2.22 KB
master	`SendAsync`	netcoreapp3.1	5.1μs	2.15ns	8.05ns	0.0359	2.76 KB
master	`SendAsync`	net472	7.75μs	2.41ns	9.35ns	0.496	3.15 KB
#5943	`SendAsync`	net6.0	4.24μs	2.39ns	8.94ns	0.0317	2.22 KB
#5943	`SendAsync`	netcoreapp3.1	5.15μs	1.33ns	5.14ns	0.0361	2.76 KB
#5943	`SendAsync`	net472	7.72μs	1.39ns	5.19ns	0.499	3.15 KB

Benchmarks.Trace.ILoggerBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #5943

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.ILoggerBenchmark.EnrichedLog‑netcoreapp3.1	1.159	2,374.65	2,049.67

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.47μs	0.435ns	1.68ns	0.0229	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.38μs	0.732ns	2.74ns	0.0225	1.64 KB
master	`EnrichedLog`	net472	2.71μs	1.55ns	5.99ns	0.249	1.57 KB
#5943	`EnrichedLog`	net6.0	1.59μs	0.728ns	2.72ns	0.0233	1.64 KB
#5943	`EnrichedLog`	netcoreapp3.1	2.05μs	0.986ns	3.69ns	0.0215	1.64 KB
#5943	`EnrichedLog`	net472	2.73μs	2.47ns	9.55ns	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	117μs	134ns	502ns	0.0589	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	121μs	386ns	1.5μs	0	0	4.28 KB
master	`EnrichedLog`	net472	151μs	215ns	832ns	0.682	0.227	4.46 KB
#5943	`EnrichedLog`	net6.0	116μs	197ns	762ns	0.0581	0	4.28 KB
#5943	`EnrichedLog`	netcoreapp3.1	120μs	156ns	605ns	0.061	0	4.28 KB
#5943	`EnrichedLog`	net472	150μs	120ns	417ns	0.675	0.225	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	3.07μs	0.986ns	3.69ns	0.0305	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.39μs	0.973ns	3.77ns	0.0305	2.2 KB
master	`EnrichedLog`	net472	4.85μs	1.03ns	4.01ns	0.32	2.02 KB
#5943	`EnrichedLog`	net6.0	3.05μs	1.18ns	4.57ns	0.0307	2.2 KB
#5943	`EnrichedLog`	netcoreapp3.1	4.25μs	1.72ns	6.42ns	0.0279	2.2 KB
#5943	`EnrichedLog`	net472	4.92μs	1.52ns	5.87ns	0.32	2.02 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net6.0	1.31μs	0.747ns	2.79ns	0.0164	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.75μs	0.965ns	3.48ns	0.0158	1.14 KB
master	`SendReceive`	net472	2.14μs	1.44ns	5.38ns	0.183	1.16 KB
#5943	`SendReceive`	net6.0	1.39μs	1.67ns	6.47ns	0.016	1.14 KB
#5943	`SendReceive`	netcoreapp3.1	1.77μs	1.83ns	7.08ns	0.0151	1.14 KB
#5943	`SendReceive`	net472	2.17μs	1.37ns	5.13ns	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.79μs	1.15ns	4.44ns	0.0224	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.94μs	2.01ns	7.77ns	0.0218	1.65 KB
master	`EnrichedLog`	net472	4.35μs	1.25ns	4.67ns	0.322	2.04 KB
#5943	`EnrichedLog`	net6.0	2.73μs	0.9ns	3.48ns	0.0217	1.6 KB
#5943	`EnrichedLog`	netcoreapp3.1	3.93μs	1.43ns	5.56ns	0.0215	1.65 KB
#5943	`EnrichedLog`	net472	4.32μs	2.56ns	9.56ns	0.323	2.04 KB

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

Slower ⚠️ in #5943

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0	1.153	400.60	461.91

Faster 🎉 in #5943

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0	1.162	537.55	462.72

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	400ns	0.257ns	0.993ns	0.00803	576 B
master	`StartFinishSpan`	netcoreapp3.1	574ns	1.16ns	4.5ns	0.00761	576 B
master	`StartFinishSpan`	net472	618ns	0.706ns	2.73ns	0.0916	578 B
master	`StartFinishScope`	net6.0	537ns	0.549ns	2.05ns	0.00971	696 B
master	`StartFinishScope`	netcoreapp3.1	685ns	1.12ns	4.33ns	0.00929	696 B
master	`StartFinishScope`	net472	809ns	0.641ns	2.48ns	0.104	658 B
#5943	`StartFinishSpan`	net6.0	462ns	0.487ns	1.89ns	0.00814	576 B
#5943	`StartFinishSpan`	netcoreapp3.1	553ns	0.414ns	1.6ns	0.00773	576 B
#5943	`StartFinishSpan`	net472	580ns	0.613ns	2.38ns	0.0916	578 B
#5943	`StartFinishScope`	net6.0	462ns	0.323ns	1.17ns	0.00974	696 B
#5943	`StartFinishScope`	netcoreapp3.1	705ns	0.567ns	2.19ns	0.0091	696 B
#5943	`StartFinishScope`	net472	828ns	1.18ns	4.58ns	0.105	658 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	582ns	0.413ns	1.6ns	0.00987	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	899ns	1.01ns	3.77ns	0.00949	696 B
master	`RunOnMethodBegin`	net472	1.02μs	1.54ns	5.96ns	0.104	658 B
#5943	`RunOnMethodBegin`	net6.0	603ns	0.357ns	1.38ns	0.0098	696 B
#5943	`RunOnMethodBegin`	netcoreapp3.1	887ns	1.58ns	6.11ns	0.00923	696 B
#5943	`RunOnMethodBegin`	net472	1.09μs	1.96ns	7.61ns	0.104	658 B

…DataDog/dd-trace-dotnet into nacho/AvoidValidationExceptions

andrewlock · 2024-08-27T11:44:40Z

+    {
+        try
+        {
+            return request.Headers;


I suspect we call this lots of otherplaces too 😕 For example our scope extraction would presumably also trigger this? Do we need to use the helper in that case as well, or is that safe somehow? 🤔

You don't have this kind of error since 2.54
Taking a close look at the logs, you used to have it in TracingHttpModule.cs, but now you capture all the exceptions.

if (tracer.InternalActiveScope == null) { try { // extract propagated http headers headers = httpRequest.Headers.Wrap(); propagatedContext = SpanContextPropagator.Instance.Extract(headers.Value); } catch (Exception ex) { Log.Error(ex, "Error extracting propagated HTTP headers."); } }

I think that in this piece of code, it would be better to use the RequestDataHelper. I will fix it and take a look closer in other places.

I have made some updates to on the tracinghttpmodule.cs I have not identified other places that use httprequest in net framework where it could happen. The logs also don't show any HttpRequestValidationException out of the already modified classes. I was not sure about AspNetMvcIntegration for instance, but I tried with request after calling ValidateInput and it seems to be fine.

NachoEchevarria · 2024-08-27T14:10:11Z

+                            {
+                                string path = UriHelpers.GetCleanUriPath(url, app.Request.ApplicationPath);
+                                scope.Span.ResourceName = $"{app.Request.HttpMethod.ToUpperInvariant()} {path.ToLowerInvariant()}";
+                            }


I wonder if we should add some resourceName here if we cannot get the url

Yeah, we probably should - I wonder what we should put though 😕 Unless there's an "unsafe" way we can get the URL so that we can at least include the path, I'm not sure what to do 😅 Worst case we could just use app.Request.HttpMethod.ToUpperInvariant() - is that all we can realistically get?

I guess that we could use reflection by calling the internal method GetUnvalidatedPath(), but that's pretty ugly and not very efficient. I have updated the code to add the method name.

GetUnvalidatedPath That actually sounds like the right option to me 😅 We would simply duck-type it and it would work 😉

NachoEchevarria · 2024-09-03T09:01:08Z

Thanks for the reviews and feedback!

…rding (#8611) ## Summary of changes - Remove need for allocating a closure - Fix potential `HttpRequestValidationException` leaking in ASM paths ## Reason for change Calling `HttpRequest.Headers` can throw an `HttpRequestValidationException` if the headers contain "dangerous values". We guarded against that in #5943 by adding a helper, but in #6164 we refactored, and stopped using it. ## Implementation details - Reinstates using `RequestDataHelper.GetHeaders()` instead of calling `request.Headers` directly. - Removes the need for a closure allocation every time you call `SecurityCoordinator.ExtractHeaders()` by allowing you to pass the collection in to the method, and passing that through to the `Func`. Not required, just a minor perf optimization ## Test coverage Meh - correctness should be covered by existing tests. Adding a regression test seems overkill tbh

NachoEchevarria added 2 commits August 14, 2024 17:12

Avoid exception when getting cookies

53673ad

Add headers checks

c0ff68a

Merge branch 'master' into nacho/AvoidValidationExceptions

b105f40

NachoEchevarria changed the title ~~Nacho/avoid validation exceptions~~ [ASM] Avoid unhandled HttpRequestValidationExceptions Aug 26, 2024

NachoEchevarria added the area:asm label Aug 26, 2024

NachoEchevarria added 5 commits August 26, 2024 18:09

Add new tests. Protect iastRequestController

503a6b6

Merge branch 'nacho/AvoidValidationExceptions' of https://github.com/…

c8fe31f

…DataDog/dd-trace-dotnet into nacho/AvoidValidationExceptions

Undo security coordinator core

fec81e3

Undo not needed change

c42a6e3

Change test names

6a597cf

NachoEchevarria marked this pull request as ready for review August 27, 2024 11:37

NachoEchevarria requested review from a team as code owners August 27, 2024 11:37

Update HttpRequestExtensions.Core.cs

5dc8ba8

andrewlock approved these changes Aug 27, 2024

View reviewed changes

update tracinghttpmodule, securitycoordinator and headerAnalyzer

a3d3477

NachoEchevarria commented Aug 27, 2024

View reviewed changes

NachoEchevarria and others added 2 commits August 29, 2024 10:20

Fill resource name when an exception is thrown

fc3f8c4

Merge branch 'master' into nacho/AvoidValidationExceptions

ddd6235

dromanol approved these changes Sep 3, 2024

View reviewed changes

NachoEchevarria merged commit 053190c into master Sep 3, 2024

NachoEchevarria deleted the nacho/AvoidValidationExceptions branch September 3, 2024 09:01

github-actions Bot added this to the vNext-v3 milestone Sep 3, 2024

andrewlock added the identified-by:telemetry label Sep 16, 2024

andrewlock mentioned this pull request May 12, 2026

Fix ASM accessing HttpRequest.Headers on .NET Framework without guarding #8611

Merged

Uh oh!

Conversation

NachoEchevarria commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

datadog-ddstaging Bot commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

❌ Failed Tests (1)

Uh oh!

andrewlock commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Execution-Time Benchmarks Report ⏱️

Uh oh!

andrewlock commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks Report for appsec 🐌

Benchmark details

Raw results

Raw results

Raw results

More allocations ⚠️ in #5943

Fewer allocations 🎉 in #5943

Raw results

Uh oh!

andrewlock commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #5943

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #5943

Raw results

Raw results

Raw results

Raw results

Raw results

Slower ⚠️ in #5943

Faster 🎉 in #5943

Raw results

Raw results

Uh oh!

andrewlock Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

NachoEchevarria Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

NachoEchevarria Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

NachoEchevarria Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

andrewlock Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

NachoEchevarria Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

andrewlock Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

NachoEchevarria commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

NachoEchevarria commented Aug 26, 2024 •

edited

Loading

datadog-ddstaging Bot commented Aug 26, 2024 •

edited

Loading

andrewlock commented Aug 26, 2024 •

edited

Loading

andrewlock commented Aug 26, 2024 •

edited

Loading

andrewlock commented Aug 26, 2024 •

edited

Loading