Skip XSS check for freemarker built-in escaping expressions in 2.3.24 instrumentation#10865
Open
Skip XSS check for freemarker built-in escaping expressions in 2.3.24 instrumentation#10865
Conversation
…s in 2.3.24 instrumentation
jandro996
added
type: bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92be78dcf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+57
to
+58
| if (ESCAPED_EXPRESSION.get(object) instanceof BuiltIn) { | ||
| return null; |
There was a problem hiding this comment.
The new instanceof BuiltIn guard suppresses XSS reporting for all FreeMarker built-ins, not just escaping ones. In templates like ${name?upper_case} or ${name?replace("a","b")}, autoEscape can still be false and output remains unsanitized, but this branch now returns null and skips onXss, creating false negatives. The check needs to distinguish escaping built-ins specifically instead of blanket-skipping every BuiltIn expression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@jandro996 can you review this?
Member
Author
There was a problem hiding this comment.
I'm working on that, sorry for not add 👀 to the comment 😅
Member
Author
There was a problem hiding this comment.
Fixed. Replaced instanceof BuiltIn with instanceof BuiltInForLegacyEscaping, the abstract base class that covers exactly ?html, ?xml, and ?xhtml in freemarker 2.3.24. Non-escaping built-ins like ?upper_case and ?js_string extend different base classes and are unaffected. Added parameterized unit tests to cover all cases.
…caping, the abstract base class that covers exactly ?html, ?xml, and ?xhtml in freemarker 2.3.24. Non-escaping built-ins like ?upper_case and ?js_string extend different base classes and are unaffected. Added parameterized unit tests to cover all cases.
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 65 metrics, 6 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.058 s) : 0, 1058107
Total [baseline] (10.966 s) : 0, 10966215
Agent [candidate] (1.057 s) : 0, 1056580
Total [candidate] (11.077 s) : 0, 11076883
section appsec
Agent [baseline] (1.251 s) : 0, 1251486
Total [baseline] (11.159 s) : 0, 11159244
Agent [candidate] (1.251 s) : 0, 1250903
Total [candidate] (11.166 s) : 0, 11165833
section iast
Agent [baseline] (1.227 s) : 0, 1226673
Total [baseline] (11.328 s) : 0, 11327671
Agent [candidate] (1.234 s) : 0, 1234034
Total [candidate] (11.367 s) : 0, 11366816
section profiling
Agent [baseline] (1.181 s) : 0, 1180921
Total [baseline] (11.104 s) : 0, 11103866
Agent [candidate] (1.188 s) : 0, 1188419
Total [candidate] (11.147 s) : 0, 11147167
gantt
title petclinic - break down per module: candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.19 ms) : 0, 1190
crashtracking [candidate] (1.181 ms) : 0, 1181
BytebuddyAgent [baseline] (625.554 ms) : 0, 625554
BytebuddyAgent [candidate] (625.661 ms) : 0, 625661
AgentMeter [baseline] (28.978 ms) : 0, 28978
AgentMeter [candidate] (29.02 ms) : 0, 29020
GlobalTracer [baseline] (255.905 ms) : 0, 255905
GlobalTracer [candidate] (256.508 ms) : 0, 256508
AppSec [baseline] (31.554 ms) : 0, 31554
AppSec [candidate] (31.585 ms) : 0, 31585
Debugger [baseline] (60.091 ms) : 0, 60091
Debugger [candidate] (60.026 ms) : 0, 60026
Remote Config [baseline] (583.664 µs) : 0, 584
Remote Config [candidate] (609.441 µs) : 0, 609
Telemetry [baseline] (8.057 ms) : 0, 8057
Telemetry [candidate] (8.006 ms) : 0, 8006
Flare Poller [baseline] (10.325 ms) : 0, 10325
Flare Poller [candidate] (8.044 ms) : 0, 8044
section appsec
crashtracking [baseline] (1.219 ms) : 0, 1219
crashtracking [candidate] (1.2 ms) : 0, 1200
BytebuddyAgent [baseline] (660.992 ms) : 0, 660992
BytebuddyAgent [candidate] (661.299 ms) : 0, 661299
AgentMeter [baseline] (12.041 ms) : 0, 12041
AgentMeter [candidate] (12.08 ms) : 0, 12080
GlobalTracer [baseline] (259.277 ms) : 0, 259277
GlobalTracer [candidate] (258.627 ms) : 0, 258627
IAST [baseline] (24.396 ms) : 0, 24396
IAST [candidate] (24.31 ms) : 0, 24310
AppSec [baseline] (178.157 ms) : 0, 178157
AppSec [candidate] (177.865 ms) : 0, 177865
Debugger [baseline] (66.52 ms) : 0, 66520
Debugger [candidate] (65.748 ms) : 0, 65748
Remote Config [baseline] (617.705 µs) : 0, 618
Remote Config [candidate] (631.303 µs) : 0, 631
Telemetry [baseline] (8.331 ms) : 0, 8331
Telemetry [candidate] (9.084 ms) : 0, 9084
Flare Poller [baseline] (3.621 ms) : 0, 3621
Flare Poller [candidate] (3.639 ms) : 0, 3639
section iast
crashtracking [baseline] (1.194 ms) : 0, 1194
crashtracking [candidate] (1.21 ms) : 0, 1210
BytebuddyAgent [baseline] (795.509 ms) : 0, 795509
BytebuddyAgent [candidate] (801.344 ms) : 0, 801344
AgentMeter [baseline] (11.32 ms) : 0, 11320
AgentMeter [candidate] (11.581 ms) : 0, 11581
GlobalTracer [baseline] (247.084 ms) : 0, 247084
GlobalTracer [candidate] (247.865 ms) : 0, 247865
IAST [baseline] (25.25 ms) : 0, 25250
IAST [candidate] (25.349 ms) : 0, 25349
AppSec [baseline] (26.426 ms) : 0, 26426
AppSec [candidate] (26.559 ms) : 0, 26559
Debugger [baseline] (70.737 ms) : 0, 70737
Debugger [candidate] (70.896 ms) : 0, 70896
Remote Config [baseline] (522.823 µs) : 0, 523
Remote Config [candidate] (524.861 µs) : 0, 525
Telemetry [baseline] (9.146 ms) : 0, 9146
Telemetry [candidate] (9.174 ms) : 0, 9174
Flare Poller [baseline] (3.306 ms) : 0, 3306
Flare Poller [candidate] (3.353 ms) : 0, 3353
section profiling
crashtracking [baseline] (1.159 ms) : 0, 1159
crashtracking [candidate] (1.169 ms) : 0, 1169
BytebuddyAgent [baseline] (681.255 ms) : 0, 681255
BytebuddyAgent [candidate] (686.662 ms) : 0, 686662
AgentMeter [baseline] (8.625 ms) : 0, 8625
AgentMeter [candidate] (8.648 ms) : 0, 8648
GlobalTracer [baseline] (215.268 ms) : 0, 215268
GlobalTracer [candidate] (216.416 ms) : 0, 216416
AppSec [baseline] (32.267 ms) : 0, 32267
AppSec [candidate] (32.448 ms) : 0, 32448
Debugger [baseline] (65.198 ms) : 0, 65198
Debugger [candidate] (65.288 ms) : 0, 65288
Remote Config [baseline] (589.533 µs) : 0, 590
Remote Config [candidate] (583.811 µs) : 0, 584
Telemetry [baseline] (8.459 ms) : 0, 8459
Telemetry [candidate] (8.498 ms) : 0, 8498
Flare Poller [baseline] (3.478 ms) : 0, 3478
Flare Poller [candidate] (3.512 ms) : 0, 3512
ProfilingAgent [baseline] (94.049 ms) : 0, 94049
ProfilingAgent [candidate] (94.215 ms) : 0, 94215
Profiling [baseline] (94.625 ms) : 0, 94625
Profiling [candidate] (94.784 ms) : 0, 94784
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.06 s) : 0, 1060442
Total [baseline] (8.837 s) : 0, 8837474
Agent [candidate] (1.058 s) : 0, 1058026
Total [candidate] (8.87 s) : 0, 8870010
section iast
Agent [baseline] (1.222 s) : 0, 1221685
Total [baseline] (9.519 s) : 0, 9518665
Agent [candidate] (1.229 s) : 0, 1229245
Total [candidate] (9.545 s) : 0, 9544745
gantt
title insecure-bank - break down per module: candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.198 ms) : 0, 1198
crashtracking [candidate] (1.198 ms) : 0, 1198
BytebuddyAgent [baseline] (629.664 ms) : 0, 629664
BytebuddyAgent [candidate] (627.13 ms) : 0, 627130
AgentMeter [baseline] (29.117 ms) : 0, 29117
AgentMeter [candidate] (29.065 ms) : 0, 29065
GlobalTracer [baseline] (257.053 ms) : 0, 257053
GlobalTracer [candidate] (256.875 ms) : 0, 256875
AppSec [baseline] (31.774 ms) : 0, 31774
AppSec [candidate] (31.725 ms) : 0, 31725
Debugger [baseline] (59.5 ms) : 0, 59500
Debugger [candidate] (59.434 ms) : 0, 59434
Remote Config [baseline] (594.177 µs) : 0, 594
Remote Config [candidate] (590.5 µs) : 0, 591
Telemetry [baseline] (8.124 ms) : 0, 8124
Telemetry [candidate] (8.133 ms) : 0, 8133
Flare Poller [baseline] (7.354 ms) : 0, 7354
Flare Poller [candidate] (7.96 ms) : 0, 7960
section iast
crashtracking [baseline] (1.195 ms) : 0, 1195
crashtracking [candidate] (1.192 ms) : 0, 1192
BytebuddyAgent [baseline] (792.276 ms) : 0, 792276
BytebuddyAgent [candidate] (797.656 ms) : 0, 797656
AgentMeter [baseline] (11.249 ms) : 0, 11249
AgentMeter [candidate] (11.389 ms) : 0, 11389
GlobalTracer [baseline] (246.214 ms) : 0, 246214
GlobalTracer [candidate] (248.216 ms) : 0, 248216
IAST [baseline] (25.228 ms) : 0, 25228
IAST [candidate] (25.451 ms) : 0, 25451
AppSec [baseline] (26.389 ms) : 0, 26389
AppSec [candidate] (26.604 ms) : 0, 26604
Debugger [baseline] (69.505 ms) : 0, 69505
Debugger [candidate] (69.095 ms) : 0, 69095
Remote Config [baseline] (525.861 µs) : 0, 526
Remote Config [candidate] (520.317 µs) : 0, 520
Telemetry [baseline] (9.654 ms) : 0, 9654
Telemetry [candidate] (9.611 ms) : 0, 9611
Flare Poller [baseline] (3.439 ms) : 0, 3439
Flare Poller [candidate] (3.471 ms) : 0, 3471
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 3 performance regressions! Performance is the same for 16 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section baseline
no_agent (17.446 ms) : 17268, 17625
. : milestone, 17446,
appsec (18.752 ms) : 18562, 18942
. : milestone, 18752,
code_origins (17.657 ms) : 17484, 17830
. : milestone, 17657,
iast (17.354 ms) : 17184, 17525
. : milestone, 17354,
profiling (18.576 ms) : 18391, 18761
. : milestone, 18576,
tracing (17.596 ms) : 17418, 17774
. : milestone, 17596,
section candidate
no_agent (19.447 ms) : 19250, 19645
. : milestone, 19447,
appsec (18.557 ms) : 18371, 18744
. : milestone, 18557,
code_origins (18.76 ms) : 18570, 18950
. : milestone, 18760,
iast (18.043 ms) : 17865, 18222
. : milestone, 18043,
profiling (18.668 ms) : 18477, 18859
. : milestone, 18668,
tracing (17.805 ms) : 17629, 17980
. : milestone, 17805,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section baseline
no_agent (1.182 ms) : 1170, 1194
. : milestone, 1182,
iast (3.318 ms) : 3267, 3369
. : milestone, 3318,
iast_FULL (5.954 ms) : 5895, 6013
. : milestone, 5954,
iast_GLOBAL (3.722 ms) : 3657, 3787
. : milestone, 3722,
profiling (2.204 ms) : 2184, 2225
. : milestone, 2204,
tracing (1.804 ms) : 1788, 1819
. : milestone, 1804,
section candidate
no_agent (1.163 ms) : 1152, 1174
. : milestone, 1163,
iast (3.16 ms) : 3122, 3198
. : milestone, 3160,
iast_FULL (5.846 ms) : 5789, 5904
. : milestone, 5846,
iast_GLOBAL (3.683 ms) : 3622, 3744
. : milestone, 3683,
profiling (2.099 ms) : 2078, 2119
. : milestone, 2099,
tracing (1.795 ms) : 1780, 1810
. : milestone, 1795,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section baseline
no_agent (14.908 s) : 14908000, 14908000
. : milestone, 14908000,
appsec (14.519 s) : 14519000, 14519000
. : milestone, 14519000,
iast (18.389 s) : 18389000, 18389000
. : milestone, 18389000,
iast_GLOBAL (18.055 s) : 18055000, 18055000
. : milestone, 18055000,
profiling (14.735 s) : 14735000, 14735000
. : milestone, 14735000,
tracing (14.965 s) : 14965000, 14965000
. : milestone, 14965000,
section candidate
no_agent (15.434 s) : 15434000, 15434000
. : milestone, 15434000,
appsec (14.965 s) : 14965000, 14965000
. : milestone, 14965000,
iast (18.248 s) : 18248000, 18248000
. : milestone, 18248000,
iast_GLOBAL (17.92 s) : 17920000, 17920000
. : milestone, 17920000,
profiling (15.102 s) : 15102000, 15102000
. : milestone, 15102000,
tracing (14.796 s) : 14796000, 14796000
. : milestone, 14796000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.61.0-SNAPSHOT~d9a12bc8ff, baseline=1.61.0-SNAPSHOT~99d47ca835
dateFormat X
axisFormat %s
section baseline
no_agent (1.471 ms) : 1460, 1483
. : milestone, 1471,
appsec (3.792 ms) : 3572, 4012
. : milestone, 3792,
iast (2.245 ms) : 2177, 2314
. : milestone, 2245,
iast_GLOBAL (2.294 ms) : 2224, 2364
. : milestone, 2294,
profiling (2.1 ms) : 2044, 2156
. : milestone, 2100,
tracing (2.064 ms) : 2010, 2118
. : milestone, 2064,
section candidate
no_agent (1.475 ms) : 1463, 1486
. : milestone, 1475,
appsec (3.774 ms) : 3554, 3994
. : milestone, 3774,
iast (2.248 ms) : 2179, 2317
. : milestone, 2248,
iast_GLOBAL (2.294 ms) : 2224, 2364
. : milestone, 2294,
profiling (2.099 ms) : 2042, 2155
. : milestone, 2099,
tracing (2.053 ms) : 1999, 2106
. : milestone, 2053,
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
escapedExpressionfield check inDollarVariable24Helper:BuiltIn, returnnulland skip the XSS sinkfreemarker-2.3.9instrumentationonXssis never called with anullvalueMotivation
The
DollarVariable24advice only checked theautoEscapeflag (set by<#ftl output_format="HTML" auto_esc=true>), but did not account for explicit built-in escaping like${name?html}. This caused a false positive XSS vulnerability report for templates using?html,?xml, etc.Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.