JS: Add query for unsafe construction of code from library input by erik-krogh · Pull Request #5841 · github/codeql

erik-krogh · 2021-05-05T17:35:48Z

CVE-2017-5954: TP/FP (conditional unsafety)
CVE-2019-5413: TP/TN
CVE-2020-28502: TP/TN
CVE-2020-7712: TP/TN
CVE-2021-23406: TP/TN
CVE-2021-23337: TP
CVE-2021-32831: TP
CVE-2021-23358: TP
CVE-2020-7640: TP/TN

A test run shows a lot of results, many of which just looks to be (very) bad style.
So I've set the severity to warning.

An evaluation looks good.. (The baseline is main + a where none() edition of the query).
The new results look reasonable to me, even though they're basically just bad style.
(I suppose there are performance reasons for implementing it in that style).

esbena

LGTM, but lets merge #5769 first, and then apply the teachings from that PR here.

esbena · 2021-05-07T12:25:32Z

+
+<overview>
+<p>
+Dynamically constructing code with inputs from exported functions 


Same formulation concerns as in #5769

esbena · 2021-05-07T12:28:22Z

+    override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
+      super.hasFlowPath(source, sink) and
+      exists(DataFlow::MidPathNode mid |
+        source.getASuccessor*() = mid and
+        sink = mid.getASuccessor() and
+        mid.getPathSummary().hasReturn() = false
+      )
+    }


Rebase on top of #5769 , and reuse the similar predicate from that PR

esbena · 2021-05-07T12:29:23Z

+  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+    ExternalInputSource() {
+      this = Exports::getALibraryInputParameter() and
+      not this.getName() = "code"


Suggested change

not this.getName() = "code"

// permit parameters that clearly are intended to contain executable code.

not this.getName() = "code"

(go home diff, you are drunk :) )

asgerf · 2021-10-01T10:15:58Z

Sorry for leaving this PR hanging for so long. Looking at the evaluation results, it looks like all of them are FPs:

The Angular results are in internal API. I've put up a PR to fix it here: JS: Restrict what PackageExports considers to be public #6789
The result in Underscore is a template compiler, which by design can evaluate JS code. It's not bad style or a performance trick; these templates can contain JS code which should be evaluated as-is.

I'd say templating engines should be considered a well-known counter example, and an allow-list should be used to exclude methods with names like template or compile or perhaps even render.

erik-krogh · 2021-10-28T13:03:02Z

The Angular results are in internal API. I've put up a PR to fix it here: JS: Restrict what PackageExports considers to be public #6789

I've rebased on top of that PR, and the angular results disappeared in a new evaluation.

The result in Underscore is a template compiler, which by design can evaluate JS code. It's not bad style or a performance trick; these templates can contain JS code which should be evaluated as-is.

Yes, the text argument in the underscore template compiler is unsafe by design.
But the kind of code-injection that this query flags can happen in a template compiler (see CVE-2021-23337 and CVE-2021-23358, both of which we get a TP for).
So I think the noise in Underscore is something we can live with.

esbena · 2022-02-07T07:35:55Z

I'd say templating engines should be considered a well-known counter example, and an allow-list should be used to exclude methods with names like template or compile or perhaps even render.

I agree with this. Could we use a whitelist to avoid intentional APIs like those and otherwise merge this twice-stalled PR as is? We can then separately address the undesirable FNs the whitelist causes.

erik-krogh · 2022-02-07T12:33:48Z

I agree with this. Could we use a whitelist to avoid intentional APIs like those and otherwise merge this twice-stalled PR as is? We can then separately address the undesirable FNs the whitelist causes.

I took a look at the method-names in the CVEs currently flagged by this query:

CVE	method-name
CVE-2017-5954	deserialize
CVE-2019-5413	compile
CVE-2020-28502	send
CVE-2020-7712	parseLookup
CVE-2021-23406	compile
CVE-2021-23337	template
CVE-2021-32831	set
CVE-2021-23358	template
CVE-2020-7640	create

Adding a allow-list based on names that are template-engine-like would remove some of those TPs.
I would prefer we lower the precision of the query without an allow-list.

(I rebased the PR on main, and updated to the new change-note format).

…ary functions

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

ethanpalm

This looks good from a docs point of view. I am just making one small change to move the explanation of why the first example is a bad coding practice to the paragraph before the example rather than after the example ⚡

erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label May 5, 2021

github-actions Bot added documentation JS labels May 5, 2021

intrigus-lgtm reviewed May 5, 2021

View reviewed changes

Comment thread javascript/ql/src/Security/CWE-094/UnsafeCodeConstruction.qhelp Outdated

erik-krogh force-pushed the libCode branch 2 times, most recently from ed24fe9 to 9f2f022 Compare May 6, 2021 12:29

erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label May 6, 2021

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

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

esbena reviewed May 7, 2021

View reviewed changes

erik-krogh force-pushed the libCode branch from 0f2e6c7 to 01a7d15 Compare May 13, 2021 16:08

erik-krogh force-pushed the libCode branch from 01a7d15 to 147b4a9 Compare September 6, 2021 19:29

erik-krogh added 14 commits February 7, 2022 13:34

add code-injection sink for calls to node

68a5c1f

add JSON.stringify as a code-injection sanitizer

955ad8c

add js/unsafe-code-construction query

198a464

add test for unsafe-code-construction query

d790f3c

add qhelp for unsafe-code-construction

d77c28f

add missing qldoc

59cc099

ignore sources named "code"

53315e6

add comment about parameters named "code"

681179d

reuse utility predicate

a9f7756

update qhelp to focus on properly documenting potentially unsafe libr…

eb133f5

…ary functions

move .qll files from src to lib

91b03f5

add values written to the global scope as exports

d1d4ebb

update expected output now that JSON.stringify() is seen as a sanitizer

896d2ba

add change note

06f9924

lower the precision of js/unsafe-code-construction

6f28cb9

erik-krogh force-pushed the libCode branch from 1479eca to 6f28cb9 Compare February 7, 2022 12:35

esbena reviewed Feb 7, 2022

View reviewed changes

Comment thread javascript/ql/src/Security/CWE-094/UnsafeCodeConstruction.ql Outdated

esbena added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Feb 7, 2022

fix typo

aa95dd4

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

ethanpalm previously approved these changes Feb 9, 2022

View reviewed changes

Comment thread javascript/ql/src/Security/CWE-094/UnsafeCodeConstruction.qhelp Outdated

Comment thread javascript/ql/src/Security/CWE-094/UnsafeCodeConstruction.qhelp Outdated

Comment thread javascript/ql/src/Security/CWE-094/UnsafeCodeConstruction.qhelp Outdated

Move explanation of example above sample code

2f7f9d9

ethanpalm dismissed their stale review via 2f7f9d9 February 9, 2022 18:45

ethanpalm approved these changes Feb 9, 2022

View reviewed changes

esbena approved these changes Feb 10, 2022

View reviewed changes

codeql-ci merged commit 1a91a79 into github:main Feb 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

JS: Add query for unsafe construction of code from library input#5841
codeql-ci merged 17 commits into
github:mainfrom
erik-krogh:libCode

erik-krogh commented May 5, 2021 •

edited

Loading

Uh oh!

Uh oh!

esbena left a comment

Uh oh!

esbena May 7, 2021

Uh oh!

esbena May 7, 2021

Uh oh!

esbena May 7, 2021

Uh oh!

esbena May 7, 2021

Uh oh!

asgerf commented Oct 1, 2021

Uh oh!

erik-krogh commented Oct 28, 2021

Uh oh!

esbena commented Feb 7, 2022

Uh oh!

erik-krogh commented Feb 7, 2022 •

edited

Loading

Uh oh!

Uh oh!

ethanpalm left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

	not this.getName() = "code"
	// permit parameters that clearly are intended to contain executable code.
	not this.getName() = "code"

Conversation

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

Uh oh!

Uh oh!

Uh oh!

esbena left a comment

Choose a reason for hiding this comment

Uh oh!

esbena May 7, 2021

Choose a reason for hiding this comment

Uh oh!

esbena May 7, 2021

Choose a reason for hiding this comment

Uh oh!

esbena May 7, 2021

Choose a reason for hiding this comment

Uh oh!

esbena May 7, 2021

Choose a reason for hiding this comment

Uh oh!

asgerf commented Oct 1, 2021

Uh oh!

erik-krogh commented Oct 28, 2021

Uh oh!

esbena commented Feb 7, 2022

Uh oh!

erik-krogh commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ethanpalm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

erik-krogh commented May 5, 2021 •

edited

Loading

erik-krogh commented Feb 7, 2022 •

edited

Loading