[IAST] Taint values coming from database by dromanol · Pull Request #5804 · DataDog/dd-trace-dotnet

dromanol · 2024-07-24T18:02:03Z

Summary of changes

Instrumented GetValue and GetString in DbDataReader to taint values coming from database

Reason for change

In order to detect vulnerabilities like Stored XSS and Stored SQLI we need to keep track of the values delivered by database queries
RFC here

Implementation details

Added CallTargets for DbDataReader.GetValue and DbDataReader.GetString in ADO.NET
Related PRs:
https://github.com/DataDog/dd-go/pull/144940
https://github.com/DataDog/experimental/pull/3556

Test coverage

Added instrumentation test for db tainting, unit tests for tainting config logic and integration tests for Stored XSS and Stored SQLI
Added test to ensure an injection different from XSS and SQLI is not triggered if tainted source comes entirely from DB

Other details

andrewlock · 2024-07-24T18:28:55Z

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

datadog-ddstaging · 2024-07-24T18:31:21Z

Datadog Report

Branch report: dani/iast/taint_db
Commit report: d603b70
Test service: dd-trace-dotnet

✅ 0 Failed, 347526 Passed, 1788 Skipped, 15h 15m 59.3s Total Time

andrewlock · 2024-07-24T19:00:10Z

Benchmarks Report for appsec 🐌

Benchmarks for #5804 compared to master:

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

Faster 🎉 in #5804

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1	1.263	253.22	200.42
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorMoreComplexBody‑net472	1.170	4,381.92	3,743.88

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	74.4μs	121ns	470ns	0.0747	0	6 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	62.8μs	79.7ns	309ns	0.0934	0	6.95 KB
master	`AllCycleSimpleBody`	net472	49.6μs	70ns	262ns	1.3	0	8.34 KB
master	`AllCycleMoreComplexBody`	net6.0	79.7μs	84.2ns	304ns	0.119	0	9.51 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	70.6μs	125ns	466ns	0.142	0	10.36 KB
master	`AllCycleMoreComplexBody`	net472	56.8μs	43ns	161ns	1.87	0.0284	11.85 KB
master	`ObjectExtractorSimpleBody`	net6.0	148ns	0.127ns	0.49ns	0.00391	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	253ns	0.188ns	0.703ns	0.0037	0	272 B
master	`ObjectExtractorSimpleBody`	net472	235ns	0.151ns	0.566ns	0.0445	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	3.09μs	0.793ns	2.86ns	0.0525	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.99μs	2.63ns	9.86ns	0.05	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	4.38μs	1.88ns	7.04ns	0.603	0.00656	3.8 KB
#5804	`AllCycleSimpleBody`	net6.0	75μs	98.4ns	381ns	0.0758	0	6 KB
#5804	`AllCycleSimpleBody`	netcoreapp3.1	63.6μs	84.8ns	306ns	0.0638	0	6.95 KB
#5804	`AllCycleSimpleBody`	net472	48.8μs	71.6ns	277ns	1.32	0	8.34 KB
#5804	`AllCycleMoreComplexBody`	net6.0	81.8μs	114ns	441ns	0.119	0	9.51 KB
#5804	`AllCycleMoreComplexBody`	netcoreapp3.1	71μs	129ns	482ns	0.107	0	10.37 KB
#5804	`AllCycleMoreComplexBody`	net472	56.5μs	83.6ns	324ns	1.86	0.0282	11.85 KB
#5804	`ObjectExtractorSimpleBody`	net6.0	144ns	0.296ns	1.11ns	0.00394	0	280 B
#5804	`ObjectExtractorSimpleBody`	netcoreapp3.1	201ns	0.319ns	1.19ns	0.00367	0	272 B
#5804	`ObjectExtractorSimpleBody`	net472	219ns	0.173ns	0.625ns	0.0446	0	281 B
#5804	`ObjectExtractorMoreComplexBody`	net6.0	3.09μs	2.47ns	9.57ns	0.0525	0	3.78 KB
#5804	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	4.05μs	1.72ns	6.42ns	0.051	0	3.69 KB
#5804	`ObjectExtractorMoreComplexBody`	net472	3.74μs	1.48ns	5.53ns	0.602	0.00564	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μs	30.3ns	117ns	0.458	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	54.4μs	21.4ns	82.9ns	0.435	0	32.4 KB
master	`EncodeArgs`	net472	66.1μs	39.6ns	143ns	5.14	0.0664	32.5 KB
master	`EncodeLegacyArgs`	net6.0	71μs	26.9ns	97ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	105μs	38.9ns	135ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	152μs	109ns	423ns	0.302	0	2.15 KB
#5804	`EncodeArgs`	net6.0	38.9μs	27.4ns	106ns	0.451	0	32.4 KB
#5804	`EncodeArgs`	netcoreapp3.1	54.8μs	29.7ns	115ns	0.438	0	32.4 KB
#5804	`EncodeArgs`	net472	65.7μs	41.4ns	160ns	5.14	0.0659	32.5 KB
#5804	`EncodeLegacyArgs`	net6.0	73.9μs	346ns	1.29μs	0	0	2.14 KB
#5804	`EncodeLegacyArgs`	netcoreapp3.1	110μs	188ns	728ns	0	0	2.14 KB
#5804	`EncodeLegacyArgs`	net472	151μs	104ns	404ns	0.302	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	75ns	281ns	0	2.42 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	200μs	221ns	826ns	0	2.37 KB
master	`RunWafRealisticBenchmark`	net472	209μs	46.7ns	181ns	0.314	2.43 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	122μs	33.3ns	125ns	0	1.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	131μs	130ns	505ns	0	1.45 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	139μs	46.2ns	173ns	0.209	1.48 KB
#5804	`RunWafRealisticBenchmark`	net6.0	184μs	32.7ns	122ns	0	2.42 KB
#5804	`RunWafRealisticBenchmark`	netcoreapp3.1	197μs	119ns	446ns	0	2.37 KB
#5804	`RunWafRealisticBenchmark`	net472	208μs	41.3ns	160ns	0.312	2.43 KB
#5804	`RunWafRealisticBenchmarkWithAttack`	net6.0	122μs	123ns	478ns	0	1.46 KB
#5804	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	132μs	146ns	567ns	0	1.45 KB
#5804	`RunWafRealisticBenchmarkWithAttack`	net472	140μs	41.5ns	161ns	0.209	1.48 KB

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

More allocations ⚠️ in #5804

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0	255.33 KB	267.34 KB	12.02 KB	4.71%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1	255.17 KB	265.44 KB	10.27 KB	4.03%

Fewer allocations 🎉 in #5804

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	61.83 KB	59.04 KB	-2.79 KB	-4.52%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`StringConcatBenchmark`	net6.0	61.3μs	853ns	8.49μs	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	60.7μs	821ns	8.04μs	42.64 KB
master	`StringConcatBenchmark`	net472	36.5μs	73ns	263ns	61.83 KB
master	`StringConcatAspectBenchmark`	net6.0	318μs	1.76μs	11.2μs	255.33 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	345μs	1.64μs	7.51μs	255.17 KB
master	`StringConcatAspectBenchmark`	net472	292μs	7.14μs	68.9μs	278.53 KB
#5804	`StringConcatBenchmark`	net6.0	59.4μs	738ns	7.31μs	43.44 KB
#5804	`StringConcatBenchmark`	netcoreapp3.1	59.6μs	813ns	7.93μs	42.64 KB
#5804	`StringConcatBenchmark`	net472	37.1μs	53.8ns	186ns	59.04 KB
#5804	`StringConcatAspectBenchmark`	net6.0	334μs	1.87μs	11.5μs	267.34 KB
#5804	`StringConcatAspectBenchmark`	netcoreapp3.1	355μs	2.02μs	15.3μs	265.44 KB
#5804	`StringConcatAspectBenchmark`	net472	290μs	7.15μs	69.7μs	278.53 KB

andrewlock · 2024-07-24T19:43:39Z

Benchmarks Report for tracer 🐌

Benchmarks for #5804 compared to master:

1 benchmarks are faster, with geometric mean 1.151
1 benchmarks are slower, with geometric mean 1.127
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.87μs	42.6ns	319ns	0.0162	0.00808	0	5.42 KB
master	`StartStopWithChild`	netcoreapp3.1	9.85μs	54.6ns	349ns	0.0145	0.00966	0	5.62 KB
master	`StartStopWithChild`	net472	16μs	39.9ns	154ns	1.03	0.318	0.0955	6.07 KB
#5804	`StartStopWithChild`	net6.0	7.9μs	45ns	331ns	0.0163	0.00815	0	5.43 KB
#5804	`StartStopWithChild`	netcoreapp3.1	10μs	57ns	395ns	0.0201	0.00502	0	5.62 KB
#5804	`StartStopWithChild`	net472	16μs	51.3ns	192ns	1.02	0.308	0.0891	6.08 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	472μs	288ns	1.12μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	632μs	224ns	838ns	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	855μs	505ns	1.89μs	0.428	3.3 KB
#5804	`WriteAndFlushEnrichedTraces`	net6.0	479μs	222ns	860ns	0	2.7 KB
#5804	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	633μs	487ns	1.82μs	0	2.7 KB
#5804	`WriteAndFlushEnrichedTraces`	net472	842μs	504ns	1.95μs	0.419	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	192μs	1.02μs	6.79μs	0.184	18.45 KB
master	`SendRequest`	netcoreapp3.1	209μs	1.18μs	7.91μs	0.207	20.61 KB
master	`SendRequest`	net472	0.000596ns	0.000353ns	0.00132ns	0	0 b
#5804	`SendRequest`	net6.0	185μs	829ns	3.21μs	0.182	18.45 KB
#5804	`SendRequest`	netcoreapp3.1	213μs	1.23μs	9.8μs	0.202	20.61 KB
#5804	`SendRequest`	net472	0.00447ns	0.0014ns	0.00541ns	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	566μs	1.87μs	7.25μs	0.561	0	0	41.73 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	664μs	2.09μs	7.82μs	0.326	0	0	41.98 KB
master	`WriteAndFlushEnrichedTraces`	net472	855μs	4.18μs	17.7μs	8.63	2.47	0.411	53.28 KB
#5804	`WriteAndFlushEnrichedTraces`	net6.0	544μs	2.25μs	8.43μs	0.568	0	0	41.59 KB
#5804	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	684μs	3.59μs	17.9μs	0.334	0	0	41.8 KB
#5804	`WriteAndFlushEnrichedTraces`	net472	861μs	3μs	11.6μs	8.13	2.57	0.428	53.33 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteNonQuery`	net6.0	1.17μs	0.924ns	3.58ns	0.0143	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.66μs	0.756ns	2.73ns	0.0133	1.02 KB
master	`ExecuteNonQuery`	net472	2.05μs	1.8ns	6.96ns	0.157	987 B
#5804	`ExecuteNonQuery`	net6.0	1.23μs	1.24ns	4.81ns	0.0141	1.02 KB
#5804	`ExecuteNonQuery`	netcoreapp3.1	1.68μs	1.41ns	5.45ns	0.0141	1.02 KB
#5804	`ExecuteNonQuery`	net472	1.94μs	1.3ns	4.85ns	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.18μs	0.457ns	1.77ns	0.0136	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.47μs	0.46ns	1.72ns	0.0133	976 B
master	`CallElasticsearch`	net472	2.51μs	1.61ns	6.23ns	0.158	995 B
master	`CallElasticsearchAsync`	net6.0	1.21μs	0.956ns	3.7ns	0.0133	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.59μs	0.972ns	3.64ns	0.0136	1.02 KB
master	`CallElasticsearchAsync`	net472	2.65μs	1.82ns	7.06ns	0.166	1.05 KB
#5804	`CallElasticsearch`	net6.0	1.17μs	0.748ns	2.9ns	0.0135	976 B
#5804	`CallElasticsearch`	netcoreapp3.1	1.51μs	0.422ns	1.46ns	0.0129	976 B
#5804	`CallElasticsearch`	net472	2.43μs	2.34ns	9.05ns	0.158	995 B
#5804	`CallElasticsearchAsync`	net6.0	1.23μs	0.662ns	2.48ns	0.0135	952 B
#5804	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	0.668ns	2.5ns	0.0133	1.02 KB
#5804	`CallElasticsearchAsync`	net472	2.7μs	1.17ns	4.39ns	0.166	1.05 KB

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

Slower ⚠️ in #5804

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync‑net6.0	1.127	1,168.21	1,316.78

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net6.0	1.17μs	0.935ns	3.5ns	0.0134	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.68μs	0.805ns	3.12ns	0.0125	952 B
master	`ExecuteAsync`	net472	1.81μs	1.03ns	3.85ns	0.145	915 B
#5804	`ExecuteAsync`	net6.0	1.32μs	0.859ns	3.33ns	0.0134	952 B
#5804	`ExecuteAsync`	netcoreapp3.1	1.58μs	0.651ns	2.35ns	0.0127	952 B
#5804	`ExecuteAsync`	net472	1.76μs	1.3ns	5.03ns	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.08μs	1.74ns	6.5ns	0.0305	2.22 KB
master	`SendAsync`	netcoreapp3.1	5.04μs	2.85ns	11.1ns	0.038	2.76 KB
master	`SendAsync`	net472	7.78μs	1.67ns	6.24ns	0.497	3.15 KB
#5804	`SendAsync`	net6.0	4.09μs	1.32ns	4.77ns	0.0307	2.22 KB
#5804	`SendAsync`	netcoreapp3.1	5.15μs	2.88ns	10.8ns	0.036	2.76 KB
#5804	`SendAsync`	net472	7.78μs	2.82ns	10.6ns	0.498	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.5μs	0.663ns	2.48ns	0.0234	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.12μs	0.486ns	1.75ns	0.0221	1.64 KB
master	`EnrichedLog`	net472	2.71μs	4.48ns	17.4ns	0.249	1.57 KB
#5804	`EnrichedLog`	net6.0	1.49μs	0.509ns	1.91ns	0.0231	1.64 KB
#5804	`EnrichedLog`	netcoreapp3.1	2.18μs	1.03ns	3.99ns	0.022	1.64 KB
#5804	`EnrichedLog`	net472	2.76μs	1.2ns	4.66ns	0.25	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	163ns	630ns	0.0575	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	120μs	146ns	566ns	0	0	4.28 KB
master	`EnrichedLog`	net472	149μs	159ns	615ns	0.673	0.224	4.46 KB
#5804	`EnrichedLog`	net6.0	113μs	96.1ns	372ns	0.0567	0	4.28 KB
#5804	`EnrichedLog`	netcoreapp3.1	120μs	186ns	719ns	0	0	4.28 KB
#5804	`EnrichedLog`	net472	150μs	187ns	725ns	0.681	0.227	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.19μs	0.898ns	3.48ns	0.0303	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.34μs	1.63ns	6.3ns	0.0298	2.2 KB
master	`EnrichedLog`	net472	4.83μs	1.27ns	4.75ns	0.319	2.02 KB
#5804	`EnrichedLog`	net6.0	3.19μs	0.817ns	3.16ns	0.0302	2.2 KB
#5804	`EnrichedLog`	netcoreapp3.1	4.16μs	3.31ns	12.8ns	0.0291	2.2 KB
#5804	`EnrichedLog`	net472	4.89μs	1.41ns	5.46ns	0.319	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.33μs	0.654ns	2.53ns	0.016	0	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.78μs	1.33ns	5.13ns	0.0152	0	1.14 KB
master	`SendReceive`	net472	2.24μs	2.28ns	8.82ns	0.183	0.00112	1.16 KB
#5804	`SendReceive`	net6.0	1.36μs	0.507ns	1.9ns	0.0163	0	1.14 KB
#5804	`SendReceive`	netcoreapp3.1	1.76μs	1.03ns	4ns	0.016	0	1.14 KB
#5804	`SendReceive`	net472	2.14μs	2.06ns	7.96ns	0.183	0.00107	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	0.556ns	2.08ns	0.0222	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.88μs	1.29ns	5ns	0.0214	1.65 KB
master	`EnrichedLog`	net472	4.43μs	1.19ns	4.46ns	0.323	2.04 KB
#5804	`EnrichedLog`	net6.0	2.87μs	1.07ns	4.15ns	0.0226	1.6 KB
#5804	`EnrichedLog`	netcoreapp3.1	3.83μs	1.12ns	4.18ns	0.023	1.65 KB
#5804	`EnrichedLog`	net472	4.33μs	2.16ns	8.36ns	0.322	2.04 KB

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

Faster 🎉 in #5804

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0	1.151	547.60	475.81

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	401ns	0.224ns	0.868ns	0.00809	576 B
master	`StartFinishSpan`	netcoreapp3.1	567ns	0.387ns	1.5ns	0.0079	576 B
master	`StartFinishSpan`	net472	598ns	0.647ns	2.5ns	0.0916	578 B
master	`StartFinishScope`	net6.0	547ns	0.227ns	0.878ns	0.00964	696 B
master	`StartFinishScope`	netcoreapp3.1	747ns	0.654ns	2.53ns	0.0093	696 B
master	`StartFinishScope`	net472	889ns	0.598ns	2.32ns	0.104	658 B
#5804	`StartFinishSpan`	net6.0	401ns	0.173ns	0.622ns	0.00804	576 B
#5804	`StartFinishSpan`	netcoreapp3.1	547ns	0.453ns	1.76ns	0.00797	576 B
#5804	`StartFinishSpan`	net472	556ns	0.435ns	1.68ns	0.0915	578 B
#5804	`StartFinishScope`	net6.0	476ns	0.524ns	2.03ns	0.00974	696 B
#5804	`StartFinishScope`	netcoreapp3.1	742ns	1.49ns	5.78ns	0.00926	696 B
#5804	`StartFinishScope`	net472	850ns	0.888ns	3.44ns	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	652ns	0.672ns	2.6ns	0.00986	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	929ns	0.986ns	3.82ns	0.00924	696 B
master	`RunOnMethodBegin`	net472	1.06μs	0.959ns	3.71ns	0.104	658 B
#5804	`RunOnMethodBegin`	net6.0	612ns	3.35ns	20.4ns	0.00966	696 B
#5804	`RunOnMethodBegin`	netcoreapp3.1	920ns	1.17ns	4.55ns	0.00921	696 B
#5804	`RunOnMethodBegin`	net472	1.07μs	0.819ns	3.17ns	0.104	658 B

github-actions · 2024-07-30T16:59:57Z

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

4 occurrences of :

-      "hash": -1004380463,
+      "hash": XXX,
[...]
-        "path": "AspNetCore.Views_Iast_ReflectedXss+<<ExecuteAsync>b__8_1>d",
+        "path": "AspNetCore.Views_Iast_Xss+<<ExecuteAsync>b__8_1>d",

andrewlock · 2024-08-16T11:38:41Z

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 (5804) (11.571M)   : 0, 11570803
    master (11.688M)   : 0, 11687847
    benchmarks/2.9.0 (11.522M)   : 0, 11522274

    section Automatic
    This PR (5804) (7.854M)   : 0, 7853824
    master (7.784M)   : 0, 7784093
    benchmarks/2.9.0 (8.260M)   : 0, 8259819

    section Trace stats
    master (8.067M)   : 0, 8066980

    section Manual
    master (11.463M)   : 0, 11462744

    section Manual + Automatic
    This PR (5804) (7.235M)   : 0, 7235302
    master (7.262M)   : 0, 7262481

    section DD_TRACE_ENABLED=0
    master (10.761M)   : 0, 10760787

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5804) (9.574M)   : 0, 9573879

    section Automatic
    This PR (5804) (6.450M)   : 0, 6449959

    section Manual + Automatic
    This PR (5804) (6.224M)   : 0, 6223971

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (5804) (9.989M)   : 0, 9988582
    benchmarks/2.9.0 (10.078M)   : 0, 10078164

    section Automatic
    This PR (5804) (6.734M)   : 0, 6733949
    benchmarks/2.9.0 (7.346M)   : 0, 7346394

    section Manual + Automatic
    This PR (5804) (6.297M)   : 0, 6296581

Updated source name Codegen files

Fix linux tests Fix async test termination

Fix db tests

Fix netcore2 tests

Fix arm tests

Restored sealed status

robertpi · 2024-08-21T08:51:42Z

+            var currentSpan = scope?.Span;
+            var traceContext = currentSpan?.Context?.TraceContext;
+            traceContext?.IastRequestContext?.AddDbValue(column, value);
+            return true;


because of the null coercion we could have failed to add the data, but we'd still return true.

I'm not sure if the return value is used?

Yeah, return value is just for unit testing (to see if the record count has been reached)

robertpi · 2024-08-21T09:06:58Z

@@ -0,0 +1,50 @@
+[


No vuln detected because we don't want stored XXS to tricker for ssrf? Could be nice to update the PR text to explain that (or some other place?), it had me puzzled for a while.

No vuln detected because tainted source is purely database and it's not a XSS nor SQLI, so no injection should be raised.

robertpi

Couple of minor comments / questions, otherwise good.

bouwkast

LGTM (I didn't really review the IAST logic) but other bits looked fien

bouwkast · 2024-08-21T21:01:18Z

+        /// <returns>Calltarget state value</returns>
+        internal static CallTargetState OnMethodBegin<TTarget>(TTarget instance, CancellationToken cancellationToken)
+        {
+            return new CallTargetState(null);


I don't really understand why null here (and in the other OnMethodBegin)?

Yeah, if you're not doing anything in these methods, they should be omitted.

Because I don't want to generate a span, I just want to intercept the call

bouwkast · 2024-08-21T21:06:05Z

  "DD_IAST_REGEXP_TIMEOUT": "iast_regexp_timeout",
  "DD_IAST_TELEMETRY_VERBOSITY": "iast_telemetry_verbosity",
  "DD_IAST_TRUNCATION_MAX_VALUE_LENGTH": "iast_truncation_max_value_length",
+  "DD_IAST_DB_ROWS_TO_TAINT": "iast_db_rows_to_taint",


Just wondering if this was added to dd-go as well?

Yes it was. The PR is in the description 👍

andrewlock

Just checked the tracer stuff, no npgsql integration?

andrewlock · 2024-08-22T07:49:58Z

+        /// <summary>
+        /// OnMethodBegin callback
+        /// </summary>
+        /// <typeparam name="TTarget">Type of the target</typeparam>
+        /// <param name="instance">Instance value, aka `this` of the instrumented method.</param>
+        /// <returns>Calltarget state value</returns>
+        internal static CallTargetState OnMethodBegin<TTarget>(TTarget instance)
+        {
+            return new CallTargetState(null);
+        }


If you're not doing anything in OnMethodBegin, you can normally omit it... I don't know if the Adonet integrations are different? 🤔

andrewlock · 2024-08-22T07:51:38Z

+                }
+            }
+
+            state.Scope.DisposeWithException(exception);


Scope is always null here, right? Or are we creating a span somewhere else? :/ If so, in general that's kinda dangerous, as it has the potential for a span memory leak if we're relying on this being called to clear up the span...

Suggested change

state.Scope.DisposeWithException(exception);

andrewlock · 2024-08-22T07:51:59Z

+        /// <param name="exception">Exception instance in case the original code threw an exception.</param>
+        /// <param name="state">Calltarget state value</param>
+        /// <returns>A response value, in an async scenario will be T of Task of T</returns>
+        internal static CallTargetReturn<TReturn> OnMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception exception, in CallTargetState state)


Suggested change

internal static CallTargetReturn<TReturn> OnMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception exception, in CallTargetState state)

internal static CallTargetReturn<TReturn> OnMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception? exception, in CallTargetState state)

andrewlock · 2024-08-22T07:52:44Z

+        {
+            try
+            {
+                if (exception is null && returnValue is string value)


Surely value is always a string no? If so, we don't need to use generics for TReturn I don't think?

andrewlock · 2024-08-22T07:54:47Z

+                if (exception is null && returnValue is string value)
+                {
+                    string column;
+                    if (instance is IDataRecord record && state.State is int index)


Can we put a IDataRecord constraint on the TTarget instance? 🤔

andrewlock · 2024-08-22T07:55:23Z

+                    else
+                    {
+                        column = state.State?.ToString() ?? string.Empty;
+                    }


nit: state.State is guaranteed to be int and presumably instance is always IDataRecord? so I would probably just do

Suggested change

else

{

column = state.State?.ToString() ?? string.Empty;

}

else

{

column = string.Empty;

}

andrewlock · 2024-08-22T07:55:53Z

+        /// <returns>Calltarget state value</returns>
+        internal static CallTargetState OnMethodBegin<TTarget>(TTarget instance, CancellationToken cancellationToken)
+        {
+            return new CallTargetState(null);


Yeah, if you're not doing anything in these methods, they should be omitted.

andrewlock · 2024-08-22T07:56:03Z

+        /// <param name="exception">Exception instance in case the original code threw an exception.</param>
+        /// <param name="state">Calltarget state value</param>
+        /// <returns>A response value, in an async scenario will be T of Task of T</returns>
+        internal static TReturn OnAsyncMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception exception, in CallTargetState state)


Suggested change

internal static TReturn OnAsyncMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception exception, in CallTargetState state)

internal static TReturn OnAsyncMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception? exception, in CallTargetState state)

andrewlock · 2024-08-22T07:56:40Z

+                }
+            }
+
+            state.Scope.DisposeWithException(exception);


Always null, so can omit

Suggested change

state.Scope.DisposeWithException(exception);

andrewlock · 2024-08-22T07:56:55Z

+                }
+            }
+
+            state.Scope.DisposeWithException(exception);


You're not creating a span, right?

Suggested change

state.Scope.DisposeWithException(exception);

## Summary of changes - The instrumentation introduced for tainting values coming from database was incorrect for `Microsoft.Data.Sqlite`. A typo was introduced in the name `Microsoft.Data.Sqlite.SqliteDataReader`, thus making the instrumentation fail. - Instrumentations now use the latest dotnet version supported (here dotnet 9) ## Test coverage Tests on database tainting such as Stored XSS or Stored SQLI has been updated to also create an SQLite connection using `Microsoft.Data.Sqlite` while also keeping the `System.Data.SQLite` connection. ## Other details Original implementation PR: #5804

dromanol added area:asm area:asm-iast labels Jul 24, 2024

dromanol marked this pull request as ready for review July 31, 2024 22:09

dromanol requested review from a team as code owners July 31, 2024 22:09

dromanol force-pushed the dani/iast/taint_db branch from 4969960 to 698fe56 Compare August 1, 2024 11:05

lucaspimentel changed the title ~~[IAST] Taint values coming from data base~~ [IAST] Taint values coming from database Aug 2, 2024

lucaspimentel reviewed Aug 2, 2024

View reviewed changes

Comment thread ...tadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/AdoNetClientInstrumentMethodsAttribute.cs

dromanol force-pushed the dani/iast/taint_db branch 2 times, most recently from 5b199be to f6fca5f Compare August 7, 2024 09:55

dromanol force-pushed the dani/iast/taint_db branch from b51f8b5 to d1f6f3a Compare August 16, 2024 09:06

dromanol added 11 commits August 20, 2024 23:19

Initial method captures

29b879f

DbDataReader GetString tainting with row limit

2d7e0fa

Updated source name Codegen files

Updated config_norm_rules.json

d77b40b

Added tests

ec9c8f8

Better CallTarget coverage

8420fff

Fix linux tests Fix async test termination

Support for ReadAsync

f98649f

Fix db tests

StoredXss integration tests

e92224b

Fix netcore2 tests

Added interceptor for GetValue

bf4798a

Fix arm tests

Update AdoNetClientInstrumentMethodsAttribute.cs

3cc5e73

Restored sealed status

Rebase fixes

577a970

Added dbsource related tests

91ddea6

dromanol added 3 commits August 20, 2024 23:19

Updated test app

1db78bd

Rebase fix

f4c385f

Fix snapshot

30d3ba5

dromanol force-pushed the dani/iast/taint_db branch 2 times, most recently from 6ac3f42 to d603b70 Compare August 20, 2024 22:47

Fix arm tests

d603b70

robertpi reviewed Aug 21, 2024

View reviewed changes

robertpi approved these changes Aug 21, 2024

View reviewed changes

bouwkast approved these changes Aug 21, 2024

View reviewed changes

dromanol merged commit 0b37571 into master Aug 22, 2024

dromanol deleted the dani/iast/taint_db branch August 22, 2024 07:57

github-actions Bot added this to the vNext-v3 milestone Aug 22, 2024

andrewlock reviewed Aug 22, 2024

View reviewed changes

e-n-0 mentioned this pull request Nov 15, 2024

[ASM] iast: Fix Microsoft.Data.Sqlite database tainting #6295

Merged

	internal static CallTargetReturn<TReturn> OnMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception exception, in CallTargetState state)
	internal static CallTargetReturn<TReturn> OnMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception? exception, in CallTargetState state)

	internal static TReturn OnAsyncMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception exception, in CallTargetState state)
	internal static TReturn OnAsyncMethodEnd<TTarget, TReturn>(TTarget instance, TReturn returnValue, Exception? exception, in CallTargetState state)

Uh oh!

Conversation

dromanol commented Jul 24, 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!

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

Uh oh!

Execution-Time Benchmarks Report ⏱️

Uh oh!

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

Uh oh!

Datadog Report

Uh oh!

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

Uh oh!

Benchmarks Report for appsec 🐌

Benchmark details

Faster 🎉 in #5804

Raw results

Raw results

Raw results

More allocations ⚠️ in #5804

Fewer allocations 🎉 in #5804

Raw results

Uh oh!

andrewlock commented Jul 24, 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

Raw results

Raw results

Slower ⚠️ in #5804

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Faster 🎉 in #5804

Raw results

Raw results

Uh oh!

github-actions Bot commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Snapshots difference summary

Uh oh!

Uh oh!

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

Uh oh!

Throughput/Crank Report ⚡

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertpi left a comment

Choose a reason for hiding this comment

Uh oh!

bouwkast left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dromanol commented Jul 24, 2024 •

edited

Loading

andrewlock commented Jul 24, 2024 •

edited

Loading

datadog-ddstaging Bot commented Jul 24, 2024 •

edited

Loading

andrewlock commented Jul 24, 2024 •

edited

Loading

andrewlock commented Jul 24, 2024 •

edited

Loading

github-actions Bot commented Jul 30, 2024 •

edited

Loading

andrewlock commented Aug 16, 2024 •

edited

Loading

dromanol Aug 21, 2024 •

edited

Loading