The Wayback Machine - https://web.archive.org/web/20260215035338/https://github.com/github/codeql/pull/4263
Skip to content

JS: Add a js/client-side-url-redirect sink/source for importScripts. #4263

Merged
codeql-ci merged 8 commits intogithub:mainfrom
erik-krogh:importScripts
Sep 16, 2020
Merged

JS: Add a js/client-side-url-redirect sink/source for importScripts. #4263
codeql-ci merged 8 commits intogithub:mainfrom
erik-krogh:importScripts

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Sep 14, 2020

Adds support for the sink and source in CVE-2020-14000.

importScripts() is a global function in WebWorkers that is used for importing scripts.
(It's used quite a lot).

Calling importScripts(url) does the same as the below document.createElement(..) snippet, but in a WebWorker.

var el = document.createElement("script");
el.src = url;
document.body.appendChild(el); // NOT OK
/// ---- Basically the same as ----
importScripts(url); 

This PR also adds support for registering postMessage(..) handlers using global.onmessage.
E.g.

self.onmessage = function (e) {
    importScripts(e); // NOT OK
}

@erik-krogh erik-krogh changed the title JS: Add a js/client-side-url-redirect sink for importScripts. JS: Add a js/client-side-url-redirect sink/source for importScripts. Sep 14, 2020
@erik-krogh erik-krogh marked this pull request as ready for review September 14, 2020 21:47
@erik-krogh erik-krogh requested a review from a team as a code owner September 14, 2020 21:47
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One question and a nit.

*/
class ImportScriptsSink extends ScriptUrlSink {
ImportScriptsSink() {
this = DataFlow::globalVarRef("importScripts").getACall().getAnArgument()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

importScripts() and self.importScripts() are effectively equivalent — both represent importScripts() being called from inside the worker's inner scope.
(https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/importScripts)

Do we support the mentioned self.importScripts as well?

Copy link
Contributor Author

@erik-krogh erik-krogh Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should - I'll add a test for it.
Edit: It worked.

erik-krogh and others added 2 commits September 15, 2020 12:14
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(tests are failing though)

@erik-krogh
Copy link
Contributor Author

(tests are failing though)

They just needed a restart.

@codeql-ci codeql-ci merged commit c2175b6 into github:main Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

HTTPS · web.archive.org
← Home