JS: promote the `js/missing-origin-verification` query by erik-krogh · Pull Request #8724 · github/codeql

erik-krogh · 2022-04-12T12:29:08Z

CVE-2022-24762: TP/TN

We already treat the data from a "message" event as a RemoteFlowSource.

However, sometimes there might be a security issue related to the "message" event that we don't flag.

Maybe data is forwarded using a very dynamic message passing system (surprisingly common for "message" events).
Maybe there is no XSS/Code/Command sink, and the security issue is information disclosure (e.g. CVE-2022-24762).

For these reasons it can be a good idea to flag all "message" events that have no origin check.
I've placed it as a medium precision warning query, so it is not enabled by default.

Here are some example results.

I organized the tests in CWE-020 into subfolders because it was starting to be hard to keep track of them.
I did a drive-by addition to PostMessageEventSanitizer, such that it now recognizes inclusion-tests.

Evaluation will come later when DCA starts working again.

erik-krogh · 2022-04-20T16:29:06Z

The evaluation looks about as expected.

No performance regression, but lots of new results.
The results look reasonable, but it's still not something I want to be enabled in the default suite.

esbena · 2022-04-21T07:03:15Z

+  result =
+    origin(t.continue(), handler)
+        .getAMethodCall([
+            "toString", "toLowerCase", "toUpperCase", "toLocaleLowerCase", "toLocaleUpperCase"


Not relevant for this PR:

"toLocaleLowerCase", "toLocaleUpperCase"

Hmm. I would consider it a bug to use those instead of the canonical to(Lower|Upper)Case calls, but perhaps the programmers are saved by the fact that all browser origins normalizes the same way regardless of locales.

Hmm. I would consider it a bug to use those instead of the canonical to(Lower|Upper)Case calls

It would definitely be weird to use the Locale variants of those methods.
I mainly included them because Copilot suggested them.

esbena · 2022-04-21T07:06:32Z

+}
+
+/** Holds if there exists a check of the .origin or .source of the postmessage `handler`. */
+predicate hasOriginCheck(PostMessageHandler handler) {


I expected to see various startsWith and endsWith variants here. They can be sufficient if done correctly, if they are done incorrectly, we could flag them with a separate query.

Did you see any FPs due to not recognizing such checks?

Did you see any FPs due to not recognizing such checks?

I went through the example results, and there was indeed an FP related to startsWith: https://lgtm.com/query/4833361159564412520/
I'll get StartsWith/EndsWith in there.

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

mchammer01 · 2022-05-05T09:30:08Z

👋🏻 - I'll try to review this later today. Sorry for the delay, the team is understaffed this week 😢

mchammer01

@erik-krogh - this LGTM ✨
A few editorial comments/suggestions for your consideration.
I believe that you do not need to add the MITRE CWE link at the end of the References section in the qhelp file as this is done automatically behind the scenes from the tags specified in the ql file.
Approving this so I am not blocking you.

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>

github-advanced-security

Found 13 vulnerabilities.

   * An equality test on `e.origin` or `e.source` where `e` is a `postMessage` event object,
   * considered as a sanitizer for `e`.
   */
-  private class PostMessageEventSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {



    PostMessageEventSanitizer() {
-      exists(string prop | prop = "origin" or prop = "source" |
-        astNode.getAnOperand().(PropAccess).accesses(event, prop) and


-      exists(string prop | prop = "origin" or prop = "source" |
-        astNode.getAnOperand().(PropAccess).accesses(event, prop) and
-        event.mayReferToParameter(any(PostMessageEventHandler h).getEventParameter())
+      event.mayReferToParameter(any(PostMessageEventHandler h).getEventParameter()) and


+          test.getAnOperand().flow() = read
+        )
+        or
+        exists(InclusionTest test | polarity = test.getPolarity() and this = test |



    override predicate sanitizes(boolean outcome, Expr e) {
-      outcome = astNode.getPolarity() and
+      outcome = polarity and


mchammer01

LGTM, thanks for addressing my comments ✨
Sorry, added a minor suggestion as there were missing words in a sentence that I seem to have missed during my first review 😞

mchammer01 · 2022-05-05T11:57:38Z

+
+<p>
+The example is fixed below, where the origin is checked to be trusted. 
+It is therefore not possible for a malicious user to attack using an untrusted origin.


Suggested change

It is therefore not possible for a malicious user to attack using an untrusted origin.

It is therefore not possible for a malicious user to raise an attack using an untrusted origin.

Hmmm. I'm not fond the the raise an addition.
How about perform an instead?

Perform an attack is absolutely fine 👍🏻

erik-krogh added 5 commits April 12, 2022 10:39

move js/missing-postmessageorigin-verification out of experimental

18532ba

reorganize the tests in CWE-020

ec9c308

update expected output after test reorganization

e2badab

add InclusionTest to PostMessageEventSanitizer

2d6d304

various improvements to the js/missing-origin-verification query

591fcda

erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Apr 12, 2022

github-actions Bot added documentation JS labels Apr 12, 2022

erik-krogh added 2 commits April 12, 2022 14:37

rename files

bca4d14

add change-note

df295e6

erik-krogh marked this pull request as ready for review April 12, 2022 13:40

erik-krogh requested a review from a team as a code owner April 12, 2022 13:40

erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Apr 20, 2022

esbena reviewed Apr 21, 2022

View reviewed changes

erik-krogh and others added 2 commits April 25, 2022 14:07

fix qhelp: the window, not the origin, is sending the message

fe3d71e

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

include startsWith/endsWith checks in js/missing-origin-check

0a26e89

erik-krogh requested a review from esbena April 29, 2022 13:32

esbena previously approved these changes May 3, 2022

View reviewed changes

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

mchammer01 self-requested a review May 5, 2022 09:30

mchammer01 previously approved these changes May 5, 2022

View reviewed changes

apply suggestions from doc review

2d7c7ff

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>

erik-krogh dismissed stale reviews from mchammer01 and esbena via 2d7c7ff May 5, 2022 11:03

erik-krogh requested review from esbena and mchammer01 May 5, 2022 11:04

github-advanced-security AI found potential problems May 5, 2022

View reviewed changes

mchammer01 previously approved these changes May 5, 2022

View reviewed changes

erik-krogh commented May 5, 2022

View reviewed changes

Comment thread javascript/ql/src/Security/CWE-020/MissingOriginCheck.qhelp Outdated

erik-krogh dismissed mchammer01’s stale review via 58db922 May 5, 2022 12:24

add missing word in qhelp

58db922

esbena approved these changes May 9, 2022

View reviewed changes

erik-krogh merged commit 53b26eb into github:main May 9, 2022

	It is therefore not possible for a malicious user to attack using an untrusted origin.
	It is therefore not possible for a malicious user to raise an attack using an untrusted origin.

Conversation

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

Uh oh!

Uh oh!

erik-krogh commented Apr 20, 2022

Uh oh!

Uh oh!

esbena Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

erik-krogh Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

esbena Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

erik-krogh Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

mchammer01 commented May 5, 2022

Uh oh!

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-advanced-security AI left a comment

Choose a reason for hiding this comment

Uh oh!

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

mchammer01 left a comment

Choose a reason for hiding this comment

Uh oh!

mchammer01 May 5, 2022

Choose a reason for hiding this comment

Uh oh!

erik-krogh May 5, 2022

Choose a reason for hiding this comment

Uh oh!

mchammer01 May 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

erik-krogh commented Apr 12, 2022 •

edited

Loading