X Tutup
The Wayback Machine - https://web.archive.org/web/20240212155818/https://github.com/github/codeql/pull/15571
Skip to content
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

Jump to bottom

JS: False negative - unsafe postMessage handler not detected #15571

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

JS: False negative - unsafe postMessage handler not detected #15571

wants to merge 3 commits into from
+9 −29

Conversation

danilishinyar
Copy link

Description of the issue

I tested CodeQL on JS, which handles postMessage validation. The rule for detecting the absence of an origin check in the postMessage handler function does not cover all possible ways to bypass that check.
Firstly, the rule doesn't identify cases where only event.source is checked. Such verification can be easily bypassed.

https://book.hacktricks.xyz/pentesting-web/postmessage-vulnerabilities#bypassing-e.source

/** Gets a reference to the origin or the source of a postmessage event. */
DataFlow::SourceNode sourceOrOrigin(PostMessageHandler handler) {
  result = source(DataFlow::TypeTracker::end(), handler) or
  result = origin(DataFlow::TypeTracker::end(), handler)
}

Secondly, checks like "safeOrigin".startsWith(event.origin) and "safeOrigin".endsWith(event.origin) can also be bypassed.

Lastly, there are methods to bypass the equality operation as well, so it's only safe to use strict equality operators (===and !==).

For more details on bypassing origin checks, you can refer to: https://book.hacktricks.xyz/pentesting-web/postmessage-vulnerabilities#origin-check-bypasses, https://book.hacktricks.xyz/pentesting-web/postmessage-vulnerabilities#e.origin-window.origin-bypass

predicate hasOriginCheck(PostMessageHandler handler) {
  // event.origin === "constant"
  exists(EqualityTest test | sourceOrOrigin(handler).flowsToExpr(test.getAnOperand()))
  or
  // set.includes(event.source)
  exists(InclusionTest test | sourceOrOrigin(handler).flowsTo(test.getContainedNode()))
  or
  // "safeOrigin".startsWith(event.origin)
  exists(StringOps::StartsWith starts |
    origin(DataFlow::TypeTracker::end(), handler).flowsTo(starts.getSubstring())
  )
  or
  // "safeOrigin".endsWith(event.origin)
  exists(StringOps::EndsWith ends |
    origin(DataFlow::TypeTracker::end(), handler).flowsTo(ends.getSubstring())
  )
}

The only secure way to handle postMessage is to always check event.origin.

CodeQL Rule
Rule: MissingOriginCheck.ql

Thank you for your time!

@danilishinyar danilishinyar requested a review from a team as a code owner February 9, 2024 14:22
@github-actions github-actions bot added the JS label Feb 9, 2024
@sidshank sidshank changed the title False negative: unsafe postMessage handler not detected JS: False negative - unsafe postMessage handler not detected Feb 11, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 12, 2024
View reviewed changes
javascript/ql/src/Security/CWE-020/MissingOriginCheck.ql
}

/** Holds if there exists a check of the .origin or .source of the postmessage `handler`. */
/** The only way to make secure postMessage `handler` is to check event.origin with strict equality operator or use whitelist */

Check warning

Code scanning / CodeQL

Predicate QLDoc style. Warning

The QLDoc for a predicate without a result should start with 'Holds'.
@erik-krogh
Copy link
Contributor

Firstly, the rule doesn't identify cases where only event.source is checked. Such verification can be easily bypassed.

https://book.hacktricks.xyz/pentesting-web/postmessage-vulnerabilities#bypassing-e.source

I don't agree on that.
Yes, an attacker can easily set e.source to be null, but an attacker cannot set e.source to any other value.
So any validation that e.source is a non-null value cannot be easily bypassed.

Secondly, checks like "safeOrigin".startsWith(event.origin) and "safeOrigin".endsWith(event.origin) can also be bypassed.

Technically yes, but mostly no.
Assume that const safe = "https://example.org".
There is no other domain I can buy that satisfies safe.startsWith(event.origin).
E.g. the string "https://examp" would be a "bypass", but I can't use that for an attack.

Yes, bypasses can exist for other values of safe, but that doesn't mean that a .startsWith(...) check is always unsafe.

Lastly, there are methods to bypass the equality operation as well, so it's only safe to use strict equality operators (===and !==).

The only bypass mentioned in your sources is if both sides are null. So not relevant if you're checking against any non-null value.

The only secure way to handle postMessage is to always check event.origin.

Again, no.
E.g. checking that e.source is a known non-null value is safe.

In general our analysis doesn't try to flag every program that could possibly be unsafe, because it's extremely hard to guarantee that no exploit can exist.
So trying to flag everything that could be unsafe would result in an overwhelming amount of false positives.
(Here is a nice paper by Google that describes what happens if you have too many false positives)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@erik-krogh erik-krogh

Labels
documentation external-contribution JS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danilishinyar @erik-krogh @sidshank
X Tutup