JS: Add unsafe-html-construction query by erik-krogh · Pull Request #5769 · github/codeql

erik-krogh · 2021-04-26T08:42:37Z

Gets a TP for CVE-2020-7750

The idea (and structure) of the query is the same as js/shell-command-constructed-from-input:

Taint-track library input to some input transformation.
This input transformation can be:
- Construct some HTML string (found some TPs from that).
- Parse as XML (CVE-2020-7750)
- Parse as markdown in an unsafe way. (I haven't found any real results for this, but I want to keep it anyway).
Type-track the transformed input to an XSS sink.

The TP rate is good.
I had some FPs from a run-on-all, but almost all where removed with the remove flowpaths that has a return without a matching call commit and the not this = JQuery::dollarSource() check.

I'll look into promoting classFieldStep to a SharedTaintStep after this PR has been merged.

Performance evaluation:
part 1 looks good
part 2 (and a redo) looks good. It seems performance on the workers got more unstable 🤷
The new results look like TPs (or at least bad style).

esbena

Generally LGTM. Here's half a review. I would very much like to give it a second read-through, but in the interest of progress, I am submitting some comments now.

esbena · 2021-05-06T11:52:54Z

+  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+    ExternalInputSource() {
+      this = Exports::getALibraryInputParameter() and
+      not this = JQuery::dollarSource()


Please add a comment about why we need to exclude dollarSource

esbena · 2021-05-06T12:58:42Z

+
+	<p>
+		Alternatively, use a HTML sanitizer to escape/remove unsafe content.
+	</p>


Unlike the command-injection variant of this query, I think it is (more) reasonable to do some ad hoc HTML concatenation in a library.
Experience from the unsafe jquery plugins tells that some library maintainers do not feel responsible for these kinds of XSS vectors, and some of them just update their README to tell clients that they are responsible for sanitizing HTML before using the library.

Can we angle the qhelp such that additional documentation is a known fix type (a fix that won't remove the "FP").

Perhaps some qhelp fragments should be shared between this query and js/unsafe-jquery-plugin.

codeql/javascript/ql/src/Security/CWE-079/UnsafeJQueryPlugin.qhelp

Lines 6 to 35 in 059a5f3

<overview>

<p>

Library plugins, such as those for the jQuery library, are often

configurable through options provided by the clients of the

plugin.

Clients, however, do not know the implementation details

of the plugin, so it is important to document the capabilities of each

option. The documentation for the plugin options that the client is

responsible for sanitizing is of particular importance.

Otherwise, the plugin may write user input (for example, a URL query

parameter) to a web page without properly sanitizing it first,

which allows for a cross-site scripting vulnerability in the client

application through dynamic HTML construction.

</p>

</overview>

<recommendation>

<p>

Document all options that can lead to cross-site scripting

attacks, and guard against unsafe inputs where dynamic HTML

construction is not intended.

</p>

</recommendation>

I like the idea, but I don't think we can share fragments as the terms used are different
(plugin vs library and options vs input).

I'm rewriting the qhelp to focus more on unsafe endpoints being properly documented.

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

esbena

Thanks.
Here are some additional comments.

esbena · 2021-05-07T10:09:27Z

+		Dynamically constructing HTML with inputs from library functions that are 
+		available to external clients may inadvertently leave a client open to XSS attacks.
+
+		Clients using the exported function may use inputs containing unsafe HTML,
+		and if these inputs end up in the DOM, the client may be vulnerable to
+		cross-site scripting attacks.


XSS attacks

cross-site scripting attacks

Pick one.

esbena · 2021-05-07T10:09:56Z

+		Dynamically constructing HTML with inputs from library functions that are 
+		available to external clients may inadvertently leave a client open to XSS attacks.
+
+		Clients using the exported function may use inputs containing unsafe HTML,


Suggested change

Clients using the exported function may use inputs containing unsafe HTML,

Clients using the exported function may use inputs containing unsafe HTML fragments,

esbena · 2021-05-07T10:11:30Z

+<recommendation>
+
+	<p>
+		If possible, use safe APIs when inserting HTML into the DOM.  


Suggested change

If possible, use safe APIs when inserting HTML into the DOM.

If possible, use safe APIs when modifying the DOM.

When the suggestion below is to use innerText, it is confusion to see "when inserting HTML".

esbena · 2021-05-07T10:13:13Z

-}
+module Markdown {
+  /**
+   * A taint-step that parses a markdown document, but preserves taint.import


Suggested change

* A taint-step that parses a markdown document, but preserves taint.import

* A taint-step that parses a markdown document, but preserves taint.

?

esbena · 2021-05-07T10:17:05Z

+        call = markdownIt().getMember(["use", "set", "configure", "enable", "disable"]).getACall() and
+        result = call.getReturn() and
+        not call.getParameter(0).getAValueReachingRhs() =
+          DataFlow::moduleImport("markdown-it-sanitizer")


Should this negation be in the preservesHtml predicate?

I think that would require adding a boolean isSanitized parameter to the markdownIt() predicate.

The MarkdownStep::step predicate is documented as being a taint-step.
And if markdown-it-sanitizer is used, then there is no taint-step.
So I think it's fine.

esbena · 2021-05-07T10:25:06Z

-        sink = mid.getASuccessor() and
-        mid.getPathSummary().hasReturn() = false
-      )
+      UnsafeHtmlConstruction::requireMatchedReturn(source, sink)


Can we move this step to a shared library.
I the step already existed in UnsafeShellCommandConstruction.qll , and was simply referenced from UnsafeHtmlConstruction.qll I would be more inclined to shrug this off, but now we are introducing a dependency on a new query instead.

github-actions Bot added the JS label Apr 26, 2021

erik-krogh force-pushed the libXss branch from 6947e07 to e8a84c1 Compare April 26, 2021 09:48

github-actions Bot added the documentation label Apr 26, 2021

erik-krogh force-pushed the libXss branch 2 times, most recently from ec82340 to 27b718f Compare April 26, 2021 18:10

erik-krogh force-pushed the libXss branch from 873b191 to a230273 Compare May 4, 2021 11:17

split the markdown steps into a separate class

a400a1e

erik-krogh force-pushed the libXss branch 2 times, most recently from b4fbde1 to f662ddd Compare May 6, 2021 08:59

erik-krogh added 8 commits May 6, 2021 11:04

add js/html-constructed-from-input query

e86a3b5

add test for js/html-constructed-from-input

6e754c7

remove flowpaths that has a returns without a matching call

23908f9

share code between unsafe-shell and unsafe-html queries

ee0140e

add qhelp

7ef641e

add change note

5c37e6a

add jQuery options objects as sources

8ba5bdd

add sanitizers from DOM and jQuery queries

3815797

erik-krogh force-pushed the libXss branch from f662ddd to 3815797 Compare May 6, 2021 09:05

erik-krogh marked this pull request as ready for review May 6, 2021 09:09

erik-krogh requested a review from a team as a code owner May 6, 2021 09:09

esbena reviewed May 6, 2021

View reviewed changes

erik-krogh and others added 3 commits May 6, 2021 21:55

Apply suggestions from code review

2d1ba59

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

Apply suggestions from code review

be69c3a

Co-authored-by: Esben Sparre Andreasen <esbena@github.com>

corrections after code review

b53759c

esbena reviewed May 7, 2021

View reviewed changes

esbena mentioned this pull request May 7, 2021

JS: Add query for unsafe construction of code from library input #5841

Merged

erik-krogh added 4 commits May 10, 2021 10:05

add comment about filtering away jQuery from the source

3fe5dd0

rewrite the qhelp to focus more on documenting unsafe functions

646bf99

fix typo

b4e35f5

move hasPathWithoutUnmatchedReturn to Configuration.qll

d913668

esbena approved these changes May 10, 2021

View reviewed changes

codeql-ci merged commit a3d17a1 into github:main May 10, 2021

	<overview>
	<p>

	Library plugins, such as those for the jQuery library, are often
	configurable through options provided by the clients of the
	plugin.


	Clients, however, do not know the implementation details
	of the plugin, so it is important to document the capabilities of each
	option. The documentation for the plugin options that the client is
	responsible for sanitizing is of particular importance.

	Otherwise, the plugin may write user input (for example, a URL query
	parameter) to a web page without properly sanitizing it first,
	which allows for a cross-site scripting vulnerability in the client
	application through dynamic HTML construction.

	</p>
	</overview>

	<recommendation>
	<p>

	Document all options that can lead to cross-site scripting
	attacks, and guard against unsafe inputs where dynamic HTML
	construction is not intended.

	</p>
	</recommendation>

	Clients using the exported function may use inputs containing unsafe HTML,
	Clients using the exported function may use inputs containing unsafe HTML fragments,

	If possible, use safe APIs when inserting HTML into the DOM.
	If possible, use safe APIs when modifying the DOM.

	* A taint-step that parses a markdown document, but preserves taint.import
	* A taint-step that parses a markdown document, but preserves taint.

Conversation

erik-krogh commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erik-krogh commented Apr 26, 2021 •

edited

Loading