-
Notifications
You must be signed in to change notification settings - Fork 150
[ASM] Taint request body #4080
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ASM] Taint request body #4080
Conversation
Datadog ReportBranch report: ✅ |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0e2ee35 to
423835b
Compare
Datadog ReportBranch report: ❌ ❌ Failed Tests (28)
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
robertpi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a suggestion for optimization, but seems good otherwise.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4dfbf1c to
8046d56
Compare
This comment has been minimized.
This comment has been minimized.
This reverts commit f5681dd.
94addbd to
a10e7b7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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:
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 (4080) - mean (3,005ms) : 2894, 3117
. : milestone, 3005,
master - mean (3,004ms) : 2867, 3141
. : milestone, 3004,
section CallTarget+Inlining+NGEN
This PR (4080) - mean (3,774ms) : 3704, 3843
. : milestone, 3774,
master - mean (3,765ms) : 3677, 3852
. : milestone, 3765,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4080) - mean (3,129ms) : 3026, 3231
. : milestone, 3129,
master - mean (3,143ms) : 3033, 3254
. : milestone, 3143,
section CallTarget+Inlining+NGEN
This PR (4080) - mean (3,580ms) : 3492, 3667
. : milestone, 3580,
master - mean (3,578ms) : 3477, 3679
. : milestone, 3578,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4080) - mean (3,113ms) : 2975, 3251
. : milestone, 3113,
master - mean (3,101ms) : 2958, 3244
. : milestone, 3101,
section CallTarget+Inlining+NGEN
This PR (4080) - mean (3,537ms) : 3435, 3639
. : milestone, 3537,
master - mean (3,537ms) : 3437, 3637
. : milestone, 3537,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4080) - mean (194ms) : 189, 199
. : milestone, 194,
master - mean (185ms) : 176, 193
. : milestone, 185,
section CallTarget+Inlining+NGEN
This PR (4080) - mean (1,027ms) : 992, 1062
. : milestone, 1027,
master - mean (1,004ms) : 982, 1026
. : milestone, 1004,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4080) - mean (364ms) : 360, 367
. : milestone, 364,
master - mean (364ms) : 357, 371
. : milestone, 364,
section CallTarget+Inlining+NGEN
This PR (4080) - mean (1,106ms) : 1085, 1128
. : milestone, 1106,
master - mean (1,103ms) : 1079, 1126
. : milestone, 1103,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4080) - mean (352ms) : 347, 358
. : milestone, 352,
master - mean (351ms) : 346, 357
. : milestone, 351,
section CallTarget+Inlining+NGEN
This PR (4080) - mean (1,055ms) : 1031, 1079
. : milestone, 1055,
master - mean (1,052ms) : 1027, 1078
. : milestone, 1052,
|
Benchmarks Report 🐌Benchmarks for #4080 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Throughput/Crank Report:zap: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 (4080) (10.998M) : 0, 10998114
master (11.316M) : 0, 11315615
benchmarks/2.30.0 (11.327M) : 0, 11326709
benchmarks/2.9.0 (11.162M) : 0, 11161538
section Automatic
This PR (4080) (7.857M) : 0, 7857344
master (7.955M) : 0, 7954979
benchmarks/2.30.0 (7.858M) : 0, 7857987
benchmarks/2.9.0 (8.099M) : 0, 8099075
section Trace stats
master (7.958M) : 0, 7957963
benchmarks/2.30.0 (7.824M) : 0, 7824267
section Manual
This PR (4080) (9.701M) : 0, 9700584
master (9.977M) : 0, 9977251
benchmarks/2.30.0 (10.102M) : 0, 10101646
section Manual + Automatic
This PR (4080) (7.435M) : 0, 7435013
master (7.656M) : 0, 7655837
benchmarks/2.30.0 (7.554M) : 0, 7553853
section Version Conflict
master (6.909M) : 0, 6908693
benchmarks/2.30.0 (6.855M) : 0, 6855103
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (4080) (9.473M) : 0, 9472865
master (9.677M) : 0, 9676680
benchmarks/2.30.0 (9.576M) : 0, 9575959
benchmarks/2.9.0 (9.705M) : 0, 9704781
section Automatic
This PR (4080) (6.775M) : 0, 6775122
master (6.912M) : 0, 6911855
benchmarks/2.30.0 (6.907M) : 0, 6906859
section Trace stats
master (6.815M) : 0, 6815156
benchmarks/2.30.0 (6.852M) : 0, 6852240
section Manual
This PR (4080) (8.516M) : 0, 8516253
master (8.405M) : 0, 8404707
benchmarks/2.30.0 (8.238M) : 0, 8238091
section Manual + Automatic
This PR (4080) (6.636M) : 0, 6636140
master (6.343M) : 0, 6343365
benchmarks/2.30.0 (6.616M) : 0, 6615730
section Version Conflict
master (6.033M) : 0, 6032976
benchmarks/2.30.0 (6.030M) : 0, 6030374
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (4080) (10.037M) : 0, 10036541
master (9.908M) : 0, 9907712
benchmarks/2.30.0 (10.356M) : 0, 10355657
benchmarks/2.9.0 (9.827M) : 0, 9827121
section Automatic
This PR (4080) (7.174M) : crit ,0, 7174211
master (7.628M) : 0, 7628164
benchmarks/2.30.0 (7.232M) : 0, 7231961
benchmarks/2.9.0 (7.246M) : 0, 7246397
section Trace stats
master (7.404M) : 0, 7403510
benchmarks/2.30.0 (7.389M) : 0, 7389346
section Manual
This PR (4080) (8.888M) : crit ,0, 8888175
master (9.534M) : 0, 9534015
benchmarks/2.30.0 (9.091M) : 0, 9091062
section Manual + Automatic
This PR (4080) (7.083M) : 0, 7083272
master (7.400M) : 0, 7400474
benchmarks/2.30.0 (6.850M) : 0, 6849618
section Version Conflict
master (6.735M) : 0, 6735450
benchmarks/2.30.0 (6.115M) : 0, 6115255
gantt
title Throughput Linux x64 (ASM) (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (4080) (7.498M) : 0, 7498119
master (7.421M) : 0, 7421296
benchmarks/2.30.0 (7.197M) : 0, 7197004
benchmarks/2.9.0 (7.748M) : 0, 7748029
section No attack
This PR (4080) (2.384M) : 0, 2384133
master (2.403M) : 0, 2403011
benchmarks/2.30.0 (2.367M) : 0, 2366569
benchmarks/2.9.0 (3.274M) : 0, 3273699
section Attack
This PR (4080) (2.025M) : 0, 2024685
master (2.033M) : 0, 2032674
benchmarks/2.30.0 (1.983M) : 0, 1982525
benchmarks/2.9.0 (2.598M) : 0, 2597950
section Blocking
This PR (4080) (4.042M) : 0, 4042052
master (4.078M) : 0, 4078429
benchmarks/2.30.0 (3.949M) : 0, 3948532
|
|
Thank you for your time and feedback!!! |
Summary of changes
This PR implements a way to taint the values coming from the requests body in order to check if they are used in a vulnerable call.
Reason for change
It is a required feature for this quarter.
Implementation details
In ASM, the request body was already been analyzed, so we will use the same mechanisms for IAST, In IAST, we will only consider the string values received.
ASM has an instrumentation point, in (DefaultModelBindingContext_SetResult_Integration.cs), that we will use for IAST.
Test coverage
Some integration tests have been added.
Other details