-
Notifications
You must be signed in to change notification settings - Fork 304
Optimize IAST Vulnerability Detection #8885
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
base: master
Are you sure you want to change the base?
Optimize IAST Vulnerability Detection #8885
Conversation
dd-java-agent/agent-iast/src/main/java/com/datadog/iast/overhead/OverheadController.java
Outdated
Show resolved
Hide resolved
dd-java-agent/agent-iast/src/main/java/com/datadog/iast/overhead/OverheadController.java
Outdated
Show resolved
Hide resolved
dd-java-agent/agent-iast/src/main/java/com/datadog/iast/overhead/OverheadController.java
Outdated
Show resolved
Hide resolved
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 1 performance regressions! Performance is the same for 56 metrics, 14 unstable metrics.
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.50.0-SNAPSHOT~73d972ecc4, baseline=1.50.0-SNAPSHOT~7787af738f
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.025 s) : 0, 1024517
Total [baseline] (8.539 s) : 0, 8539338
Agent [candidate] (1.03 s) : 0, 1029629
Total [candidate] (8.524 s) : 0, 8524051
section iast
Agent [baseline] (1.148 s) : 0, 1148149
Total [baseline] (9.167 s) : 0, 9166991
Agent [candidate] (1.156 s) : 0, 1156254
Total [candidate] (9.146 s) : 0, 9146193
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.159 s) : 0, 1158819
Total [baseline] (9.156 s) : 0, 9155716
Agent [candidate] (1.158 s) : 0, 1158293
Total [candidate] (9.135 s) : 0, 9134712
section iast_TELEMETRY_OFF
Agent [baseline] (1.151 s) : 0, 1150775
Total [baseline] (9.183 s) : 0, 9183222
Agent [candidate] (1.148 s) : 0, 1147794
Total [candidate] (9.178 s) : 0, 9178207
gantt
title insecure-bank - break down per module: candidate=1.50.0-SNAPSHOT~73d972ecc4, baseline=1.50.0-SNAPSHOT~7787af738f
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (683.783 ms) : 0, 683783
BytebuddyAgent [candidate] (687.247 ms) : 0, 687247
GlobalTracer [baseline] (240.915 ms) : 0, 240915
GlobalTracer [candidate] (241.515 ms) : 0, 241515
AppSec [baseline] (57.015 ms) : 0, 57015
AppSec [candidate] (57.38 ms) : 0, 57380
Debugger [baseline] (6.99 ms) : 0, 6990
Debugger [candidate] (6.956 ms) : 0, 6956
Remote Config [baseline] (746.811 µs) : 0, 747
Remote Config [candidate] (739.446 µs) : 0, 739
Telemetry [baseline] (11.505 ms) : 0, 11505
Telemetry [candidate] (12.184 ms) : 0, 12184
section iast
BytebuddyAgent [baseline] (800.187 ms) : 0, 800187
BytebuddyAgent [candidate] (807.261 ms) : 0, 807261
GlobalTracer [baseline] (230.101 ms) : 0, 230101
GlobalTracer [candidate] (230.399 ms) : 0, 230399
IAST [baseline] (26.289 ms) : 0, 26289
IAST [candidate] (28.332 ms) : 0, 28332
AppSec [baseline] (53.613 ms) : 0, 53613
AppSec [candidate] (52.281 ms) : 0, 52281
Debugger [baseline] (5.982 ms) : 0, 5982
Debugger [candidate] (5.91 ms) : 0, 5910
Remote Config [baseline] (594.623 µs) : 0, 595
Remote Config [candidate] (589.101 µs) : 0, 589
Telemetry [baseline] (7.907 ms) : 0, 7907
Telemetry [candidate] (7.917 ms) : 0, 7917
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (808.541 ms) : 0, 808541
BytebuddyAgent [candidate] (806.264 ms) : 0, 806264
GlobalTracer [baseline] (232.052 ms) : 0, 232052
GlobalTracer [candidate] (231.986 ms) : 0, 231986
IAST [baseline] (26.433 ms) : 0, 26433
IAST [candidate] (28.766 ms) : 0, 28766
AppSec [baseline] (53.685 ms) : 0, 53685
AppSec [candidate] (52.198 ms) : 0, 52198
Debugger [baseline] (5.918 ms) : 0, 5918
Debugger [candidate] (5.997 ms) : 0, 5997
Remote Config [baseline] (598.853 µs) : 0, 599
Remote Config [candidate] (589.583 µs) : 0, 590
Telemetry [baseline] (7.993 ms) : 0, 7993
Telemetry [candidate] (8.06 ms) : 0, 8060
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (803.169 ms) : 0, 803169
BytebuddyAgent [candidate] (798.385 ms) : 0, 798385
GlobalTracer [baseline] (230.812 ms) : 0, 230812
GlobalTracer [candidate] (230.85 ms) : 0, 230850
IAST [baseline] (27.768 ms) : 0, 27768
IAST [candidate] (30.408 ms) : 0, 30408
AppSec [baseline] (51.009 ms) : 0, 51009
AppSec [candidate] (50.181 ms) : 0, 50181
Debugger [baseline] (5.983 ms) : 0, 5983
Debugger [candidate] (5.998 ms) : 0, 5998
Remote Config [baseline] (610.09 µs) : 0, 610
Remote Config [candidate] (598.765 µs) : 0, 599
Telemetry [baseline] (7.855 ms) : 0, 7855
Telemetry [candidate] (7.908 ms) : 0, 7908
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.50.0-SNAPSHOT~73d972ecc4, baseline=1.50.0-SNAPSHOT~7787af738f
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.032 s) : 0, 1032004
Total [baseline] (11.058 s) : 0, 11058326
Agent [candidate] (1.026 s) : 0, 1026149
Total [candidate] (11.169 s) : 0, 11168886
section appsec
Agent [baseline] (1.162 s) : 0, 1162141
Total [baseline] (11.191 s) : 0, 11190855
Agent [candidate] (1.163 s) : 0, 1162988
Total [candidate] (11.21 s) : 0, 11209506
section iast
Agent [baseline] (1.165 s) : 0, 1165431
Total [baseline] (11.348 s) : 0, 11347563
Agent [candidate] (1.153 s) : 0, 1152638
Total [candidate] (11.328 s) : 0, 11328012
section profiling
Agent [baseline] (1.275 s) : 0, 1275238
Total [baseline] (11.579 s) : 0, 11579010
Agent [candidate] (1.264 s) : 0, 1264255
Total [candidate] (11.397 s) : 0, 11397026
gantt
title petclinic - break down per module: candidate=1.50.0-SNAPSHOT~73d972ecc4, baseline=1.50.0-SNAPSHOT~7787af738f
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (688.772 ms) : 0, 688772
BytebuddyAgent [candidate] (685.389 ms) : 0, 685389
GlobalTracer [baseline] (242.224 ms) : 0, 242224
GlobalTracer [candidate] (241.139 ms) : 0, 241139
AppSec [baseline] (58.085 ms) : 0, 58085
AppSec [candidate] (57.737 ms) : 0, 57737
Debugger [baseline] (6.162 ms) : 0, 6162
Debugger [candidate] (6.127 ms) : 0, 6127
Remote Config [baseline] (722.507 µs) : 0, 723
Remote Config [candidate] (738.983 µs) : 0, 739
Telemetry [baseline] (12.284 ms) : 0, 12284
Telemetry [candidate] (11.372 ms) : 0, 11372
section appsec
BytebuddyAgent [baseline] (700.114 ms) : 0, 700114
BytebuddyAgent [candidate] (700.078 ms) : 0, 700078
GlobalTracer [baseline] (237.67 ms) : 0, 237670
GlobalTracer [candidate] (237.634 ms) : 0, 237634
IAST [baseline] (21.897 ms) : 0, 21897
IAST [candidate] (23.005 ms) : 0, 23005
AppSec [baseline] (175.958 ms) : 0, 175958
AppSec [candidate] (176.18 ms) : 0, 176180
Debugger [baseline] (5.968 ms) : 0, 5968
Debugger [candidate] (5.987 ms) : 0, 5987
Remote Config [baseline] (629.335 µs) : 0, 629
Remote Config [candidate] (633.584 µs) : 0, 634
Telemetry [baseline] (7.359 ms) : 0, 7359
Telemetry [candidate] (7.379 ms) : 0, 7379
section iast
BytebuddyAgent [baseline] (813.556 ms) : 0, 813556
BytebuddyAgent [candidate] (802.134 ms) : 0, 802134
GlobalTracer [baseline] (233.121 ms) : 0, 233121
GlobalTracer [candidate] (231.04 ms) : 0, 231040
IAST [baseline] (25.627 ms) : 0, 25627
IAST [candidate] (27.72 ms) : 0, 27720
AppSec [baseline] (54.808 ms) : 0, 54808
AppSec [candidate] (53.614 ms) : 0, 53614
Debugger [baseline] (6.024 ms) : 0, 6024
Debugger [candidate] (5.981 ms) : 0, 5981
Remote Config [baseline] (599.64 µs) : 0, 600
Remote Config [candidate] (599.468 µs) : 0, 599
Telemetry [baseline] (7.93 ms) : 0, 7930
Telemetry [candidate] (8.015 ms) : 0, 8015
section profiling
BytebuddyAgent [baseline] (681.493 ms) : 0, 681493
BytebuddyAgent [candidate] (674.662 ms) : 0, 674662
GlobalTracer [baseline] (362.187 ms) : 0, 362187
GlobalTracer [candidate] (360.054 ms) : 0, 360054
AppSec [baseline] (61.596 ms) : 0, 61596
AppSec [candidate] (61.541 ms) : 0, 61541
Debugger [baseline] (6.24 ms) : 0, 6240
Debugger [candidate] (6.073 ms) : 0, 6073
Remote Config [baseline] (669.573 µs) : 0, 670
Remote Config [candidate] (642.745 µs) : 0, 643
Telemetry [baseline] (8.154 ms) : 0, 8154
Telemetry [candidate] (8.07 ms) : 0, 8070
ProfilingAgent [baseline] (103.775 ms) : 0, 103775
ProfilingAgent [candidate] (102.703 ms) : 0, 102703
Profiling [baseline] (103.799 ms) : 0, 103799
Profiling [candidate] (102.728 ms) : 0, 102728
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 2 performance regressions! Performance is the same for 0 metrics, 12 unstable metrics.
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.50.0-SNAPSHOT~73d972ecc4, baseline=1.50.0-SNAPSHOT~7787af738f
dateFormat X
axisFormat %s
section baseline
no_agent (1.476 ms) : 1465, 1488
. : milestone, 1476,
appsec (2.413 ms) : 2364, 2462
. : milestone, 2413,
iast (2.203 ms) : 2142, 2264
. : milestone, 2203,
iast_GLOBAL (2.245 ms) : 2183, 2307
. : milestone, 2245,
profiling (2.046 ms) : 1996, 2096
. : milestone, 2046,
tracing (2.028 ms) : 1980, 2077
. : milestone, 2028,
section candidate
no_agent (1.478 ms) : 1466, 1489
. : milestone, 1478,
appsec (2.412 ms) : 2364, 2461
. : milestone, 2412,
iast (2.195 ms) : 2133, 2257
. : milestone, 2195,
iast_GLOBAL (2.236 ms) : 2174, 2297
. : milestone, 2236,
profiling (2.035 ms) : 1986, 2084
. : milestone, 2035,
tracing (2.018 ms) : 1970, 2066
. : milestone, 2018,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.50.0-SNAPSHOT~73d972ecc4, baseline=1.50.0-SNAPSHOT~7787af738f
dateFormat X
axisFormat %s
section baseline
no_agent (14.951 s) : 14951000, 14951000
. : milestone, 14951000,
appsec (14.597 s) : 14597000, 14597000
. : milestone, 14597000,
iast (18.899 s) : 18899000, 18899000
. : milestone, 18899000,
iast_GLOBAL (18.148 s) : 18148000, 18148000
. : milestone, 18148000,
profiling (15.105 s) : 15105000, 15105000
. : milestone, 15105000,
tracing (15.058 s) : 15058000, 15058000
. : milestone, 15058000,
section candidate
no_agent (15.637 s) : 15637000, 15637000
. : milestone, 15637000,
appsec (15.012 s) : 15012000, 15012000
. : milestone, 15012000,
iast (18.526 s) : 18526000, 18526000
. : milestone, 18526000,
iast_GLOBAL (17.89 s) : 17890000, 17890000
. : milestone, 17890000,
profiling (15.344 s) : 15344000, 15344000
. : milestone, 15344000,
tracing (14.982 s) : 14982000, 14982000
. : milestone, 14982000,
|
600341f
to
416dbae
Compare
…ad/OverheadController.java Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com>
@@ -65,7 +66,11 @@ private void onCookies(final List<Cookie> cookies) { | |||
return; | |||
} | |||
final AgentSpan span = AgentTracer.activeSpan(); | |||
if (!overheadController.consumeQuota(Operations.REPORT_VULNERABILITY, span)) { | |||
// TODO decide if we remove this one quota for all vulnerabilities as new IAST sampling |
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.
@smola What do you think about this?
We had previously implemented it so that only one quota would be consumed for all header/cookie-related vulnerabilities reported here.
The new algorithm would fix this behavior, but maybe it’s worth keeping the existing logic to ensure these vulns (and any future ones like them) are reported sooner.
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.
Yes, I think we should probably preserve the behavior that all header/cookie vulns are a single one for quota purposes.
@@ -0,0 +1,92 @@ | |||
package datadog.smoketest |
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.
I’m not a big fan of making assertions based on line numbers, since changes to the controller can easily break the tests.
However, in this case it’s the simplest way to verify the vulnerabilities, especially since it’s useful to repeat types.
That said, I’ve created a dedicated controller just for this test, so the risk of breaking it due to unrelated changes is minimized.
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.
That's fine. Another option is detecting them based on evidence, if each instance has different evidence. That would be more stable than lines.
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
@@ -46,7 +46,7 @@ public IastContext resolve() { | |||
|
|||
@Override | |||
public IastContext buildRequestContext() { | |||
return new IastRequestContext(globalContext.getTaintedObjects()); | |||
return new IastRequestContext((TaintedObjects) globalContext.getTaintedObjects()); |
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.
Why does this need a cast? getTaintedObjects()
should return TaintedObjects
or a subclass of it already?
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.
I've added this new constructor
/**
* Use this constructor only when you want to create a new context with a fresh overhead context
* (e.g. for testing purposes).
*
* @param overheadContext the overhead context to use
*/
public IastRequestContext(final OverheadContext overheadContext) {
this.vulnerabilityBatch = new VulnerabilityBatch();
this.overheadContext = overheadContext;
this.taintedObjects = TaintedObjects.build(TaintedMap.build(MAP_SIZE));
}
in IastContext we have
/**
* Get the tainted objects dictionary linked to the context, since we have no visibility over the
* {@code TaintedObject} class from here, we use a dirty generics hack.
*/
@Nonnull
<TO> TO getTaintedObjects();
with public IastRequestContext(final TaintedObjects taintedObjects)
and public IastRequestContext(final OverheadContext overheadContext)
we need to cast to specify which constructor should be use
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.
Rollback those changes thanks to a different approach
* Key: HTTP_METHOD + " " + HTTP_PATH Value: Map<vulnerabilityType, count> | ||
*/ | ||
static final Map<String, Map<VulnerabilityType, Integer>> globalMap = | ||
new LinkedHashMap<String, Map<VulnerabilityType, Integer>>(GLOBAL_MAP_MAX_SIZE, 0.75f, true) { |
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.
ConcurrentHashMap
if this is going to be accessed concurrently.
* Global LRU cache mapping each “method + path” key to its historical vulnerabilityCounts map. | ||
* Key: HTTP_METHOD + " " + HTTP_PATH Value: Map<vulnerabilityType, count> | ||
*/ | ||
static final Map<String, Map<VulnerabilityType, Integer>> globalMap = |
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.
Ideally we shouldn't be using Map<VulnerabilityType, Integer>
for the inner map, but fixed-size arrays indexed by VulnerabilityType index. Way more efficient. Also if we use AtomicIntegerArray
for the inner array, we can perform atomic increments of the counters, which will be needed here.
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.
Thanks! I’ve updated all the collections as we agreed offline
String method = null; | ||
String path = null; | ||
if (span != null) { | ||
Object methodTag = span.getLocalRootSpan().getTag(Tags.HTTP_METHOD); |
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.
Get local root span to a variable first, reuse that from there.
@@ -65,7 +66,11 @@ private void onCookies(final List<Cookie> cookies) { | |||
return; | |||
} | |||
final AgentSpan span = AgentTracer.activeSpan(); | |||
if (!overheadController.consumeQuota(Operations.REPORT_VULNERABILITY, span)) { | |||
// TODO decide if we remove this one quota for all vulnerabilities as new IAST sampling |
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.
Yes, I think we should probably preserve the behavior that all header/cookie vulns are a single one for quota purposes.
@@ -0,0 +1,92 @@ | |||
package datadog.smoketest |
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.
That's fine. Another option is detecting them based on evidence, if each instance has different evidence. That would be more stable than lines.
What Does This Do
Changes to OverheadContext
The OverheadContext class has been extended to support three separate tracking maps:
An additional field, isGlobal, has been added to indicate whether the context is global or request-scoped. If isGlobal is true, the maps are not used, and quota checks proceed using the global strategy only.
A new method, resetMaps(), has been added to update globalMap when the request ends and vulnerability data has been reported. Two scenarios are supported:
Changes to OverheadController
The method consumeQuota() has been extended to receive a vulnerabilityType and modified to support the new logic:
If an OverheadContext is present and not global, and there is remaining quota and a valid span, the controller now invokes a new method maybeSkipVulnerability() to determine whether quota should actually be consumed or not, based on endpoint-specific history.
It's better to check the Algorithm execution example flow diagram to understand how this should work
Changes to IastRequestContext
In releaseRequestContext(), the request now calls resetMaps() on the associated OverheadContext, ensuring globalMap is updated at the end of each request.
Motivation
[RFC-1029] Optimizing IAST Vulnerability Detection implementation
Additional Notes
java tracer needs to implement also [RFC-1029-A1] Solution for dynamic http routes
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: APPSEC-57267