[IAST] Safeguard Method Replace aspects with try/catch #5841

daniel-romano-DD · 2024-08-01T21:51:40Z

Summary of changes

Covered all AspectMethodReplace aspects with try catch clauses to ensure no crash will bubble up to client, following new analyzer rules.

Disabled some weird casts and processes to support some functions not present en NetCore 2.0, but present in 2.1 (netstandard2 assembly is loaded in netcore2.1 apps).

Disabled some overloads receiving generic undefined arguments until proper callsite support is implemented.

Reason for change

SSI will make the tracer enabled for a lot more of services when available. We must ensure we do not break any of them, and if so, that we provide a fast answer.

Implementation details

Apply analyzer suggestions adding a try / catch clause in all Methodreplace aspects

Test coverage

Other details

andrewlock · 2024-08-01T22:17:25Z

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 (5841) - mean (75ms)  : 63, 86
     .   : milestone, 75,
    master - mean (73ms)  : 64, 82
     .   : milestone, 73,

    section CallTarget+Inlining+NGEN
    This PR (5841) - mean (1,068ms)  : 1049, 1088
     .   : milestone, 1068,
    master - mean (1,061ms)  : 1045, 1078
     .   : milestone, 1061,

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

    section CallTarget+Inlining+NGEN
    This PR (5841) - mean (756ms)  : 729, 782
     .   : milestone, 756,
    master - mean (746ms)  : 728, 763
     .   : milestone, 746,

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

    section CallTarget+Inlining+NGEN
    This PR (5841) - mean (742ms)  : 636, 848
     .   : milestone, 742,
    master - mean (702ms)  : 685, 718
     .   : milestone, 702,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5841) - mean (193ms)  : 190, 196
     .   : milestone, 193,
    master - mean (192ms)  : 189, 195
     .   : milestone, 192,

    section CallTarget+Inlining+NGEN
    This PR (5841) - mean (1,174ms)  : 1149, 1199
     .   : milestone, 1174,
    master - mean (1,163ms)  : 1140, 1186
     .   : milestone, 1163,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5841) - mean (278ms)  : 271, 285
     .   : milestone, 278,
    master - mean (276ms)  : 271, 281
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (5841) - mean (922ms)  : 894, 951
     .   : milestone, 922,
    master - mean (913ms)  : 894, 933
     .   : milestone, 913,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5841) - mean (266ms)  : 262, 270
     .   : milestone, 266,
    master - mean (266ms)  : 263, 269
     .   : milestone, 266,

    section CallTarget+Inlining+NGEN
    This PR (5841) - mean (904ms)  : 878, 930
     .   : milestone, 904,
    master - mean (918ms)  : 856, 979
     .   : milestone, 918,

datadog-ddstaging · 2024-08-01T22:22:37Z

Datadog Report

Branch report: dani/iast/callsite_security_replace
Commit report: c6b25ee
Test service: dd-trace-dotnet

✅ 0 Failed, 425962 Passed, 2656 Skipped, 28h 2m 39.79s Total Time

andrewlock · 2024-08-02T00:13:26Z

Benchmarks Report for appsec 🐌

Benchmarks for #5841 compared to master:

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

Faster 🎉 in #5841

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1	1.276	249.89	195.90

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	72.8μs	112ns	433ns	0.0724	0	6 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	61.6μs	55.2ns	207ns	0.0922	0	6.95 KB
master	`AllCycleSimpleBody`	net472	47.4μs	27.3ns	102ns	1.31	0	8.33 KB
master	`AllCycleMoreComplexBody`	net6.0	78.8μs	164ns	635ns	0.119	0	9.51 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	68.8μs	103ns	400ns	0.137	0	10.36 KB
master	`AllCycleMoreComplexBody`	net472	54.7μs	51.1ns	184ns	1.86	0.0274	11.85 KB
master	`ObjectExtractorSimpleBody`	net6.0	144ns	0.122ns	0.457ns	0.00397	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	250ns	0.148ns	0.535ns	0.00375	0	272 B
master	`ObjectExtractorSimpleBody`	net472	167ns	0.274ns	1.06ns	0.0446	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	2.99μs	1.2ns	4.64ns	0.0526	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.91μs	1.79ns	6.95ns	0.051	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	3.79μs	4.94ns	18.5ns	0.602	0.0057	3.8 KB
#5841	`AllCycleSimpleBody`	net6.0	73.5μs	122ns	438ns	0.0744	0	6 KB
#5841	`AllCycleSimpleBody`	netcoreapp3.1	62.4μs	84.8ns	328ns	0.0932	0	6.95 KB
#5841	`AllCycleSimpleBody`	net472	48.2μs	67ns	259ns	1.32	0	8.34 KB
#5841	`AllCycleMoreComplexBody`	net6.0	78.7μs	108ns	417ns	0.118	0	9.51 KB
#5841	`AllCycleMoreComplexBody`	netcoreapp3.1	69.6μs	102ns	381ns	0.138	0	10.37 KB
#5841	`AllCycleMoreComplexBody`	net472	54.9μs	93.1ns	360ns	1.86	0.0274	11.85 KB
#5841	`ObjectExtractorSimpleBody`	net6.0	143ns	0.162ns	0.627ns	0.00397	0	280 B
#5841	`ObjectExtractorSimpleBody`	netcoreapp3.1	196ns	0.17ns	0.658ns	0.00366	0	272 B
#5841	`ObjectExtractorSimpleBody`	net472	167ns	0.166ns	0.6ns	0.0446	0	281 B
#5841	`ObjectExtractorMoreComplexBody`	net6.0	3.14μs	2.4ns	9.28ns	0.0533	0	3.78 KB
#5841	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	4.07μs	2.6ns	10.1ns	0.0513	0	3.69 KB
#5841	`ObjectExtractorMoreComplexBody`	net472	3.79μs	2.38ns	9.23ns	0.602	0.00568	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.8μs	25.2ns	97.6ns	0.457	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	54.3μs	28.3ns	110ns	0.434	0	32.4 KB
master	`EncodeArgs`	net472	66.4μs	32.4ns	117ns	5.16	0.0662	32.5 KB
master	`EncodeLegacyArgs`	net6.0	78μs	26.5ns	103ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	105μs	416ns	1.61μs	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	155μs	81.5ns	316ns	0.311	0	2.15 KB
#5841	`EncodeArgs`	net6.0	36.5μs	14.1ns	52.9ns	0.456	0	32.4 KB
#5841	`EncodeArgs`	netcoreapp3.1	54.5μs	52.8ns	205ns	0.436	0	32.4 KB
#5841	`EncodeArgs`	net472	65.7μs	31.9ns	124ns	5.13	0.0658	32.5 KB
#5841	`EncodeLegacyArgs`	net6.0	71.4μs	28.2ns	109ns	0	0	2.14 KB
#5841	`EncodeLegacyArgs`	netcoreapp3.1	107μs	113ns	439ns	0	0	2.14 KB
#5841	`EncodeLegacyArgs`	net472	154μs	43.5ns	163ns	0.309	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	21.2ns	76.3ns	0	2.42 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	195μs	278ns	1.07μs	0	2.37 KB
master	`RunWafRealisticBenchmark`	net472	211μs	106ns	381ns	0.315	2.43 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	123μs	29.9ns	116ns	0	1.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	129μs	75.7ns	283ns	0	1.45 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	140μs	36.8ns	143ns	0.21	1.48 KB
#5841	`RunWafRealisticBenchmark`	net6.0	186μs	74.4ns	278ns	0	2.42 KB
#5841	`RunWafRealisticBenchmark`	netcoreapp3.1	198μs	136ns	508ns	0	2.37 KB
#5841	`RunWafRealisticBenchmark`	net472	209μs	45.7ns	177ns	0.312	2.43 KB
#5841	`RunWafRealisticBenchmarkWithAttack`	net6.0	123μs	43.2ns	156ns	0	1.46 KB
#5841	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	130μs	57.6ns	216ns	0	1.45 KB
#5841	`RunWafRealisticBenchmarkWithAttack`	net472	140μs	48.1ns	186ns	0.209	1.48 KB

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

More allocations ⚠️ in #5841

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1	252.69 KB	262.51 KB	9.82 KB	3.89%

Fewer allocations 🎉 in #5841

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	62.46 KB	57.9 KB	-4.56 KB	-7.30%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`StringConcatBenchmark`	net6.0	61.2μs	702ns	6.95μs	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	52.6μs	247ns	1.08μs	42.64 KB
master	`StringConcatBenchmark`	net472	37.6μs	80.5ns	301ns	62.46 KB
master	`StringConcatAspectBenchmark`	net6.0	275μs	5.22μs	51.7μs	254.11 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	337μs	1.85μs	14.7μs	252.69 KB
master	`StringConcatAspectBenchmark`	net472	303μs	7.47μs	73.9μs	278.53 KB
#5841	`StringConcatBenchmark`	net6.0	61.1μs	676ns	6.62μs	43.44 KB
#5841	`StringConcatBenchmark`	netcoreapp3.1	53.5μs	251ns	1.06μs	42.64 KB
#5841	`StringConcatBenchmark`	net472	38.2μs	176ns	680ns	57.9 KB
#5841	`StringConcatAspectBenchmark`	net6.0	316μs	1.76μs	10.6μs	255.09 KB
#5841	`StringConcatAspectBenchmark`	netcoreapp3.1	345μs	1.82μs	9.09μs	262.51 KB
#5841	`StringConcatAspectBenchmark`	net472	287μs	6.11μs	59.8μs	278.53 KB

andrewlock · 2024-08-02T00:21:19Z

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 (5841) (11.551M)   : 0, 11551201
    master (11.276M)   : 0, 11275684
    benchmarks/2.9.0 (11.335M)   : 0, 11335046

    section Automatic
    This PR (5841) (7.792M)   : 0, 7791629
    master (7.474M)   : 0, 7473550
    benchmarks/2.9.0 (8.124M)   : 0, 8123605

    section Trace stats
    master (7.762M)   : 0, 7762495

    section Manual
    master (11.218M)   : 0, 11218180

    section Manual + Automatic
    This PR (5841) (7.174M)   : 0, 7174091
    master (6.967M)   : 0, 6966824

    section DD_TRACE_ENABLED=0
    master (10.397M)   : 0, 10396799

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5841) (9.303M)   : 0, 9302776

    section Automatic
    This PR (5841) (6.679M)   : 0, 6678749

    section Manual + Automatic
    This PR (5841) (5.978M)   : 0, 5978055

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5841) (10.212M)   : 0, 10211794
    benchmarks/2.9.0 (10.092M)   : 0, 10091577

    section Automatic
    This PR (5841) (7.018M)   : 0, 7018352
    benchmarks/2.9.0 (7.470M)   : 0, 7469858

    section Manual + Automatic
    This PR (5841) (6.539M)   : 0, 6538583

andrewlock · 2024-08-02T00:22:21Z

Benchmarks Report for tracer 🐌

Benchmarks for #5841 compared to master:

2 benchmarks are faster, with geometric mean 1.164
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.64μs	42.2ns	274ns	0.0117	0.00392	0	5.42 KB
master	`StartStopWithChild`	netcoreapp3.1	9.74μs	52ns	260ns	0.0144	0.00482	0	5.62 KB
master	`StartStopWithChild`	net472	16.2μs	51.1ns	198ns	1.02	0.299	0.0945	6.07 KB
#5841	`StartStopWithChild`	net6.0	7.61μs	43.1ns	302ns	0.019	0.00759	0	5.43 KB
#5841	`StartStopWithChild`	netcoreapp3.1	9.92μs	53.6ns	303ns	0.0187	0.00935	0	5.61 KB
#5841	`StartStopWithChild`	net472	16μs	68.5ns	265ns	1.04	0.311	0.0956	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	475μs	247ns	924ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	639μs	675ns	2.61μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	829μs	1.02μs	3.94μs	0.417	3.3 KB
#5841	`WriteAndFlushEnrichedTraces`	net6.0	452μs	139ns	522ns	0	2.7 KB
#5841	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	643μs	315ns	1.18μs	0	2.7 KB
#5841	`WriteAndFlushEnrichedTraces`	net472	838μs	1.38μs	5.15μs	0.414	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	210μs	1.23μs	11.4μs	0.213	18.45 KB
master	`SendRequest`	netcoreapp3.1	231μs	1.33μs	10.4μs	0.22	20.61 KB
master	`SendRequest`	net472	0.0304ns	0.00606ns	0.0235ns	0	0 b
#5841	`SendRequest`	net6.0	200μs	1.17μs	11.5μs	0.205	18.45 KB
#5841	`SendRequest`	netcoreapp3.1	244μs	1.46μs	14.4μs	0.252	20.61 KB
#5841	`SendRequest`	net472	0.00345ns	0.000938ns	0.00351ns	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	2.35μs	9.09μs	0.546	0	0	41.49 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	705μs	3.88μs	22.3μs	0.342	0	0	41.77 KB
master	`WriteAndFlushEnrichedTraces`	net472	863μs	3.27μs	12.7μs	8.13	2.57	0.428	53.3 KB
#5841	`WriteAndFlushEnrichedTraces`	net6.0	558μs	1.89μs	6.83μs	0.563	0	0	41.54 KB
#5841	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	709μs	3.94μs	26.1μs	0.347	0	0	41.83 KB
#5841	`WriteAndFlushEnrichedTraces`	net472	869μs	3.15μs	12.2μs	8.25	2.6	0.434	53.31 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteNonQuery`	net6.0	1.24μs	1.19ns	4.62ns	0.0142	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.73μs	1.86ns	6.97ns	0.0137	1.02 KB
master	`ExecuteNonQuery`	net472	2.01μs	1.82ns	7.07ns	0.157	987 B
#5841	`ExecuteNonQuery`	net6.0	1.22μs	0.856ns	3.2ns	0.0142	1.02 KB
#5841	`ExecuteNonQuery`	netcoreapp3.1	1.7μs	1.27ns	4.76ns	0.0136	1.02 KB
#5841	`ExecuteNonQuery`	net472	1.98μs	2.12ns	8.23ns	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.26μs	0.497ns	1.86ns	0.0134	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.51μs	0.979ns	3.66ns	0.0131	976 B
master	`CallElasticsearch`	net472	2.51μs	2.63ns	10.2ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.26μs	0.705ns	2.64ns	0.0133	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.63μs	0.782ns	2.93ns	0.0139	1.02 KB
master	`CallElasticsearchAsync`	net472	2.69μs	3.12ns	12.1ns	0.167	1.05 KB
#5841	`CallElasticsearch`	net6.0	1.18μs	1.81ns	6.75ns	0.0135	976 B
#5841	`CallElasticsearch`	netcoreapp3.1	1.56μs	1.32ns	5.11ns	0.0134	976 B
#5841	`CallElasticsearch`	net472	2.59μs	2.58ns	9.98ns	0.158	995 B
#5841	`CallElasticsearchAsync`	net6.0	1.32μs	0.739ns	2.86ns	0.0132	952 B
#5841	`CallElasticsearchAsync`	netcoreapp3.1	1.6μs	2.61ns	9.06ns	0.0144	1.02 KB
#5841	`CallElasticsearchAsync`	net472	2.66μs	2.78ns	10.8ns	0.166	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.24μs	0.461ns	1.72ns	0.0136	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.69μs	1.52ns	5.87ns	0.0127	952 B
master	`ExecuteAsync`	net472	1.73μs	0.839ns	3.25ns	0.145	915 B
#5841	`ExecuteAsync`	net6.0	1.26μs	0.458ns	1.78ns	0.0132	952 B
#5841	`ExecuteAsync`	netcoreapp3.1	1.6μs	1.1ns	4.11ns	0.0128	952 B
#5841	`ExecuteAsync`	net472	1.76μs	0.865ns	3ns	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.17μs	1.63ns	5.66ns	0.0297	2.22 KB
master	`SendAsync`	netcoreapp3.1	5.22μs	5.4ns	20.9ns	0.0365	2.76 KB
master	`SendAsync`	net472	7.77μs	2.91ns	11.3ns	0.499	3.15 KB
#5841	`SendAsync`	net6.0	4.13μs	1.24ns	4.82ns	0.031	2.22 KB
#5841	`SendAsync`	netcoreapp3.1	5.19μs	2.22ns	8.58ns	0.0362	2.76 KB
#5841	`SendAsync`	net472	7.76μs	3.26ns	12.6ns	0.496	3.15 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net6.0	1.41μs	0.779ns	3.02ns	0.0226	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.17μs	0.909ns	3.52ns	0.0228	1.64 KB
master	`EnrichedLog`	net472	2.61μs	1.27ns	4.91ns	0.249	1.57 KB
#5841	`EnrichedLog`	net6.0	1.5μs	2.23ns	8.63ns	0.0232	1.64 KB
#5841	`EnrichedLog`	netcoreapp3.1	2.16μs	1.17ns	4.4ns	0.0225	1.64 KB
#5841	`EnrichedLog`	net472	2.77μs	1.39ns	5.4ns	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	114μs	140ns	541ns	0.0569	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	122μs	201ns	777ns	0	0	4.28 KB
master	`EnrichedLog`	net472	148μs	129ns	499ns	0.663	0.221	4.46 KB
#5841	`EnrichedLog`	net6.0	115μs	177ns	612ns	0.0582	0	4.28 KB
#5841	`EnrichedLog`	netcoreapp3.1	119μs	367ns	1.37μs	0	0	4.28 KB
#5841	`EnrichedLog`	net472	150μs	209ns	808ns	0.67	0.223	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.06μs	0.957ns	3.7ns	0.0306	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.13μs	1.03ns	4ns	0.0309	2.2 KB
master	`EnrichedLog`	net472	4.84μs	1.05ns	4.07ns	0.319	2.02 KB
#5841	`EnrichedLog`	net6.0	3.09μs	1.27ns	4.56ns	0.031	2.2 KB
#5841	`EnrichedLog`	netcoreapp3.1	4.3μs	1.9ns	7.35ns	0.028	2.2 KB
#5841	`EnrichedLog`	net472	4.85μs	1.55ns	5.79ns	0.32	2.02 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`SendReceive`	net6.0	1.34μs	0.529ns	1.98ns	0.0161	0	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.77μs	0.708ns	2.74ns	0.015	0	1.14 KB
master	`SendReceive`	net472	2.11μs	0.648ns	2.51ns	0.183	0.00105	1.16 KB
#5841	`SendReceive`	net6.0	1.29μs	1.44ns	5.6ns	0.0162	0	1.14 KB
#5841	`SendReceive`	netcoreapp3.1	1.76μs	0.974ns	3.77ns	0.0155	0	1.14 KB
#5841	`SendReceive`	net472	2.14μs	1.02ns	3.94ns	0.183	0.00106	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.66μs	0.924ns	3.58ns	0.0225	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.74μs	1.54ns	5.97ns	0.0223	1.65 KB
master	`EnrichedLog`	net472	4.48μs	3.59ns	13.4ns	0.323	2.04 KB
#5841	`EnrichedLog`	net6.0	2.79μs	1.54ns	5.76ns	0.0224	1.6 KB
#5841	`EnrichedLog`	netcoreapp3.1	3.86μs	2.04ns	7.89ns	0.0212	1.65 KB
#5841	`EnrichedLog`	net472	4.25μs	3.22ns	12.5ns	0.323	2.04 KB

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

Faster 🎉 in #5841

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0	1.118	558.76	499.60

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	407ns	0.192ns	0.742ns	0.00818	576 B
master	`StartFinishSpan`	netcoreapp3.1	560ns	0.375ns	1.4ns	0.00765	576 B
master	`StartFinishSpan`	net472	680ns	0.568ns	2.2ns	0.0917	578 B
master	`StartFinishScope`	net6.0	564ns	2.31ns	8.94ns	0.0098	696 B
master	`StartFinishScope`	netcoreapp3.1	682ns	0.69ns	2.58ns	0.00946	696 B
master	`StartFinishScope`	net472	907ns	4.16ns	16.1ns	0.104	658 B
#5841	`StartFinishSpan`	net6.0	399ns	0.108ns	0.388ns	0.00817	576 B
#5841	`StartFinishSpan`	netcoreapp3.1	610ns	0.272ns	1.05ns	0.00771	576 B
#5841	`StartFinishSpan`	net472	622ns	0.272ns	1.02ns	0.0916	578 B
#5841	`StartFinishScope`	net6.0	499ns	0.317ns	1.23ns	0.00983	696 B
#5841	`StartFinishScope`	netcoreapp3.1	702ns	0.276ns	1.03ns	0.0095	696 B
#5841	`StartFinishScope`	net472	867ns	0.516ns	2ns	0.104	658 B

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

Faster 🎉 in #5841

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0	1.211	736.64	608.53

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	736ns	0.281ns	1.05ns	0.00957	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	857ns	0.266ns	0.958ns	0.00923	696 B
master	`RunOnMethodBegin`	net472	1.13μs	0.57ns	2.21ns	0.104	658 B
#5841	`RunOnMethodBegin`	net6.0	609ns	0.379ns	1.37ns	0.00962	696 B
#5841	`RunOnMethodBegin`	netcoreapp3.1	916ns	0.47ns	1.7ns	0.00938	696 B
#5841	`RunOnMethodBegin`	net472	1.13μs	2.8ns	10.9ns	0.104	658 B

andrewlock

LGTM, I'm happy if you are!

The only ones I wonder about are where we're trying to calculate the length of the stringbuilder before we append to it, I worry that it could be a source of bugs, and wonder if it's just safer to use what we were doing before?

Other than that, I'm happy if you are, thanks!

tracer/src/Datadog.Trace.Tools.Analyzers/AspectAnalyzers/BeforeAfterAspectAnalyzer.cs

tracer/src/Datadog.Trace.Tools.Analyzers/AspectAnalyzers/ReplaceAspectAnalyzer.cs

tracer/src/Datadog.Trace/Iast/Aspects/System.Net/WebUtilityAspect.cs

tracer/src/Datadog.Trace/Iast/Aspects/System.Text/StringBuilderAspects.cs

andrewlock · 2024-08-02T12:02:29Z

tracer/src/Datadog.Trace/Iast/Aspects/System/StringAspects.cs

        return result;
    }

-#if !NETFRAMEWORK
+#if NETCOREAPP2_1_OR_GREATER


nit: we should probably stick to NETCOREAPP

Suggested change

#if NETCOREAPP2_1_OR_GREATER

#if NETCOREAPP

The problem here is netcoreapp includes netcoreapp2.0, and many overloads are missing there and available from 2.1 (even we don't compile for that platform). This could be netcoreapp3_1_or_greater, but I don't loose the faith in changing netstandard2 for netcoreapp2.1 someday

netcoreapp includes netcoreapp2.0

I see, so the concern is we'll add a .NET Core 2.0 target, then this would break. TBH. I don't think we care about that, because it's never going to happen 😄 But it's a nit anyway so all good

...grations/Samples.InstrumentedTests/Vulnerabilities/StringBuilder/StringBuilderRemoveTests.cs

NachoEchevarria · 2024-08-05T10:27:19Z

...lications/integrations/Samples.InstrumentedTests/Vulnerabilities/String/StringConcatTests.cs

@@ -485,22 +496,22 @@ public void GivenATaintedObject_WhenCallingConcatWith4ObjectParams_ResultIsTaint
        AssertTaintedFormatWithOriginalCallCheck(":+-TAINTED2-+:concat:+-tainted-+:concat2", String.Concat(taintedValue2, (object)"concat", taintedValue, (object)"concat2"), () => String.Concat(taintedValue2, (object)"concat", taintedValue, (object)"concat2"));
    }

-    [Fact]
+    [Fact(Skip = "Aspect disabled until undefined generics are supported")]


What is the issue with the undefined generics? Until now, they were supported, right? but there must be a strong reason to stop supporting them.

They are not supported in instrumentation. We just added heuristic code to circumvent this, causing potential crashes. As the purpose of this PR is to add security to our code, these overloads were removed until a proper support is implemented.

Covered all `AspectMethodReplace` aspects with try catch clauses to ensure no crash will bubble up to client, following new analyzer rules. Disabled some weird casts and processes to support some functions not present en NetCore 2.0, but present in 2.1 (netstandard2 assembly is loaded in netcore2.1 apps). Disabled some overloads receiving generic undefined arguments until proper callsite support is implemented. SSI will make the tracer enabled for a lot more of services when available. We must ensure we do not break any of them, and if so, that we provide a fast answer. Apply analyzer suggestions adding a try / catch clause in all `Methodreplace` aspects

…5855) ## Summary of changes Covered all `AspectMethodReplace` aspects with try catch clauses to ensure no crash will bubble up to client, following new analyzer rules. Disabled some weird casts and processes to support some functions not present en NetCore 2.0, but present in 2.1 (netstandard2 assembly is loaded in netcore2.1 apps). Disabled some overloads receiving generic undefined arguments until proper callsite support is implemented. ## Reason for change SSI will make the tracer enabled for a lot more of services when available. We must ensure we do not break any of them, and if so, that we provide a fast answer. ## Implementation details Apply analyzer suggestions adding a try / catch clause in all `Methodreplace` aspects

daniel-romano-DD requested review from a team as code owners August 1, 2024 21:51

daniel-romano-DD added area:asm area:asm-iast labels Aug 1, 2024

daniel-romano-DD force-pushed the dani/iast/callsite_security_replace branch from b5a02be to a2cf4e2 Compare August 1, 2024 22:41

daniel-romano-DD force-pushed the dani/iast/callsite_security_replace branch 3 times, most recently from 6baff6a to 70d74af Compare August 2, 2024 08:59

andrewlock changed the base branch from master to dani/iast/callsite_security August 2, 2024 09:27

andrewlock approved these changes Aug 2, 2024

View reviewed changes

lucaspimentel changed the title ~~[IAST] Method Replace aspects securized~~ [IAST] Safeguard Method Replace aspects with try/catch Aug 2, 2024

daniel-romano-DD force-pushed the dani/iast/callsite_security_replace branch from 82d3fc0 to 9de6130 Compare August 2, 2024 19:49

NachoEchevarria reviewed Aug 5, 2024

View reviewed changes

NachoEchevarria approved these changes Aug 5, 2024

View reviewed changes

Base automatically changed from dani/iast/callsite_security to master August 5, 2024 17:05

daniel-romano-DD added 4 commits August 5, 2024 19:12

CallSite replace aspects securization

2317eac

Minor fixes

cb89e7c

Updated overload exclusions in tests

543b947

Fixed now unsupported tests for netcore 3

c6b25ee

daniel-romano-DD force-pushed the dani/iast/callsite_security_replace branch from 69a372d to c6b25ee Compare August 5, 2024 17:12

daniel-romano-DD merged commit 8a6b8e2 into master Aug 6, 2024
70 checks passed

daniel-romano-DD deleted the dani/iast/callsite_security_replace branch August 6, 2024 07:37

github-actions bot added this to the vNext-v3 milestone Aug 6, 2024

robertpi mentioned this pull request Aug 7, 2024

[IAST] Safeguard Method Replace aspects with try/catch (#5841 -> v2) #5855

Merged

[IAST] Safeguard Method Replace aspects with try/catch #5841

[IAST] Safeguard Method Replace aspects with try/catch #5841

Conversation

daniel-romano-DD commented Aug 1, 2024 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

andrewlock commented Aug 1, 2024 • edited Loading

Execution-Time Benchmarks Report ⏱️

datadog-ddstaging bot commented Aug 1, 2024 • edited Loading

Datadog Report

andrewlock commented Aug 2, 2024 • edited Loading

Benchmarks Report for appsec 🐌

Benchmark details

Faster 🎉 in #5841

Raw results

Raw results

Raw results

More allocations ⚠️ in #5841

Fewer allocations 🎉 in #5841

Raw results

andrewlock commented Aug 2, 2024 • edited Loading

Throughput/Crank Report ⚡

andrewlock commented Aug 2, 2024 • edited Loading

Benchmarks Report for tracer 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #5841

Raw results

Faster 🎉 in #5841

Raw results

andrewlock left a comment

Choose a reason for hiding this comment

andrewlock Aug 2, 2024

Choose a reason for hiding this comment

daniel-romano-DD Aug 2, 2024

Choose a reason for hiding this comment

andrewlock Aug 5, 2024

Choose a reason for hiding this comment

NachoEchevarria Aug 5, 2024

Choose a reason for hiding this comment

daniel-romano-DD Aug 5, 2024

Choose a reason for hiding this comment

daniel-romano-DD commented Aug 1, 2024 •

edited

Loading

andrewlock commented Aug 1, 2024 •

edited

Loading

datadog-ddstaging bot commented Aug 1, 2024 •

edited

Loading

andrewlock commented Aug 2, 2024 •

edited

Loading

andrewlock commented Aug 2, 2024 •

edited

Loading

andrewlock commented Aug 2, 2024 •

edited

Loading