Skip to content

JS: Improve XSS detection for serialize-javascript with tainted objects #19771

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

Merged
merged 5 commits into from
Jun 17, 2025

Conversation

Napalys
Copy link
Contributor

@Napalys Napalys commented Jun 16, 2025

Enhances the XSS analysis to track taint through serialize-javascript calls when tainted data is passed via object properties.

@Napalys Napalys force-pushed the js/sanitizer_serialize branch from c0fb7b6 to 71a5a6d Compare June 16, 2025 07:44
@Napalys Napalys marked this pull request as ready for review June 16, 2025 07:53
@Copilot Copilot AI review requested due to automatic review settings June 16, 2025 07:53
@Napalys Napalys requested a review from a team as a code owner June 16, 2025 07:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Enhance XSS analysis to track taint through serialize-javascript calls when tainted object properties are passed, and update tests and expected outputs to flag unsafe serializations.

  • Add new test cases in tst2.js covering object arguments and {unsafe: true}
  • Update two expected files to include alerts for unsafe serialization of object-based inputs
  • Implement a new SerializeJavascriptFlowStep in Xss.qll and document the change

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/tst2.js Added routes testing serializeJavaScript with object props and {unsafe: true}
javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected Expanded expected alerts to include taint through object serialization with unsafe flag
javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected Expanded expected alerts and dataflow edges/nodes for unsafe object serialization
javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll Introduced SerializeJavascriptFlowStep to propagate taint through object arguments
javascript/ql/lib/change-notes/2025-06-16-serialize-js.md Documented the improved XSS detection for object properties in change notes
Comments suppressed due to low confidence (1)

javascript/ql/lib/semmle/javascript/security/dataflow/Xss.qll:64

  • [nitpick] Indent the SerializeJavascriptFlowStep class definition to align with the surrounding code (4 spaces) for consistent formatting.
private class SerializeJavascriptFlowStep extends DataFlow::AdditionalFlowStep {

call = DataFlow::moduleImport("serialize-javascript").getACall() and
succ = call and
propWrite.getRhs() = pred and
propWrite.getBase().getALocalSource().flowsTo(call.getArgument(0))
Copy link
Preview

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flow step currently propagates taint through object arguments regardless of the unsafe option. Restrict this step to only when the unsafe flag is set to true to avoid false positives on safe serializations.

Suggested change
propWrite.getBase().getALocalSource().flowsTo(call.getArgument(0))
propWrite.getBase().getALocalSource().flowsTo(call.getArgument(0)) and
call.getOptionArgument(1, "unsafe").mayHaveBooleanValue(true)

Copilot uses AI. Check for mistakes.

exists(DataFlow::CallNode call, DataFlow::PropWrite propWrite |
call = DataFlow::moduleImport("serialize-javascript").getACall() and
succ = call and
propWrite.getRhs() = pred and
Copy link
Preview

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flow step only handles PropWrite assignments but does not cover object literal initializers (e.g., {someProperty: p}). Add handling for object literal property initializers so inline object literals also propagate taint.

Copilot uses AI. Check for mistakes.

@Napalys Napalys force-pushed the js/sanitizer_serialize branch from 71a5a6d to fffbc0c Compare June 16, 2025 08:38
---
category: minorAnalysis
---
* Improved XSS detection for `serialize-javascript` calls with tainted object properties.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your changes I don't feel this is entirely accurate.
Can you change to a more general statement that we track data through calls to serialize-javascript?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in fc0c8a8.

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume DCA evaluations looked good.

@Napalys Napalys merged commit ac533ea into github:main Jun 17, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants