[IAST] fix tainting values stored in the db #6389

robertpi · 2024-12-03T13:52:34Z

Summary of changes

Add tests and corrections to support for various database platforms.

Reason for change

Some bugs were reported relating to database tainting not working correctly for some dbs.

Test coverage

Adds new integration tests dependent on docker db instances.

github-actions · 2024-12-03T14:30:23Z

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.

1 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,
[...]
-      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.IastController.StoredSqli (Samples.Security.AspNetCore5),
+      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController.StoredSqli (Samples.Security.AspNetCore5.DatabaseIntegration),

1 occurrences of :

-      http.url: http://localhost:00000/Iast/StoredSqli?useMicrosoftDataDb=...,
+      http.url: http://localhost:00000/Iast/StoredSqli?database=...,

1 occurrences of :

-        "path": "Samples.Security.AspNetCore5.Controllers.IastController",
+        "path": "Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController",

3 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,

1 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,
[...]
-      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.IastController.StoredXss (Samples.Security.AspNetCore5),
+      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController.StoredXss (Samples.Security.AspNetCore5.DatabaseIntegration),

1 occurrences of :

-      http.url: http://localhost:00000/Iast/StoredXss?param=%3Cb%3ERawValue%3C/b%3E&useMicrosoftDataDb=...,
+      http.url: http://localhost:00000/Iast/StoredXss?param=%3Cb%3ERawValue%3C/b%3E&database=...,

1 occurrences of :

-    Service: Samples.Security.AspNetCore5,
+    Service: Samples.Security.AspNetCore5.DatabaseIntegration,
[...]
-      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.IastController.StoredXssEscaped (Samples.Security.AspNetCore5),
+      aspnet_core.endpoint: Samples.Security.AspNetCore5.Controllers.DatabaseIntegration.IastController.StoredXssEscaped (Samples.Security.AspNetCore5.DatabaseIntegration),

1 occurrences of :

-      http.url: http://localhost:00000/Iast/StoredXssEscaped?useMicrosoftDataDb=...,
+      http.url: http://localhost:00000/Iast/StoredXssEscaped?database=...,

andrewlock · 2024-12-03T15:08: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 (6389) - mean (68ms)  : 65, 71
     .   : milestone, 68,
    master - mean (68ms)  : 64, 73
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (979ms)  : 957, 1000
     .   : milestone, 979,
    master - mean (974ms)  : 948, 999
     .   : milestone, 974,

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

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (679ms)  : 666, 693
     .   : milestone, 679,
    master - mean (674ms)  : 660, 689
     .   : milestone, 674,

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

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (635ms)  : 620, 650
     .   : milestone, 635,
    master - mean (629ms)  : 614, 644
     .   : milestone, 629,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6389) - mean (194ms)  : 190, 199
     .   : milestone, 194,
    master - mean (194ms)  : 190, 198
     .   : milestone, 194,

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (1,103ms)  : 1070, 1137
     .   : milestone, 1103,
    master - mean (1,098ms)  : 1074, 1123
     .   : milestone, 1098,

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

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (878ms)  : 844, 913
     .   : milestone, 878,
    master - mean (869ms)  : 838, 901
     .   : milestone, 869,

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

    section CallTarget+Inlining+NGEN
    This PR (6389) - mean (853ms)  : 819, 887
     .   : milestone, 853,
    master - mean (850ms)  : 811, 890
     .   : milestone, 850,

andrewlock · 2024-12-03T15:34:02Z

Benchmarks Report for appsec 🐌

Benchmarks for #6389 compared to master:

1 benchmarks are faster, with geometric mean 1.172
1 benchmarks are slower, with geometric mean 1.130
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 - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #6389

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net6.0	1.130	188,463.30	212,878.99

Faster 🎉 in #6389

Benchmark	base/diff	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1	1.172	229.02	195.35

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net6.0	187μs	117ns	437ns	2.62	0	188.52 KB
master	`AllCycleSimpleBody`	netcoreapp3.1	281μs	159ns	597ns	2.66	0	195.8 KB
master	`AllCycleSimpleBody`	net472	250μs	153ns	574ns	35.8	2	225.32 KB
master	`AllCycleMoreComplexBody`	net6.0	189μs	80.4ns	290ns	2.74	0	192.02 KB
master	`AllCycleMoreComplexBody`	netcoreapp3.1	298μs	639ns	2.48μs	2.63	0	199.22 KB
master	`AllCycleMoreComplexBody`	net472	254μs	425ns	1.64μs	36.4	2.02	228.83 KB
master	`ObjectExtractorSimpleBody`	net6.0	138ns	0.184ns	0.69ns	0.00394	0	280 B
master	`ObjectExtractorSimpleBody`	netcoreapp3.1	229ns	0.339ns	1.31ns	0.00366	0	272 B
master	`ObjectExtractorSimpleBody`	net472	163ns	0.264ns	1.02ns	0.0446	0	281 B
master	`ObjectExtractorMoreComplexBody`	net6.0	2.89μs	1.4ns	5.22ns	0.0522	0	3.78 KB
master	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.73μs	3.7ns	13.8ns	0.0508	0	3.69 KB
master	`ObjectExtractorMoreComplexBody`	net472	3.6μs	2.19ns	8.18ns	0.603	0.00543	3.8 KB
#6389	`AllCycleSimpleBody`	net6.0	202μs	106ns	395ns	2.65	0	188.38 KB
#6389	`AllCycleSimpleBody`	netcoreapp3.1	305μs	116ns	417ns	2.58	0	195.66 KB
#6389	`AllCycleSimpleBody`	net472	267μs	320ns	1.15μs	35.7	2	225.16 KB
#6389	`AllCycleMoreComplexBody`	net6.0	213μs	88.5ns	343ns	2.64	0	191.88 KB
#6389	`AllCycleMoreComplexBody`	netcoreapp3.1	308μs	225ns	870ns	2.76	0	199.07 KB
#6389	`AllCycleMoreComplexBody`	net472	273μs	452ns	1.63μs	36.3	2.05	228.67 KB
#6389	`ObjectExtractorSimpleBody`	net6.0	137ns	0.109ns	0.392ns	0.00391	0	280 B
#6389	`ObjectExtractorSimpleBody`	netcoreapp3.1	195ns	0.0694ns	0.269ns	0.00374	0	272 B
#6389	`ObjectExtractorSimpleBody`	net472	163ns	0.113ns	0.44ns	0.0446	0	281 B
#6389	`ObjectExtractorMoreComplexBody`	net6.0	2.91μs	2.71ns	10.5ns	0.0538	0	3.78 KB
#6389	`ObjectExtractorMoreComplexBody`	netcoreapp3.1	3.97μs	2.34ns	9.05ns	0.0517	0	3.69 KB
#6389	`ObjectExtractorMoreComplexBody`	net472	3.59μs	3.04ns	11.4ns	0.602	0.00538	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.2μs	26.4ns	98.6ns	0.445	0	32.4 KB
master	`EncodeArgs`	netcoreapp3.1	54.7μs	56.4ns	219ns	0.432	0	32.4 KB
master	`EncodeArgs`	net472	67.7μs	43.1ns	161ns	5.16	0.067	32.5 KB
master	`EncodeLegacyArgs`	net6.0	74.2μs	107ns	416ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	netcoreapp3.1	104μs	163ns	630ns	0	0	2.14 KB
master	`EncodeLegacyArgs`	net472	154μs	128ns	497ns	0.306	0	2.15 KB
#6389	`EncodeArgs`	net6.0	36.9μs	23.5ns	81.3ns	0.443	0	32.4 KB
#6389	`EncodeArgs`	netcoreapp3.1	54.4μs	54.1ns	203ns	0.433	0	32.4 KB
#6389	`EncodeArgs`	net472	67.9μs	125ns	484ns	5.15	0.0673	32.5 KB
#6389	`EncodeLegacyArgs`	net6.0	73.1μs	37.4ns	140ns	0.0364	0	2.14 KB
#6389	`EncodeLegacyArgs`	netcoreapp3.1	106μs	171ns	638ns	0	0	2.14 KB
#6389	`EncodeLegacyArgs`	net472	154μs	96.7ns	362ns	0.307	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	181μs	84ns	291ns	0	2.44 KB
master	`RunWafRealisticBenchmark`	netcoreapp3.1	192μs	249ns	964ns	0	2.39 KB
master	`RunWafRealisticBenchmark`	net472	209μs	180ns	696ns	0.309	2.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net6.0	122μs	119ns	461ns	0	1.47 KB
master	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	129μs	162ns	583ns	0	1.46 KB
master	`RunWafRealisticBenchmarkWithAttack`	net472	139μs	56.6ns	219ns	0.209	1.49 KB
#6389	`RunWafRealisticBenchmark`	net6.0	190μs	162ns	626ns	0	2.44 KB
#6389	`RunWafRealisticBenchmark`	netcoreapp3.1	194μs	192ns	718ns	0	2.39 KB
#6389	`RunWafRealisticBenchmark`	net472	207μs	126ns	471ns	0.31	2.46 KB
#6389	`RunWafRealisticBenchmarkWithAttack`	net6.0	123μs	108ns	419ns	0	1.47 KB
#6389	`RunWafRealisticBenchmarkWithAttack`	netcoreapp3.1	130μs	116ns	451ns	0	1.46 KB
#6389	`RunWafRealisticBenchmarkWithAttack`	net472	141μs	96.5ns	361ns	0.211	1.49 KB

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

More allocations ⚠️ in #6389

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net6.0	257.94 KB	264.13 KB	6.18 KB	2.40%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1	253.05 KB	255.14 KB	2.09 KB	0.83%

Fewer allocations 🎉 in #6389

Benchmark	Base Allocated	Diff Allocated	Change	Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472	61.66 KB	60.06 KB	-1.6 KB	-2.59%

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Allocated
master	`StringConcatBenchmark`	net6.0	52.3μs	249ns	932ns	43.44 KB
master	`StringConcatBenchmark`	netcoreapp3.1	53.4μs	176ns	660ns	42.64 KB
master	`StringConcatBenchmark`	net472	36.4μs	80.4ns	290ns	61.66 KB
master	`StringConcatAspectBenchmark`	net6.0	310μs	1.09μs	3.92μs	257.94 KB
master	`StringConcatAspectBenchmark`	netcoreapp3.1	333μs	1.74μs	9.37μs	253.05 KB
master	`StringConcatAspectBenchmark`	net472	284μs	6.2μs	59.5μs	278.53 KB
#6389	`StringConcatBenchmark`	net6.0	51.5μs	244ns	944ns	43.44 KB
#6389	`StringConcatBenchmark`	netcoreapp3.1	58μs	794ns	7.86μs	42.64 KB
#6389	`StringConcatBenchmark`	net472	37.4μs	87.5ns	328ns	60.06 KB
#6389	`StringConcatAspectBenchmark`	net6.0	291μs	6.09μs	60.3μs	264.13 KB
#6389	`StringConcatAspectBenchmark`	netcoreapp3.1	345μs	1.77μs	10.8μs	255.14 KB
#6389	`StringConcatAspectBenchmark`	net472	272μs	4.5μs	42.9μs	278.53 KB

datadog-ddstaging · 2024-12-03T16:07:58Z

Datadog Report

Branch report: robert/asm/test-stored-xxs
Commit report: e9072d2
Test service: dd-trace-dotnet

✅ 0 Failed, 466351 Passed, 3694 Skipped, 33h 5m 16.77s Total Time

andrewlock · 2024-12-03T16:16:47Z

Benchmarks Report for tracer 🐌

Benchmarks for #6389 compared to master:

1 benchmarks are slower, with geometric mean 1.121
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μs	44.8ns	297ns	0.0159	0.00794	0	5.61 KB
master	`StartStopWithChild`	netcoreapp3.1	9.82μs	47.8ns	203ns	0.0242	0.00969	0	5.8 KB
master	`StartStopWithChild`	net472	16.2μs	49.4ns	191ns	1.04	0.3	0.0974	6.21 KB
#6389	`StartStopWithChild`	net6.0	8.26μs	46.7ns	313ns	0.0163	0.00815	0	5.61 KB
#6389	`StartStopWithChild`	netcoreapp3.1	10.3μs	54.7ns	310ns	0.0199	0.00497	0	5.81 KB
#6389	`StartStopWithChild`	net472	16.4μs	43.1ns	167ns	1.06	0.341	0.106	6.2 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`WriteAndFlushEnrichedTraces`	net6.0	480μs	735ns	2.85μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	647μs	309ns	1.2μs	0	2.7 KB
master	`WriteAndFlushEnrichedTraces`	net472	853μs	419ns	1.62μs	0.425	3.3 KB
#6389	`WriteAndFlushEnrichedTraces`	net6.0	489μs	347ns	1.34μs	0	2.7 KB
#6389	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	701μs	629ns	2.44μs	0	2.7 KB
#6389	`WriteAndFlushEnrichedTraces`	net472	877μs	359ns	1.29μs	0.434	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	150μs	900ns	8.91μs	0.136	14.47 KB
master	`SendRequest`	netcoreapp3.1	169μs	1.02μs	9.81μs	0.181	17.27 KB
master	`SendRequest`	net472	0.00166ns	0.000575ns	0.00215ns	0	0 b
#6389	`SendRequest`	net6.0	152μs	887ns	7.73μs	0.142	14.47 KB
#6389	`SendRequest`	netcoreapp3.1	166μs	945ns	7.14μs	0.225	17.27 KB
#6389	`SendRequest`	net472	0.00082ns	0.000466ns	0.00168ns	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	570μs	3.09μs	18μs	0.579	0	0	41.64 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	677μs	3.69μs	21.2μs	0.347	0	0	41.78 KB
master	`WriteAndFlushEnrichedTraces`	net472	833μs	3.19μs	11.9μs	8.33	2.5	0.417	53.26 KB
#6389	`WriteAndFlushEnrichedTraces`	net6.0	556μs	2.16μs	8.06μs	0.534	0	0	41.57 KB
#6389	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	658μs	2.24μs	8.38μs	0.321	0	0	41.78 KB
#6389	`WriteAndFlushEnrichedTraces`	net472	849μs	3.14μs	11.3μs	8.08	2.55	0.425	53.27 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.3μs	0.717ns	2.78ns	0.0142	0	1.02 KB
master	`ExecuteNonQuery`	netcoreapp3.1	1.76μs	1.82ns	7.04ns	0.0139	0	1.02 KB
master	`ExecuteNonQuery`	net472	2.01μs	1.64ns	6.34ns	0.156	0.00101	987 B
#6389	`ExecuteNonQuery`	net6.0	1.35μs	1.52ns	5.87ns	0.0143	0	1.02 KB
#6389	`ExecuteNonQuery`	netcoreapp3.1	1.79μs	3.58ns	13.4ns	0.0142	0	1.02 KB
#6389	`ExecuteNonQuery`	net472	2.14μs	2.28ns	8.54ns	0.157	0.00107	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.27μs	0.541ns	2.1ns	0.0133	976 B
master	`CallElasticsearch`	netcoreapp3.1	1.59μs	0.682ns	2.64ns	0.0128	976 B
master	`CallElasticsearch`	net472	2.59μs	1.04ns	3.88ns	0.157	995 B
master	`CallElasticsearchAsync`	net6.0	1.39μs	0.653ns	2.53ns	0.0132	952 B
master	`CallElasticsearchAsync`	netcoreapp3.1	1.74μs	0.608ns	2.27ns	0.0139	1.02 KB
master	`CallElasticsearchAsync`	net472	2.58μs	0.841ns	3.15ns	0.167	1.05 KB
#6389	`CallElasticsearch`	net6.0	1.23μs	1.25ns	4.86ns	0.0135	976 B
#6389	`CallElasticsearch`	netcoreapp3.1	1.54μs	0.428ns	1.6ns	0.0132	976 B
#6389	`CallElasticsearch`	net472	2.64μs	1.23ns	4.76ns	0.158	995 B
#6389	`CallElasticsearchAsync`	net6.0	1.29μs	0.624ns	2.33ns	0.0136	952 B
#6389	`CallElasticsearchAsync`	netcoreapp3.1	1.65μs	0.453ns	1.69ns	0.014	1.02 KB
#6389	`CallElasticsearchAsync`	net472	2.63μs	1.65ns	6.17ns	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.31μs	3.72ns	14.4ns	0.0129	952 B
master	`ExecuteAsync`	netcoreapp3.1	1.72μs	0.578ns	2.16ns	0.0129	952 B
master	`ExecuteAsync`	net472	1.79μs	0.544ns	1.96ns	0.145	915 B
#6389	`ExecuteAsync`	net6.0	1.42μs	0.413ns	1.6ns	0.0135	952 B
#6389	`ExecuteAsync`	netcoreapp3.1	1.68μs	0.876ns	3.28ns	0.0125	952 B
#6389	`ExecuteAsync`	net472	1.8μs	0.411ns	1.59ns	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.37μs	1.03ns	3.99ns	0.0326	2.31 KB
master	`SendAsync`	netcoreapp3.1	5.27μs	2.36ns	9.13ns	0.0396	2.85 KB
master	`SendAsync`	net472	7.34μs	1.57ns	5.87ns	0.495	3.12 KB
#6389	`SendAsync`	net6.0	4.49μs	1.7ns	6.57ns	0.0315	2.31 KB
#6389	`SendAsync`	netcoreapp3.1	5.38μs	3.19ns	12.4ns	0.0376	2.85 KB
#6389	`SendAsync`	net472	7.4μs	2.61ns	10.1ns	0.492	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.48μs	2.98ns	11.6ns	0.0234	1.64 KB
master	`EnrichedLog`	netcoreapp3.1	2.2μs	1.92ns	7.43ns	0.022	1.64 KB
master	`EnrichedLog`	net472	2.71μs	1.8ns	6.48ns	0.249	1.57 KB
#6389	`EnrichedLog`	net6.0	1.46μs	1.16ns	4.5ns	0.0232	1.64 KB
#6389	`EnrichedLog`	netcoreapp3.1	2.14μs	0.755ns	2.93ns	0.0217	1.64 KB
#6389	`EnrichedLog`	net472	2.68μs	1.27ns	4.74ns	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	119μs	269ns	1.04μs	0.0594	0	4.28 KB
master	`EnrichedLog`	netcoreapp3.1	124μs	258ns	1μs	0	0	4.28 KB
master	`EnrichedLog`	net472	151μs	250ns	970ns	0.669	0.223	4.46 KB
#6389	`EnrichedLog`	net6.0	119μs	211ns	818ns	0	0	4.28 KB
#6389	`EnrichedLog`	netcoreapp3.1	123μs	206ns	797ns	0.0623	0	4.28 KB
#6389	`EnrichedLog`	net472	154μs	136ns	527ns	0.69	0.23	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.13μs	1.35ns	5.22ns	0.0312	2.2 KB
master	`EnrichedLog`	netcoreapp3.1	4.18μs	2.66ns	10.3ns	0.0291	2.2 KB
master	`EnrichedLog`	net472	4.9μs	0.728ns	2.72ns	0.319	2.02 KB
#6389	`EnrichedLog`	net6.0	2.99μs	0.888ns	3.32ns	0.0314	2.2 KB
#6389	`EnrichedLog`	netcoreapp3.1	4.23μs	1.75ns	6.77ns	0.0296	2.2 KB
#6389	`EnrichedLog`	net472	4.89μs	1.19ns	4.63ns	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.32μs	1.55ns	5.98ns	0.0158	1.14 KB
master	`SendReceive`	netcoreapp3.1	1.83μs	1.73ns	6.7ns	0.0153	1.14 KB
master	`SendReceive`	net472	2.11μs	1.3ns	5.03ns	0.184	1.16 KB
#6389	`SendReceive`	net6.0	1.42μs	1.07ns	4.16ns	0.016	1.14 KB
#6389	`SendReceive`	netcoreapp3.1	1.74μs	1.3ns	5.03ns	0.0155	1.14 KB
#6389	`SendReceive`	net472	2.07μs	1.32ns	5.11ns	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.74μs	2.57ns	9.95ns	0.022	1.6 KB
master	`EnrichedLog`	netcoreapp3.1	3.81μs	1.28ns	4.78ns	0.021	1.65 KB
master	`EnrichedLog`	net472	4.44μs	0.633ns	2.19ns	0.324	2.04 KB
#6389	`EnrichedLog`	net6.0	2.65μs	0.561ns	2.02ns	0.0228	1.6 KB
#6389	`EnrichedLog`	netcoreapp3.1	3.91μs	1.31ns	4.91ns	0.0215	1.65 KB
#6389	`EnrichedLog`	net472	4.33μs	3.73ns	14.4ns	0.323	2.04 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net6.0	398ns	0.261ns	0.977ns	0.00814	576 B
master	`StartFinishSpan`	netcoreapp3.1	589ns	0.702ns	2.72ns	0.00767	576 B
master	`StartFinishSpan`	net472	649ns	0.436ns	1.69ns	0.0918	578 B
master	`StartFinishScope`	net6.0	560ns	0.241ns	0.901ns	0.00982	696 B
master	`StartFinishScope`	netcoreapp3.1	723ns	0.791ns	3.06ns	0.0094	696 B
master	`StartFinishScope`	net472	875ns	0.528ns	2.04ns	0.104	658 B
#6389	`StartFinishSpan`	net6.0	393ns	0.249ns	0.965ns	0.00797	576 B
#6389	`StartFinishSpan`	netcoreapp3.1	580ns	0.567ns	2.2ns	0.00775	576 B
#6389	`StartFinishSpan`	net472	648ns	0.349ns	1.21ns	0.0916	578 B
#6389	`StartFinishScope`	net6.0	568ns	0.551ns	2.13ns	0.00965	696 B
#6389	`StartFinishScope`	netcoreapp3.1	749ns	0.545ns	2.11ns	0.00931	696 B
#6389	`StartFinishScope`	net472	870ns	0.475ns	1.84ns	0.104	658 B

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

Slower ⚠️ in #6389

Benchmark	diff/base	Base Median (ns)	Diff Median (ns)	Modality
Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0	1.121	609.08	682.66

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net6.0	609ns	0.524ns	2.03ns	0.00977	696 B
master	`RunOnMethodBegin`	netcoreapp3.1	927ns	0.491ns	1.9ns	0.00934	696 B
master	`RunOnMethodBegin`	net472	1.12μs	1.19ns	4.47ns	0.104	658 B
#6389	`RunOnMethodBegin`	net6.0	683ns	0.43ns	1.67ns	0.00973	696 B
#6389	`RunOnMethodBegin`	netcoreapp3.1	968ns	0.376ns	1.36ns	0.00925	696 B
#6389	`RunOnMethodBegin`	net472	1.1μs	0.504ns	1.95ns	0.104	658 B

andrewlock · 2024-12-03T20:56:44Z

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 (6389) (11.192M)   : 0, 11192159
    master (11.195M)   : 0, 11194535
    benchmarks/2.9.0 (11.033M)   : 0, 11032866

    section Automatic
    This PR (6389) (7.278M)   : 0, 7278323
    master (7.216M)   : 0, 7216361
    benchmarks/2.9.0 (7.786M)   : 0, 7785853

    section Trace stats
    master (7.611M)   : 0, 7610504

    section Manual
    master (11.163M)   : 0, 11162548

    section Manual + Automatic
    This PR (6389) (6.775M)   : 0, 6774762
    master (6.753M)   : 0, 6752572

    section DD_TRACE_ENABLED=0
    master (10.216M)   : 0, 10215642

gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6389) (9.270M)   : 0, 9270052
    benchmarks/2.9.0 (9.495M)   : 0, 9494821

    section Automatic
    This PR (6389) (6.433M)   : 0, 6432814

    section Manual + Automatic
    This PR (6389) (5.802M)   : 0, 5801580

gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (6389) (9.513M)   : 0, 9512762
    master (9.988M)   : 0, 9987589
    benchmarks/2.9.0 (10.020M)   : 0, 10019592

    section Automatic
    This PR (6389) (6.176M)   : 0, 6175629
    master (6.338M)   : 0, 6338249
    benchmarks/2.9.0 (7.255M)   : 0, 7255257

    section Trace stats
    master (7.137M)   : 0, 7136842

    section Manual
    master (9.965M)   : 0, 9965094

    section Manual + Automatic
    This PR (6389) (5.699M)   : 0, 5699227
    master (5.791M)   : 0, 5790893

    section DD_TRACE_ENABLED=0
    master (9.274M)   : 0, 9274318

NachoEchevarria · 2024-12-10T10:48:06Z

.../test/test-applications/security/Samples.Security.AspNetCore5/Properties/launchSettings.json

        "DD_APPSEC_BLOCKING_ENABLED": "false",
-        "DD_DOTNET_TRACER_HOME": "$(SolutionDir)shared\\bin\\monitoring-home",
+        "DD_DOTNET_TRACER_HOME": "c:\\code\\dd-trace-dotnet\\shared\\bin\\monitoring-home",


Probably, changes in this file were not intended to be included in the PR

Good catch, thanks! Fixed in d266b58

NachoEchevarria · 2024-12-10T10:50:01Z

...-applications/security/aspnet/Samples.Security.AspNetMvc5/Samples.Security.AspNetMvc5.csproj

@@ -85,14 +85,38 @@
    <ErrorReport>prompt</ErrorReport>
  </PropertyGroup>
  <ItemGroup>
+    <Reference Include="BouncyCastle.Crypto, Version=1.9.0.0, Culture=neutral, PublicKeyToken=0e99375e54769942, processorArchitecture=MSIL">


The c# code from this mvc web does not seemed to have changed. Do we need these new references?

Seems to have been added automatically at some point, thanks! Fixed in d266b58

NachoEchevarria · 2024-12-10T10:58:43Z

...ions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Controllers/HomeController.cs

@@ -0,0 +1,33 @@
+using Microsoft.AspNetCore.Mvc;


I don't fully get why we need a new sample page instead of reusing/expanding the old one. I am pretty sure that there is a strong reason, though.

Yeah, I don't get why either. It feels like this could be added to the Samples.Security.AspNetCore5 one

It needs to be a whole new sample, because it has dependencies databases run in docker. The way the integration tests currently work is samples are either run with docker dependencies or without them - you can build in both tests runs.

So if we want to have these endpoints to Samples.Security.AspNetCore5 it would mean moving all the tests in that run with docker dependency. We could so that, but I don't think it's what we want as the with docker run is more flaky.

We could so that, but I don't think it's what we want as the with docker run is more flaky.

Personally, I would actually have gone the other way - I'm very in favour of fewer samples. If anything, the docker run is less flakey actually 😄

Also, somewhat separate, but it would be preferable to link the files into the sample (if we really need one) rather than copying. Every extra file slows down CI a small amount 😄 + it's more duplication to deal with etc

NachoEchevarria · 2024-12-10T11:00:13Z

...tions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Data/IastControllerHelper.cs

+            return res;
+        }
+    }
+}


Should binary app.db file be included?

Yes, it's needed for sqlite tests.

daniel-romano-DD

Thanks a lot for this.

NachoEchevarria

Nice!!!

andrewlock

Mostly LGTM from the integration side. Just a few nits and suggestions. Personally I would favour merging the samples tbh (but could be done in a separate PR instead if you agree and that's easier)

andrewlock · 2024-12-11T10:07:14Z

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/MySql/MySqlDefinitions.cs

+    MaximumVersion = "9.*.*",
+    IntegrationName = nameof(IntegrationId.MySql),
+    DataReaderType = "MySql.Data.MySqlClient.MySqlDataReader",
+    DataReaderTaskType = "System.Threading.Tasks.Task`1[MySql.Data.MySqlClient.MySqlDataReader]",


As mentioned elsewhere we should mark these integration as being IAST only - may require an update to the source generator though

andrewlock · 2024-12-11T10:10:46Z

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/ReaderCloseNpgsqlIntegration.cs

nit: I think we should move this into the Npgsql sub-folder

fixed in c59e762

andrewlock · 2024-12-11T10:11:51Z

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/ReaderCloseNpgsqlIntegration.cs

+        ParameterTypeNames = new[] { ClrNames.Bool, ClrNames.Bool, ClrNames.Bool, },
+        MinimumVersion = "4.0.0",
+        MaximumVersion = "8.*.*",
+        IntegrationName = nameof(IntegrationId.Npgsql))]


Ok, we can definitely set the InstrumentationCategory for this one!

Suggested change

IntegrationName = nameof(IntegrationId.Npgsql))]

IntegrationName = nameof(IntegrationId.Npgsql),

InstrumentationCategory = InstrumentationCategory.Iast)]

fixed in c59e762

andrewlock · 2024-12-11T10:12:34Z

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AdoNet/ReaderCloseNpgsqlIntegration.cs

+        /// <summary>
+        /// OnMethodEnd callback
+        /// </summary>
+        /// <typeparam name="TTarget">Type of the target</typeparam>
+        /// <typeparam name="TReturn">Type of the return</typeparam>
+        /// <param name="instance">Instance value, aka `this` of the instrumented method.</param>
+        /// <param name="returnValue">Instance of the return value</param>
+        /// <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 OnMethodEnd<TTarget>(TTarget instance, Exception? exception, in CallTargetState state)


nit: we tend to remove these comments as being just noise these days

Suggested change

/// <summary>

/// OnMethodEnd callback

/// </summary>

/// <typeparam name="TTarget">Type of the target</typeparam>

/// <typeparam name="TReturn">Type of the return</typeparam>

/// <param name="instance">Instance value, aka `this` of the instrumented method.</param>

/// <param name="returnValue">Instance of the return value</param>

/// <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 OnMethodEnd<TTarget>(TTarget instance, Exception? exception, in CallTargetState state)

fixed in c59e762

andrewlock · 2024-12-11T10:13:59Z

tracer/src/Datadog.Trace/Iast/IastModule.cs

+        // note: the return value this method indicates if the value could have been tainted
+        // if a iast context is available. It will return true if iast is enabled and the row
+        // count conditions are met. The result is used in the unit tests, don't use it
+        // anywhere else


nit: should we add this comment to the doc comments? 🙂

Suggested change

// note: the return value this method indicates if the value could have been tainted

// if a iast context is available. It will return true if iast is enabled and the row

// count conditions are met. The result is used in the unit tests, don't use it

// anywhere else

/// <summary>

/// Add a DB value to be tainted

/// </summary>

/// <returns>The return value this method indicates if the value could have been tainted

/// if a iast context is available. It will return true if iast is enabled and the row

/// count conditions are met. The result is used in the unit tests, don't use it

/// anywhere else

/// </returns>

fixed in c59e762

tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp

andrewlock · 2024-12-11T10:16:22Z

...ions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Controllers/HomeController.cs

@@ -0,0 +1,33 @@
+using Microsoft.AspNetCore.Mvc;


We could so that, but I don't think it's what we want as the with docker run is more flaky.

Personally, I would actually have gone the other way - I'm very in favour of fewer samples. If anything, the docker run is less flakey actually 😄

andrewlock · 2024-12-11T10:17:34Z

...ions/security/Samples.Security.AspNetCore5.DatabaseIntegration/Controllers/HomeController.cs

@@ -0,0 +1,33 @@
+using Microsoft.AspNetCore.Mvc;


Also, somewhat separate, but it would be preferable to link the files into the sample (if we really need one) rather than copying. Every extra file slows down CI a small amount 😄 + it's more duplication to deal with etc

robertpi · 2024-12-11T12:57:13Z

Mostly LGTM from the integration side. Just a few nits and suggestions. Personally I would favour merging the samples tbh (but could be done in a separate PR instead if you agree and that's easier)

Okay, I agree, we can do a follow up PR to remerge the sample and move it completely over the docker dependency.

We could so that, but I don't think it's what we want as the with docker run is more flaky.

Personally, I would actually have gone the other way - I'm very in favour of fewer samples. If anything, the docker run is less flakey actually 😄

Doesn't start the depended database images failing to start create more flake? If the docker depend tests really are more stable, why not run everything there.

## Summary of changes Add tests and corrections to support for various database platforms. ## Reason for change Some bugs were reported relating to database tainting not working correctly for some dbs. ## Test coverage Adds new integration tests dependent on docker db instances.

robertpi force-pushed the robert/asm/test-stored-xxs branch from e381678 to 17c314e Compare December 3, 2024 14:29

robertpi force-pushed the robert/asm/test-stored-xxs branch from a5ed297 to fc2be2d Compare December 3, 2024 16:25

robertpi marked this pull request as ready for review December 10, 2024 10:12

robertpi requested review from a team as code owners December 10, 2024 10:12

NachoEchevarria reviewed Dec 10, 2024

View reviewed changes

daniel-romano-DD approved these changes Dec 10, 2024

View reviewed changes

robertpi force-pushed the robert/asm/test-stored-xxs branch from 094a66c to 194b611 Compare December 10, 2024 15:16

NachoEchevarria approved these changes Dec 10, 2024

View reviewed changes

andrewlock approved these changes Dec 11, 2024

View reviewed changes

robertpi added 8 commits December 12, 2024 17:42

sql server tests working

1a72d8d

sql server really working

962b1e6

postgres tests implemented and failing (as expected)

2d27585

my sql tests added and failing (more or less as expected)

f1aa42e

oracle tests added and failing

1752e30

WIP fixing tests

1cbb2f8

fix postgres md5 false positive

97d4e60

TestIastStoredXssRequest not working everywhere

e8d3d3e

robertpi added 12 commits December 13, 2024 13:39

all db tests working - probably

74cf3d8

fix flake in npgsql tests

2ff49ec

minor corrections I spotted

401e631

correct error indroduced by error corrections

a7b8823

correct some errors in tests

cc0d9df

note explaining a hack that the unit tests rely on

e02963a

hopefully fix docker releated issues

4fb8526

do connection string properlly

c167065

correct issues with framework builds

e334b1d

can't run docker in CI

8eb28e3

feedback from review

ee5f058

response to comments

e9072d2

robertpi force-pushed the robert/asm/test-stored-xxs branch from c59e762 to e9072d2 Compare December 13, 2024 12:48

robertpi merged commit 0e50622 into master Dec 13, 2024
105 of 110 checks passed

robertpi deleted the robert/asm/test-stored-xxs branch December 13, 2024 14:52

github-actions bot added this to the vNext-v3 milestone Dec 13, 2024

andrewlock added type:bug area:asm area:asm-iast labels Dec 18, 2024

	IntegrationName = nameof(IntegrationId.Npgsql))]
	IntegrationName = nameof(IntegrationId.Npgsql),
	InstrumentationCategory = InstrumentationCategory.Iast)]

[IAST] fix tainting values stored in the db #6389

[IAST] fix tainting values stored in the db #6389

Conversation

robertpi commented Dec 3, 2024

Summary of changes

Reason for change

Test coverage

github-actions bot commented Dec 3, 2024 • edited Loading

Snapshots difference summary

andrewlock commented Dec 3, 2024 • edited Loading

Execution-Time Benchmarks Report ⏱️

andrewlock commented Dec 3, 2024 • edited Loading

Benchmarks Report for appsec 🐌

Benchmark details

Slower ⚠️ in #6389

Faster 🎉 in #6389

Raw results

Raw results

Raw results

More allocations ⚠️ in #6389

Fewer allocations 🎉 in #6389

Raw results

datadog-ddstaging bot commented Dec 3, 2024 • edited Loading

Datadog Report

andrewlock commented Dec 3, 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

Raw results

Slower ⚠️ in #6389

Raw results

andrewlock commented Dec 3, 2024 • edited Loading

Throughput/Crank Report ⚡

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NachoEchevarria Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-romano-DD left a comment

Choose a reason for hiding this comment

NachoEchevarria left a comment

Choose a reason for hiding this comment

andrewlock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertpi commented Dec 11, 2024

github-actions bot commented Dec 3, 2024 •

edited

Loading

andrewlock commented Dec 3, 2024 •

edited

Loading

andrewlock commented Dec 3, 2024 •

edited

Loading

datadog-ddstaging bot commented Dec 3, 2024 •

edited

Loading

andrewlock commented Dec 3, 2024 •

edited

Loading

andrewlock commented Dec 3, 2024 •

edited

Loading

NachoEchevarria Dec 10, 2024 •

edited

Loading