[javascript] CodeQL query to detect missing origin validation in cross-origin communication via postMessage

[dellalibera]

When using cross-domain communication via the window.postMessage() API

If you do expect to receive messages from other sites, always verify the sender's identity using the origin and possibly source properties. Any window (including, for example, http://evil.example.com) can send a message to any other window, and you have no guarantees that an unknown sender will not send malicious messages.

Window.postMessage()

This query addresses the scenario where the MessageEvent.origin property is not checked
(either read or call) in the postMessage event handler.

[esbena]

Thank you, we will get back to you next week.

In the meantime, I'll direct your attention to our default sanitizer for origin validation, which seems very related:

codeql/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

Line 955 in 398678a

private class PostMessageEventSanitizer extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {

Could that inspire some changes in your end?

[JLLeitschuh]

Any way to ensure that the dev hasn't done something goofy like:

if (event.origin.contains('www.example.com')) {
}

Or, one that I've seen before when I reported CVE-2019-10779:

  var origin = event.origin;
  var host = window.location.host;

  // Stop this script being called from other domains.
  if (origin.indexOf(host) === -1)
    return;

This was the one I used when I found a vulnerability in a GCHQ project:
GHSA-6p78-9hh5-384m
https://gist.github.com/JLLeitschuh/a6d6eb3886170dafa7c0cbe1d63edefa

[JLLeitschuh]

Legit vulnerability query. I've found vulns from this mistake before. I appreciate this is being added.

[dellalibera]

Hi @esbena, @JLLeitschuh

thanks for your feedbacks, I really appreciated.

@esbena I looked at your suggestion and I used it to check if a read access to the property origin or source of the MessageEvent is in an Equality test.

@JLLeitschuh I added other checks in order to detect insufficient and insecure validation of the event.origin using functions like indexOf, startsWith, etc..

I am learning this amazing tool and I would like to contribute, so any comment/feedback is more than welcomed.

[esbena]

I like the idea in the query. It may be a bit too noisy to enable by default since there are legitimate uses of cross-origin communication that do not require origin checking, but if you are willing to look at those false positives, then the query is good.

See my inline comments about the QL content.

For the record, we already treat cross-origin messages without an origin check as taint sources for our security queries.

[esbena]

Suggested change

	exists(InsufficientOriginChecks insufficientChecks |
	this.getAPropertyRead("origin").getAMethodCall*() = insufficientChecks
	)
	this.getAPropertyRead("origin").getAMethodCall() instanceof InsufficientOriginChecks

[dellalibera]

Thanks for the all the feedback. I really appreciated.

I have just a comment about this suggestion. Should it be

this.getAPropertyRead("origin").getAMethodCall*() instanceof InsufficientOriginChecks

?

Without the * I don't catch for example the following case:

    let origin = event.origin.toLowerCase();
    let host = window.location.host;
    // BAD
    if (origin.startsWith(host) === -1)
        return;

This is because in the if statement the resolved predicate is event.origin.toLowerCase().startsWith() (please do not hesitate to correct me if I’m wrong).

[esbena]

I see, that is a good point. Lets add the * then.

(If you are interested, then the way we would check for such extended flows in a non-experimental query, would be using "type tracking". Type tracking would also allow us to identify bad checks in other functions for instance. But that is an advanced feature that can be added later)

[dellalibera]

Perfect, so I leave the *.

[esbena]

We have a few API methods for calls like this already. Do they suit your needs?

See this for instance:

codeql/javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql

Lines 16 to 32 in d80a033

	/**
	* A check on a string for whether it contains a given substring, possibly with restrictions on the location of the substring.
	*/
	class SomeSubstringCheck extends DataFlow::Node {
	DataFlow::Node substring;
	SomeSubstringCheck() {
	this.(StringOps::StartsWith).getSubstring() = substring or
	this.(StringOps::Includes).getSubstring() = substring or
	this.(StringOps::EndsWith).getSubstring() = substring
	}
	/**
	* Gets the substring.
	*/
	DataFlow::Node getSubstring() { result = substring }
	}

[dellalibera]

Thanks for pointing this out. I was already aware of these APIs, indeed I was inspired by a similar query:

codeql/javascript/ql/src/Security/CWE-020/IncorrectSuffixCheck.ql#L18-L23

Since I am pretty new to this framework, what are the benefits of using the suggested API methods?

I just want to detect if there is a weak function call (like indexOf) to validate the origin.

It seems to me that these API methods are preferred if you what to reason about the object passed to these methods.

In any case, if you think that the actual implementation can be improved, let me know. I will try to combine the already existing API methods and what I have done.

Again, thanks for your feedback.

[esbena]

This is just a minor suggestion, feel free to ignore it.

Since I am pretty new to this framework, what are the benefits of using the suggested API methods?

The benefit is that StringOps::... is like to cover more cases, and that it is likely to be extended in the future. See this case for instance:

codeql/javascript/ql/src/semmle/javascript/InclusionTests.qll

Lines 110 to 122 in 03c6d7a

	/**
	* A call to `_.includes` or similar, assumed to operate on strings.
	*/
	private class Includes_Library extends Range, DataFlow::CallNode {
	Includes_Library() {
	exists(string name |
	this = LodashUnderscore::member(name).getACall() and
	(name = "includes" or name = "include" or name = "contains")
	or
	this = Closure::moduleImport("goog.string." + name).getACall() and
	(name = "contains" or name = "caseInsensitiveContains")
	)
	}

Of course, if you spot a false positive from your query due to the use of StringOps::..., then the right choice is the simpler list of method names that you already have.

[dellalibera]

Thanks for explaining this :)

Glad to know the difference. Since StringOps covers more cases of functions that can be used to validate the origin (improperly), I changed the code in favour of StringOps.
Thanks for the feedback.

[esbena]

Feel free to adjust the QL more if my replies have inspired you, but otherwise I think we are good once the documentation has been fixed.
I will start a large evaluation once you are satisfied.

[dellalibera]

@esbena I have made the changes. Let me know if now it’s ok and if there is something else I can do. Again, thanks for all the feedback.

[esbena]

I am merging this now.

Thank you for the contribution @dellalibera.
Thank you for your suggestions @JLLeitschuh.

[dellalibera]

@esbena it was really a pleasure. I am really happy that this PR was merged.
I learnt a lot of stuff thanks to your feedback.

@JLLeitschuh thanks for your precious suggestions. I really appreciated.