-
Notifications
You must be signed in to change notification settings - Fork 293
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
Add detection of untrusted deserialization in snakeyaml library #7406
Add detection of untrusted deserialization in snakeyaml library #7406
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 46 metrics, 16 unstable metrics.
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.547 s) : 0, 1546603
Total [baseline] (11.795 s) : 0, 11795459
Agent [candidate] (1.555 s) : 0, 1554748
Total [candidate] (11.846 s) : 0, 11845568
section iast
Agent [baseline] (1.72 s) : 0, 1719770
Total [baseline] (12.486 s) : 0, 12485635
Agent [candidate] (1.717 s) : 0, 1717142
Total [candidate] (12.54 s) : 0, 12539855
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.721 s) : 0, 1721499
Total [baseline] (12.498 s) : 0, 12498084
Agent [candidate] (1.715 s) : 0, 1714567
Total [candidate] (12.494 s) : 0, 12494059
section iast_TELEMETRY_OFF
Agent [baseline] (1.71 s) : 0, 1709579
Total [baseline] (12.469 s) : 0, 12469225
Agent [candidate] (1.71 s) : 0, 1709584
Total [candidate] (12.468 s) : 0, 12468115
gantt
title insecure-bank - break down per module: candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (990.935 ms) : 0, 990935
BytebuddyAgent [candidate] (995.997 ms) : 0, 995997
GlobalTracer [baseline] (452.768 ms) : 0, 452768
GlobalTracer [candidate] (455.051 ms) : 0, 455051
AppSec [baseline] (72.265 ms) : 0, 72265
AppSec [candidate] (72.779 ms) : 0, 72779
Remote Config [baseline] (825.855 µs) : 0, 826
Remote Config [candidate] (838.115 µs) : 0, 838
Telemetry [baseline] (9.802 ms) : 0, 9802
Telemetry [candidate] (9.868 ms) : 0, 9868
section iast
BytebuddyAgent [baseline] (1.15 s) : 0, 1149823
BytebuddyAgent [candidate] (1.149 s) : 0, 1149441
GlobalTracer [baseline] (435.593 ms) : 0, 435593
GlobalTracer [candidate] (434.975 ms) : 0, 434975
AppSec [baseline] (70.539 ms) : 0, 70539
AppSec [candidate] (70.945 ms) : 0, 70945
IAST [baseline] (31.04 ms) : 0, 31040
IAST [candidate] (30.903 ms) : 0, 30903
Remote Config [baseline] (762.931 µs) : 0, 763
Remote Config [candidate] (741.481 µs) : 0, 741
Telemetry [baseline] (11.922 ms) : 0, 11922
Telemetry [candidate] (10.004 ms) : 0, 10004
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (1.151 s) : 0, 1150651
BytebuddyAgent [candidate] (1.146 s) : 0, 1145811
GlobalTracer [baseline] (436.989 ms) : 0, 436989
GlobalTracer [candidate] (434.856 ms) : 0, 434856
AppSec [baseline] (76.579 ms) : 0, 76579
AppSec [candidate] (71.962 ms) : 0, 71962
IAST [baseline] (27.249 ms) : 0, 27249
IAST [candidate] (31.074 ms) : 0, 31074
Remote Config [baseline] (724.366 µs) : 0, 724
Remote Config [candidate] (723.365 µs) : 0, 723
Telemetry [baseline] (9.199 ms) : 0, 9199
Telemetry [candidate] (10.034 ms) : 0, 10034
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (1.143 s) : 0, 1142537
BytebuddyAgent [candidate] (1.142 s) : 0, 1142482
GlobalTracer [baseline] (433.881 ms) : 0, 433881
GlobalTracer [candidate] (434.617 ms) : 0, 434617
AppSec [baseline] (71.047 ms) : 0, 71047
AppSec [candidate] (72.484 ms) : 0, 72484
IAST [baseline] (30.314 ms) : 0, 30314
IAST [candidate] (30.119 ms) : 0, 30119
Remote Config [baseline] (818.866 µs) : 0, 819
Remote Config [candidate] (732.334 µs) : 0, 732
Telemetry [baseline] (10.945 ms) : 0, 10945
Telemetry [candidate] (9.041 ms) : 0, 9041
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.547 s) : 0, 1546881
Total [baseline] (14.201 s) : 0, 14200910
Agent [candidate] (1.552 s) : 0, 1551506
Total [candidate] (14.259 s) : 0, 14258684
section appsec
Agent [baseline] (1.736 s) : 0, 1735919
Total [baseline] (14.379 s) : 0, 14379402
Agent [candidate] (1.738 s) : 0, 1737773
Total [candidate] (14.478 s) : 0, 14478455
section iast
Agent [baseline] (1.722 s) : 0, 1722189
Total [baseline] (14.812 s) : 0, 14812473
Agent [candidate] (1.724 s) : 0, 1723940
Total [candidate] (14.883 s) : 0, 14883416
section profiling
Agent [baseline] (1.863 s) : 0, 1863172
Total [baseline] (14.55 s) : 0, 14549687
Agent [candidate] (1.862 s) : 0, 1861577
Total [candidate] (14.588 s) : 0, 14588275
gantt
title petclinic - break down per module: candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (990.964 ms) : 0, 990964
BytebuddyAgent [candidate] (994.004 ms) : 0, 994004
GlobalTracer [baseline] (452.909 ms) : 0, 452909
GlobalTracer [candidate] (454.521 ms) : 0, 454521
AppSec [baseline] (72.447 ms) : 0, 72447
AppSec [candidate] (72.25 ms) : 0, 72250
Remote Config [baseline] (817.564 µs) : 0, 818
Remote Config [candidate] (819.76 µs) : 0, 820
Telemetry [baseline] (9.759 ms) : 0, 9759
Telemetry [candidate] (9.802 ms) : 0, 9802
section appsec
BytebuddyAgent [baseline] (1.01 s) : 0, 1009929
BytebuddyAgent [candidate] (1.01 s) : 0, 1009539
GlobalTracer [baseline] (446.744 ms) : 0, 446744
GlobalTracer [candidate] (447.964 ms) : 0, 447964
AppSec [baseline] (235.504 ms) : 0, 235504
AppSec [candidate] (235.247 ms) : 0, 235247
Remote Config [baseline] (753.999 µs) : 0, 754
Remote Config [candidate] (758.729 µs) : 0, 759
Telemetry [baseline] (10.299 ms) : 0, 10299
Telemetry [candidate] (12.053 ms) : 0, 12053
IAST [baseline] (25.356 ms) : 0, 25356
IAST [candidate] (24.267 ms) : 0, 24267
section iast
BytebuddyAgent [baseline] (1.153 s) : 0, 1152545
BytebuddyAgent [candidate] (1.154 s) : 0, 1153719
GlobalTracer [baseline] (436.21 ms) : 0, 436210
GlobalTracer [candidate] (436.009 ms) : 0, 436009
AppSec [baseline] (71.332 ms) : 0, 71332
AppSec [candidate] (72.185 ms) : 0, 72185
Remote Config [baseline] (754.205 µs) : 0, 754
Remote Config [candidate] (758.284 µs) : 0, 758
Telemetry [baseline] (10.873 ms) : 0, 10873
Telemetry [candidate] (12.569 ms) : 0, 12569
IAST [baseline] (30.369 ms) : 0, 30369
IAST [candidate] (28.525 ms) : 0, 28525
section profiling
BytebuddyAgent [baseline] (988.816 ms) : 0, 988816
BytebuddyAgent [candidate] (986.382 ms) : 0, 986382
GlobalTracer [baseline] (584.966 ms) : 0, 584966
GlobalTracer [candidate] (583.984 ms) : 0, 583984
AppSec [baseline] (73.983 ms) : 0, 73983
AppSec [candidate] (73.519 ms) : 0, 73519
Remote Config [baseline] (881.037 µs) : 0, 881
Remote Config [candidate] (885.477 µs) : 0, 885
Telemetry [baseline] (9.452 ms) : 0, 9452
Telemetry [candidate] (9.434 ms) : 0, 9434
ProfilingAgent [baseline] (149.059 ms) : 0, 149059
ProfilingAgent [candidate] (151.445 ms) : 0, 151445
Profiling [baseline] (149.134 ms) : 0, 149134
Profiling [candidate] (151.517 ms) : 0, 151517
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 6 metrics, 22 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section baseline
no_agent (445.531 µs) : 417, 474
. : milestone, 446,
iast (582.934 µs) : 550, 616
. : milestone, 583,
iast_FULL (678.412 µs) : 645, 711
. : milestone, 678,
iast_GLOBAL (609.805 µs) : 577, 643
. : milestone, 610,
iast_HARDCODED_SECRET_DISABLED (577.43 µs) : 545, 610
. : milestone, 577,
iast_INACTIVE (544.488 µs) : 514, 575
. : milestone, 544,
iast_TELEMETRY_OFF (575.897 µs) : 544, 608
. : milestone, 576,
tracing (532.717 µs) : 503, 562
. : milestone, 533,
section candidate
no_agent (442.329 µs) : 413, 471
. : milestone, 442,
iast (591.803 µs) : 559, 625
. : milestone, 592,
iast_FULL (680.171 µs) : 647, 713
. : milestone, 680,
iast_GLOBAL (621.342 µs) : 588, 655
. : milestone, 621,
iast_HARDCODED_SECRET_DISABLED (590.128 µs) : 556, 624
. : milestone, 590,
iast_INACTIVE (539.959 µs) : 510, 570
. : milestone, 540,
iast_TELEMETRY_OFF (571.719 µs) : 539, 605
. : milestone, 572,
tracing (534.208 µs) : 504, 564
. : milestone, 534,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section baseline
no_agent (1.695 ms) : 1670, 1720
. : milestone, 1695,
appsec (2.18 ms) : 2149, 2211
. : milestone, 2180,
appsec_no_iast (2.189 ms) : 2156, 2222
. : milestone, 2189,
iast (1.873 ms) : 1843, 1903
. : milestone, 1873,
profiling (1.95 ms) : 1913, 1987
. : milestone, 1950,
tracing (1.866 ms) : 1834, 1897
. : milestone, 1866,
section candidate
no_agent (1.689 ms) : 1663, 1714
. : milestone, 1689,
appsec (2.198 ms) : 2166, 2229
. : milestone, 2198,
appsec_no_iast (2.198 ms) : 2166, 2230
. : milestone, 2198,
iast (1.86 ms) : 1830, 1889
. : milestone, 1860,
profiling (1.946 ms) : 1909, 1982
. : milestone, 1946,
tracing (1.835 ms) : 1802, 1867
. : milestone, 1835,
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 biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section baseline
no_agent (20.553 s) : 20553000, 20553000
. : milestone, 20553000,
appsec (21.507 s) : 21507000, 21507000
. : milestone, 21507000,
iast (24.724 s) : 24724000, 24724000
. : milestone, 24724000,
iast_GLOBAL (24.943 s) : 24943000, 24943000
. : milestone, 24943000,
profiling (22.015 s) : 22015000, 22015000
. : milestone, 22015000,
tracing (21.867 s) : 21867000, 21867000
. : milestone, 21867000,
section candidate
no_agent (20.605 s) : 20605000, 20605000
. : milestone, 20605000,
appsec (21.546 s) : 21546000, 21546000
. : milestone, 21546000,
iast (23.945 s) : 23945000, 23945000
. : milestone, 23945000,
iast_GLOBAL (24.962 s) : 24962000, 24962000
. : milestone, 24962000,
profiling (21.095 s) : 21095000, 21095000
. : milestone, 21095000,
tracing (21.54 s) : 21540000, 21540000
. : milestone, 21540000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.39.0-SNAPSHOT~c3f4563aad, baseline=1.39.0-SNAPSHOT~f6c87de39a
dateFormat X
axisFormat %s
section baseline
no_agent (1.541 ms) : 1529, 1554
. : milestone, 1541,
appsec (2.726 ms) : 2663, 2788
. : milestone, 2726,
iast (2.35 ms) : 2277, 2422
. : milestone, 2350,
iast_GLOBAL (2.431 ms) : 2355, 2507
. : milestone, 2431,
profiling (2.223 ms) : 2160, 2287
. : milestone, 2223,
tracing (2.178 ms) : 2120, 2237
. : milestone, 2178,
section candidate
no_agent (1.543 ms) : 1531, 1556
. : milestone, 1543,
appsec (2.72 ms) : 2657, 2783
. : milestone, 2720,
iast (2.386 ms) : 2310, 2461
. : milestone, 2386,
iast_GLOBAL (2.412 ms) : 2337, 2486
. : milestone, 2412,
profiling (2.209 ms) : 2148, 2270
. : milestone, 2209,
tracing (2.181 ms) : 2122, 2240
. : milestone, 2181,
|
...agent/agent-iast/src/main/java/com/datadog/iast/sink/UntrustedDeserializationModuleImpl.java
Outdated
Show resolved
Hide resolved
...n/snakeyaml/src/main/java/datadog/trace/instrumentation/snakeyaml/SnakeYamlInstrumenter.java
Outdated
Show resolved
Hide resolved
dd-java-agent/instrumentation/snakeyaml/src/test/java/foo/bar/TestSnakeYamlSuite.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/api/iast/sink/UntrustedDeserializationModule.java
Outdated
Show resolved
Hide resolved
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.
IMHO we can improve it a little bit removing some complexity like the multiple Module methods
...t/agent-iast/src/test/groovy/com/datadog/iast/sink/UntrustedDeserializationModuleTest.groovy
Outdated
Show resolved
Hide resolved
...t/agent-iast/src/test/groovy/com/datadog/iast/sink/UntrustedDeserializationModuleTest.groovy
Outdated
Show resolved
Hide resolved
...n/snakeyaml/src/main/java/datadog/trace/instrumentation/snakeyaml/SnakeYamlInstrumenter.java
Outdated
Show resolved
Hide resolved
injectSysConfig('dd.iast.enabled', 'true') | ||
} | ||
|
||
void 'test snakeyaml load with an input stream'() { |
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.
We can simplify this test using one method with a where condition. For instance
void 'test snakeyaml load method'() {
given:
final module = Mock(UntrustedDeserializationModule)
InstrumentationBridge.registerIastModule(module)
when:
new Yaml().load(obj)
then:
1 * module.onObject(_)
where:
obj | _
new ByteArrayInputStream("test".getBytes()) | _
new StringReader("test") | _
"test" | _
}
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.
In this case I don't it could be possible as the load method only accepts a specific type which are InputStream, Reader or String. I cannot pass to it the Object method. One option is to do a casting previous to make the call but IMHO I think it is better to leave it as it is and don't try to simplify it as it will be hard to follow the test
@@ -14,10 +13,10 @@ public UntrustedDeserializationModuleImpl(final Dependencies dependencies) { | |||
} | |||
|
|||
@Override | |||
public void onInputStream(@Nullable InputStream is) { | |||
if (is == null) { | |||
public void onObject(@Nullable Object object) { |
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.
Would you mind creating a separate PR with just this change?
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.
FYI the PR where the change will be made is #7484
After merging it I'll rebase this branch with master
What Does This Do
Adds instrumentation for snakeyaml library versions prior to 2.0
Motivation
Detect untrusted deserialization vulnerability for the load method in the Yaml class of the snakeyaml library
Additional Notes
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-54452