JS: add method name injection query by asger-semmle · Pull Request #503 · github/codeql

asger-semmle · 2018-11-20T16:08:39Z

This adds a query for a special case of what "Remote property injection" would find, but with higher precision value and classified as a code-injection alert. In aims to only flag cases that can lead to code injection.

It's mostly a special case of the aforementioned query, although the coverage is slightly higher in some cases:

The property name in a property projection call is recognized as a property-name sink.
The subsequent invocation is found using a flow label, instead of relying on local flow.

Performance evaluation is under way.

xiemaisi

Broadly lgtm, a few suggestions.

xiemaisi · 2018-11-20T17:40:30Z

+
+<recommendation>
+<p>
+Avoid invoking user-controlled methods on the global object or on any function object.


This is a little vague. Could we suggest something more concrete, like checking the name of the function to be invoked against a whitelist?

xiemaisi · 2018-11-20T17:41:28Z

+ * @name Method name injection
+ * @description Invoking user-controlled methods on a arbitrary objects can lead to remote code execution.
+ * @kind path-problem
+ * @problem.severity warning


Only warning? It seems dangerous enough to warrant severity error.

xiemaisi · 2018-11-20T17:41:52Z

+    /**
+     * Gets the flow label relevant for this source.
+     */
+    DataFlow::FlowLabel getFlowLabel() {


Is this used anywhere?

Ah, good catch. It was supposed to be used in isSource.

xiemaisi · 2018-11-20T17:42:14Z

@@ -0,0 +1,148 @@
+/**
+ * Provides a taint tracking configuration for reasoning about method invocations


"taint-tracking"

xiemaisi · 2018-11-20T17:42:32Z

+     * Holds if a property of the given object is an unsafe function.
+     */
+    predicate isUnsafeBaseObject(DataFlow::SourceNode node) {
+      // eval an friends can be accessed from the global object.


xiemaisi · 2018-11-20T17:44:07Z

+  }
+
+  /** 
+   * A binary expression that sanitzes a value for method name injection. That 


s/sanitzes/sanitizes/

xiemaisi · 2018-11-20T17:46:31Z

+</p>
+
+<sample src="examples/MethodNameInjection.js" />
+</example>


We generally try to give a suggestion for how to fix the problematic example. The current example is hard to fix in general, but could we perhaps modify it slightly so that it could be fixed by whitelisting the name of the invoked method?

xiemaisi · 2018-11-20T17:49:18Z

+  private predicate isCoveredByMethodNameInjection(DataFlow::SourceNode node) {
+    node = DataFlow::globalObjectRef()
+    or
+    node.analyze().getAValue() instanceof AbstractCallable


Do we also want to whitelist nodes that have an invocation here? (And could we share this predicate with the other query?)

Added a common library to share this predicate, and also ConcatSanitizer which was originally stolen from there.

asger-semmle · 2018-11-20T18:45:00Z

PTAL

xiemaisi · 2018-11-21T08:39:12Z

 | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094  | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
 | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. |
 | Host header poisoning in email generation |  security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default.  |
+| Method name injection (`js/method-name-injection` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. |


I like the name, but I wonder whether we could somehow emphasise the unsafe method bit? As you know we've had customer interest in a slightly different kind of situation where method name injection results not in code injection but in a crash. That is different enough in terms of implementation and severity to warrant a separate query, and we'd need to different names and IDs for both.

Unsafe dynamic method access?

Yes, that's nice.

xiemaisi

Looking good; a few more thoughts.

xiemaisi · 2018-11-21T08:40:49Z

+<sample src="examples/MethodNameInjection.js" />
+
+<p>
+Instead of storing the API methods in the global scope, put them in an API object. It is also good


We could also suggest using a Map instead of a plain object.

Added Map to the qhelp file, although I didn't use it in the example as I found the difference in style to be distracting from what the example is showing.

For example, if we replace function play() with:

api.put("play", (data) => { // ... });

any manual calls to the play function would have to be changed to api.get("play")(...) which isn't really an attractive option.

xiemaisi · 2018-11-21T08:45:38Z

+   */
+  predicate hasUnsafeMethods(DataFlow::SourceNode node) {
+    // eval and friends can be accessed from the global object.
+    node = DataFlow::globalObjectRef()


How about document (for accessing document.write)?

Co-Authored-By: asger-semmle <42069257+asger-semmle@users.noreply.github.com>

…/ql into unsafe-global-object-access

xiemaisi · 2018-11-21T11:22:29Z

Ping @mc-semmle for doc review.

mchammer01 · 2018-11-21T13:52:30Z

 | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094  | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. |
 | File data in outbound network request | security, external/cwe/cwe-200 | Highlights locations where file data is sent in a network request. Results are not shown on LGTM by default. |
 | Host header poisoning in email generation |  security, external/cwe/cwe-640 | Highlights code that generates emails with links that can be hijacked by HTTP host header poisoning, indicating a violation of [CWE-640](https://cwe.mitre.org/data/definitions/640.html). Results shown on LGTM by default.  |
+| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. |


Are the results shown or not shown on LGTM by default?

mchammer01 · 2018-11-21T14:02:12Z

+
+<overview>
+<p>
+Invoking a user-controlled method on certain objects can lead to invocation of unsafe functions,


Invoking/invocation - would it be possible for you to use a synonym for one of the instances to avoid repetition in the same sentence?

mchammer01 · 2018-11-21T14:04:42Z

+<p>
+Invoking a user-controlled method on certain objects can lead to invocation of unsafe functions,
+such as <code>eval</code> or the <code>Function</code> constructor. In particular, the global object
+contains the <code>eval</code> function, and any function object contains the <code>Function</code> constructor


Sorry I don't understand this sentence (In particular, the global object contains the <code>eval</code> function, and any function object contains the <code>Function</code> constructor in its <code>constructor</code> property.) and how it links to what you've said previously. I am not a developer so feel free to ignore if you think this is clear enough to the intended audience.

It's a technical explanation of what was meant by "certain objects" in the previous sentence. I think it would make sense to most JavaScript developers.

mchammer01

@asger-semmle - this LGTM. I've made a couple of comments on the qhelp file, and I think you need to add a sentence to the change note to let users know whether the results of this new query are shown on LGTM by default.
Hope this helps.

asger-semmle · 2018-11-21T14:20:29Z

Thanks for the review @mc-semmle.

mchammer01

Thanks for the updates to the documentation @asger-semmle
This is good to go from my perspective

xiemaisi · 2018-11-21T16:31:42Z

Query LGTM; how is the evaluation coming along?

asger-semmle · 2018-11-21T16:34:55Z

Query LGTM; how is the evaluation coming along?

I had to restart due to that dist-compare PR I put up today, so it's not very far.

xiemaisi · 2018-11-22T09:17:52Z

+        hasUnsafeMethods(read.getBase().getALocalSource()) and
+        src = read.getPropertyNameExpr().flow() and
+        dst = read and
+        srclabel = taint() and


On second (or third) thought: shouldn't we also allow srclabel = data() here and below?

You're right - for some reason I thought data implied that taint would also be present.

It's an interesting question. I experimented with adding this sort of additional lattice structure on taint kinds, but it turned out to be tricky to get right in general.

xiemaisi · 2018-11-23T12:42:18Z

@asger-semmle, has the evaluation finished yet?

asger-semmle · 2018-11-23T12:55:05Z

No, I keep botching it. I have the query results here but no performance yet.

xiemaisi · 2018-11-26T08:16:59Z

I have retargetted this at rc/1.19 since it was reviewed and approved before feature freeze and is only blocked on testing.

xiemaisi · 2018-11-26T15:56:16Z

As discussed earlier, the evaluation results look reasonable, so in the interest of getting this PR into the current LGTM.com upgrade I'm merging it.

asger-semmle added 4 commits November 20, 2018 14:24

JS: move CodeInjection tests into subfolder

bc3b983

JS: add query MethodNameInjection

2239f86

JS: add to security suite

a2e5003

JS: suppress similar alerts from RemotePropertyInjection

8aff666

asger-semmle requested a review from a team as a code owner November 20, 2018 16:08

xiemaisi suggested changes Nov 20, 2018

View reviewed changes

xiemaisi reviewed Nov 20, 2018

View reviewed changes

asger-semmle added 7 commits November 20, 2018 18:11

JS: address some comments

1c06f45

JS: use StringConcatenation library in ConcatSanitizer

49cd287

JS: share ConcatSanitizer in common module

b16072a

JS: share detection of objects with unsafe methods

3902f75

JS: document the shared module

260ae36

JS: expand example

4138f81

JS: add qhelp example to test suite

7d80847

xiemaisi reviewed Nov 21, 2018

View reviewed changes

Comment thread javascript/ql/src/semmle/javascript/security/dataflow/MethodNameInjection.qll Outdated

xiemaisi reviewed Nov 21, 2018

View reviewed changes

xiemaisi suggested changes Nov 21, 2018

View reviewed changes

Max Schaefer and others added 3 commits November 21, 2018 10:55

Update javascript/ql/src/Security/CWE-094/MethodNameInjection.ql

fa761c0

Co-Authored-By: asger-semmle <42069257+asger-semmle@users.noreply.github.com>

JS: more comments

84d6426

Merge branch 'unsafe-global-object-access' of github.com:asger-semmle…

cb832b1

…/ql into unsafe-global-object-access

asger-semmle added 2 commits November 21, 2018 12:34

JS: rename query to Unsafe Dynamic Method Access

4ae2493

JS: fix string value of taint configuration

8c7e195

mchammer01 reviewed Nov 21, 2018

View reviewed changes

mchammer01 requested changes Nov 21, 2018

View reviewed changes

JS: address doc review

27c9326

mchammer01 previously approved these changes Nov 21, 2018

View reviewed changes

xiemaisi added the JS label Nov 22, 2018

xiemaisi reviewed Nov 22, 2018

View reviewed changes

JS: handle both data() and taint() source labels

61ef655

asger-semmle dismissed mchammer01’s stale review via 61ef655 November 22, 2018 09:59

xiemaisi approved these changes Nov 23, 2018

View reviewed changes

xiemaisi changed the base branch from master to rc/1.19 November 26, 2018 08:16

xiemaisi added this to the 1.19 milestone Nov 26, 2018

xiemaisi merged commit 8e54c7a into github:rc/1.19 Nov 26, 2018

xiemaisi mentioned this pull request Nov 28, 2018

JavaScript: Add new query UnvalidatedDynamicMethodCall. #555

Merged

asger-semmle mentioned this pull request Dec 11, 2018

Merge rc/1.19 into master #662

Closed

		@@ -0,0 +1,148 @@
		/**
		* Provides a taint tracking configuration for reasoning about method invocations

Uh oh!

Conversation

asger-semmle commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiemaisi 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!

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!

asger-semmle commented Nov 20, 2018

Uh oh!

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!

xiemaisi 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiemaisi commented Nov 21, 2018

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!

mchammer01 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asger-semmle commented Nov 21, 2018

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

xiemaisi commented Nov 21, 2018

Uh oh!

asger-semmle commented Nov 21, 2018

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!

asger-semmle commented Nov 20, 2018 •

edited

Loading

mchammer01 left a comment •

edited

Loading