-
Notifications
You must be signed in to change notification settings - Fork 292
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 XSS support for Freemarker prior 2.3.24-incubating #7497
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 15 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.40.0-SNAPSHOT~e955dade54, baseline=1.40.0-SNAPSHOT~5ddb19db3a
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.05 s) : 0, 1049646
Total [baseline] (10.385 s) : 0, 10385450
Agent [candidate] (1.052 s) : 0, 1052056
Total [candidate] (10.365 s) : 0, 10365321
section appsec
Agent [baseline] (1.181 s) : 0, 1181397
Total [baseline] (10.565 s) : 0, 10565404
Agent [candidate] (1.185 s) : 0, 1184653
Total [candidate] (10.589 s) : 0, 10588723
section iast
Agent [baseline] (1.173 s) : 0, 1172574
Total [baseline] (10.79 s) : 0, 10789761
Agent [candidate] (1.184 s) : 0, 1183647
Total [candidate] (10.827 s) : 0, 10827261
section profiling
Agent [baseline] (1.246 s) : 0, 1246387
Total [baseline] (10.635 s) : 0, 10634831
Agent [candidate] (1.246 s) : 0, 1246116
Total [candidate] (10.539 s) : 0, 10538650
gantt
title petclinic - break down per module: candidate=1.40.0-SNAPSHOT~e955dade54, baseline=1.40.0-SNAPSHOT~5ddb19db3a
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (669.919 ms) : 0, 669919
BytebuddyAgent [candidate] (670.918 ms) : 0, 670918
GlobalTracer [baseline] (306.793 ms) : 0, 306793
GlobalTracer [candidate] (307.882 ms) : 0, 307882
AppSec [baseline] (51.256 ms) : 0, 51256
AppSec [candidate] (51.524 ms) : 0, 51524
Remote Config [baseline] (678.691 µs) : 0, 679
Remote Config [candidate] (688.121 µs) : 0, 688
Telemetry [baseline] (7.444 ms) : 0, 7444
Telemetry [candidate] (7.46 ms) : 0, 7460
section appsec
BytebuddyAgent [baseline] (688.572 ms) : 0, 688572
BytebuddyAgent [candidate] (691.46 ms) : 0, 691460
GlobalTracer [baseline] (299.303 ms) : 0, 299303
GlobalTracer [candidate] (300.843 ms) : 0, 300843
AppSec [baseline] (159.772 ms) : 0, 159772
AppSec [candidate] (159.525 ms) : 0, 159525
IAST [baseline] (21.338 ms) : 0, 21338
IAST [candidate] (20.079 ms) : 0, 20079
Remote Config [baseline] (628.143 µs) : 0, 628
Remote Config [candidate] (631.665 µs) : 0, 632
Telemetry [baseline] (8.919 ms) : 0, 8919
Telemetry [candidate] (8.552 ms) : 0, 8552
section iast
BytebuddyAgent [baseline] (780.211 ms) : 0, 780211
BytebuddyAgent [candidate] (787.265 ms) : 0, 787265
GlobalTracer [baseline] (295.451 ms) : 0, 295451
GlobalTracer [candidate] (298.092 ms) : 0, 298092
AppSec [baseline] (54.678 ms) : 0, 54678
AppSec [candidate] (53.751 ms) : 0, 53751
IAST [baseline] (20.793 ms) : 0, 20793
IAST [candidate] (22.927 ms) : 0, 22927
Remote Config [baseline] (600.73 µs) : 0, 601
Remote Config [candidate] (591.621 µs) : 0, 592
Telemetry [baseline] (7.279 ms) : 0, 7279
Telemetry [candidate] (7.323 ms) : 0, 7323
section profiling
BytebuddyAgent [baseline] (664.03 ms) : 0, 664030
BytebuddyAgent [candidate] (663.752 ms) : 0, 663752
GlobalTracer [baseline] (387.868 ms) : 0, 387868
GlobalTracer [candidate] (388.739 ms) : 0, 388739
AppSec [baseline] (52.399 ms) : 0, 52399
AppSec [candidate] (52.04 ms) : 0, 52040
Remote Config [baseline] (692.246 µs) : 0, 692
Remote Config [candidate] (680.614 µs) : 0, 681
Telemetry [baseline] (7.394 ms) : 0, 7394
Telemetry [candidate] (7.302 ms) : 0, 7302
ProfilingAgent [baseline] (96.238 ms) : 0, 96238
ProfilingAgent [candidate] (95.9 ms) : 0, 95900
Profiling [baseline] (96.262 ms) : 0, 96262
Profiling [candidate] (95.923 ms) : 0, 95923
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.40.0-SNAPSHOT~e955dade54, baseline=1.40.0-SNAPSHOT~5ddb19db3a
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.048 s) : 0, 1048392
Total [baseline] (8.501 s) : 0, 8501328
Agent [candidate] (1.05 s) : 0, 1050427
Total [candidate] (8.493 s) : 0, 8493480
section iast
Agent [baseline] (1.173 s) : 0, 1173198
Total [baseline] (8.958 s) : 0, 8957842
Agent [candidate] (1.183 s) : 0, 1182943
Total [candidate] (8.961 s) : 0, 8961339
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.173 s) : 0, 1172722
Total [baseline] (8.953 s) : 0, 8952787
Agent [candidate] (1.174 s) : 0, 1174385
Total [candidate] (8.933 s) : 0, 8933498
section iast_TELEMETRY_OFF
Agent [baseline] (1.169 s) : 0, 1168641
Total [baseline] (8.942 s) : 0, 8941660
Agent [candidate] (1.18 s) : 0, 1180030
Total [candidate] (8.973 s) : 0, 8973067
gantt
title insecure-bank - break down per module: candidate=1.40.0-SNAPSHOT~e955dade54, baseline=1.40.0-SNAPSHOT~5ddb19db3a
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (669.3 ms) : 0, 669300
BytebuddyAgent [candidate] (669.773 ms) : 0, 669773
GlobalTracer [baseline] (306.176 ms) : 0, 306176
GlobalTracer [candidate] (307.395 ms) : 0, 307395
AppSec [baseline] (51.255 ms) : 0, 51255
AppSec [candidate] (51.463 ms) : 0, 51463
Remote Config [baseline] (681.422 µs) : 0, 681
Remote Config [candidate] (701.304 µs) : 0, 701
Telemetry [baseline] (7.437 ms) : 0, 7437
Telemetry [candidate] (7.524 ms) : 0, 7524
section iast
BytebuddyAgent [baseline] (780.451 ms) : 0, 780451
BytebuddyAgent [candidate] (787.106 ms) : 0, 787106
GlobalTracer [baseline] (295.219 ms) : 0, 295219
GlobalTracer [candidate] (297.83 ms) : 0, 297830
AppSec [baseline] (52.71 ms) : 0, 52710
AppSec [candidate] (53.021 ms) : 0, 53021
IAST [baseline] (23.335 ms) : 0, 23335
IAST [candidate] (23.442 ms) : 0, 23442
Remote Config [baseline] (620.89 µs) : 0, 621
Remote Config [candidate] (591.904 µs) : 0, 592
Telemetry [baseline] (7.301 ms) : 0, 7301
Telemetry [candidate] (7.261 ms) : 0, 7261
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (779.63 ms) : 0, 779630
BytebuddyAgent [candidate] (780.199 ms) : 0, 780199
GlobalTracer [baseline] (295.481 ms) : 0, 295481
GlobalTracer [candidate] (296.223 ms) : 0, 296223
AppSec [baseline] (51.885 ms) : 0, 51885
AppSec [candidate] (52.176 ms) : 0, 52176
IAST [baseline] (23.46 ms) : 0, 23460
IAST [candidate] (23.523 ms) : 0, 23523
Remote Config [baseline] (599.644 µs) : 0, 600
Remote Config [candidate] (603.28 µs) : 0, 603
Telemetry [baseline] (8.075 ms) : 0, 8075
Telemetry [candidate] (8.056 ms) : 0, 8056
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (775.921 ms) : 0, 775921
BytebuddyAgent [candidate] (784.184 ms) : 0, 784184
GlobalTracer [baseline] (295.385 ms) : 0, 295385
GlobalTracer [candidate] (298.2 ms) : 0, 298200
AppSec [baseline] (51.758 ms) : 0, 51758
AppSec [candidate] (51.873 ms) : 0, 51873
IAST [baseline] (23.27 ms) : 0, 23270
IAST [candidate] (24.22 ms) : 0, 24220
Remote Config [baseline] (599.853 µs) : 0, 600
Remote Config [candidate] (606.47 µs) : 0, 606
Telemetry [baseline] (8.115 ms) : 0, 8115
Telemetry [candidate] (7.254 ms) : 0, 7254
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 18 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.40.0-SNAPSHOT~e955dade54, baseline=1.40.0-SNAPSHOT~5ddb19db3a
dateFormat X
axisFormat %s
section baseline
no_agent (1.338 ms) : 1318, 1358
. : milestone, 1338,
appsec (1.711 ms) : 1688, 1735
. : milestone, 1711,
appsec_no_iast (1.73 ms) : 1706, 1755
. : milestone, 1730,
iast (1.488 ms) : 1465, 1511
. : milestone, 1488,
profiling (1.489 ms) : 1465, 1512
. : milestone, 1489,
tracing (1.462 ms) : 1438, 1486
. : milestone, 1462,
section candidate
no_agent (1.351 ms) : 1332, 1370
. : milestone, 1351,
appsec (1.737 ms) : 1714, 1761
. : milestone, 1737,
appsec_no_iast (1.73 ms) : 1706, 1754
. : milestone, 1730,
iast (1.486 ms) : 1463, 1508
. : milestone, 1486,
profiling (1.522 ms) : 1497, 1547
. : milestone, 1522,
tracing (1.481 ms) : 1457, 1504
. : milestone, 1481,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.40.0-SNAPSHOT~e955dade54, baseline=1.40.0-SNAPSHOT~5ddb19db3a
dateFormat X
axisFormat %s
section baseline
no_agent (367.993 µs) : 348, 388
. : milestone, 368,
iast (483.692 µs) : 462, 505
. : milestone, 484,
iast_FULL (611.515 µs) : 590, 633
. : milestone, 612,
iast_GLOBAL (512.739 µs) : 491, 535
. : milestone, 513,
iast_HARDCODED_SECRET_DISABLED (485.064 µs) : 463, 507
. : milestone, 485,
iast_INACTIVE (448.708 µs) : 428, 470
. : milestone, 449,
iast_TELEMETRY_OFF (480.594 µs) : 457, 504
. : milestone, 481,
tracing (441.431 µs) : 421, 462
. : milestone, 441,
section candidate
no_agent (376.455 µs) : 357, 396
. : milestone, 376,
iast (484.9 µs) : 463, 507
. : milestone, 485,
iast_FULL (553.115 µs) : 532, 574
. : milestone, 553,
iast_GLOBAL (525.473 µs) : 504, 546
. : milestone, 525,
iast_HARDCODED_SECRET_DISABLED (492.66 µs) : 471, 514
. : milestone, 493,
iast_INACTIVE (446.897 µs) : 426, 468
. : milestone, 447,
iast_TELEMETRY_OFF (478.536 µs) : 455, 502
. : milestone, 479,
tracing (448.331 µs) : 428, 469
. : milestone, 448,
Dacapo |
...4/src/main/java/datadog/trace/instrumentation/freemarker24/ObjectWrapperInstrumentation.java
Outdated
Show resolved
Hide resolved
.../src/main/java/datadog/trace/instrumentation/freemarker24/DollarVariableInstrumentation.java
Outdated
Show resolved
Hide resolved
...mentation/freemarker-2.3.24/src/main/java/freemarker/core/DollarVariable24DatadogAdvice.java
Outdated
Show resolved
Hide resolved
...9/src/main/java/datadog/trace/instrumentation/freemarker9/DollarVariableInstrumentation.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,11 @@ | |||
<#ftl output_format="HTML" auto_esc=false> |
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.
Can we add a test with auto_esc=true
and validate that we do not trigger the vuln?
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.
Addressed in this PR #7532
@@ -0,0 +1,8 @@ | |||
<html> |
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.
There are some directives for escaping in freemarker prior to 2.3.24, it might be interesting to try them
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.
Same as above #7532
...3.9/src/main/java/datadog/trace/instrumentation/freemarker9/DollarVariableDatadogAdvice.java
Outdated
Show resolved
Hide resolved
...3.9/src/main/java/datadog/trace/instrumentation/freemarker9/DollarVariableDatadogAdvice.java
Outdated
Show resolved
Hide resolved
...nt/instrumentation/freemarker-2.3.9/src/main/java/freemarker/core/DollarVariable9Helper.java
Outdated
Show resolved
Hide resolved
...nstrumentation/freemarker-2.3.9/src/main/java/freemarker/core/DollarVariable2_3_9Helper.java
Show resolved
Hide resolved
@@ -226,6 +226,7 @@ include ':dd-java-agent:instrumentation:elasticsearch:transport-7.3' | |||
include ':dd-java-agent:instrumentation:enable-wallclock-profiling' | |||
include ':dd-java-agent:instrumentation:exception-profiling' | |||
include ':dd-java-agent:instrumentation:finatra-2.9' | |||
include ':dd-java-agent:instrumentation:freemarker-2.3.9' |
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.
when multiple version of the same instrumentation exists we tent to group them under a top folder (i.e. freemarker
) (see conventions)
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.
This is being done in the following PR #7579. Once merged I'll do the change in this one
@@ -226,6 +226,7 @@ include ':dd-java-agent:instrumentation:elasticsearch:transport-7.3' | |||
include ':dd-java-agent:instrumentation:enable-wallclock-profiling' | |||
include ':dd-java-agent:instrumentation:exception-profiling' | |||
include ':dd-java-agent:instrumentation:finatra-2.9' | |||
include ':dd-java-agent:instrumentation:freemarker-2.3.9' | |||
include ':dd-java-agent:instrumentation:freemarker-2.3.24' |
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 would have added the freemarker-2.3.9 module as a test implementation of this one in order to test that only one instrumentation applies as expected. Even if muzzle checks that, adding this is a good practice
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.
Already added :)
What Does This Do
Adds support to the detection of XSS in the Freemarker library prior to the 2.3.24-incubating version
Motivation
Being able to detect XSS in the library of Freemarker
Additional Notes
The PR that adds support to newer versions is this one --> #7532
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-11285