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.

Sign up for GitHub

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

fix: DOM Clobbering CVE #22688

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: DOM Clobbering CVE #22688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/shell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ var quit_ = (status, toThrow) => {
#if SHARED_MEMORY && !MODULARIZE
// In MODULARIZE mode _scriptName needs to be captured already at the very top of the page immediately when the page is parsed, so it is generated there
// before the page load. In non-MODULARIZE modes generate it here.
var _scriptName = (typeof document != 'undefined') ? document.currentScript?.src : undefined;
var _scriptName = (typeof document != 'undefined' && document.currentScript?.tagName.toUpperCase() === 'SCRIPT') ? document.currentScript.src : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Man.. this is kind of ugly, and seems like it will increase code size.

I'm not necessarily against landing this, but I wonder if we can further limit the cases where _scriptName is used.. maybe we can even eliminate it somehow.

Copy link
Collaborator

@juj juj Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is pretty ugly :/ I wonder if the .toUpperCase() part at least is not needed? When testing, I am getting all uppercase letters in .tagName already. Is there a known browser where tagName not in upper case, or why is the .toUpperCase() there?

I think I would write a function into parseTools.mjs:

// Return document.currentScript.src in a fashion that guards against DOM clobbering attacks.
// See https://github.com/emscripten-core/emscripten/pull/22688
function secureGetCurrentScriptSrc() {
  /* For future test for browsers that have implemented https://github.com/whatwg/html/issues/10687
  if (MIN_CHROME_VERSION >= xyz && MIN_FIREFOX_VERSION >= abc && MIN_..._VERSION >= def) {
    return 'globalThis.document?.currentScript?.src';
  } else */ if (SUPPORTS_GLOBALTHIS) {
    return "(globalThis.document?.currentScript?.tagName == 'SCRIPT' && document.currentScript.src)";
  } else {
    return "(typeof document != 'undefined' && document.currentScript?.tagName == 'SCRIPT' && document.currentScript.src)";
  }
}

and then in all these places that read document.currentScript.src, do

var _scriptName = {{{ secureGetCurrentScriptSrc() }}};

This way we have clean JS code and a way to evolve out of this awkwardness for users in the future.


#if ENVIRONMENT_MAY_BE_NODE
if (ENVIRONMENT_IS_NODE) {
Expand Down Expand Up @@ -375,7 +375,7 @@ if (ENVIRONMENT_IS_SHELL) {
if (ENVIRONMENT_IS_WEB || ENVIRONMENT_IS_WORKER) {
if (ENVIRONMENT_IS_WORKER) { // Check worker, not web, since window could be polyfilled
scriptDirectory = self.location.href;
} else if (typeof document != 'undefined' && document.currentScript) { // web
} else if (typeof document != 'undefined' && document.currentScript?.tagName.toUpperCase() === 'SCRIPT') { // web
scriptDirectory = document.currentScript.src;
}
#if MODULARIZE
Expand Down
2 changes: 1 addition & 1 deletion src/shell_minimal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ var ENVIRONMENT_IS_PTHREAD = ENVIRONMENT_IS_WORKER && self.name == 'em-pthread';
#if !MODULARIZE
// In MODULARIZE mode _scriptName needs to be captured already at the very top of the page immediately when the page is parsed, so it is generated there
// before the page load. In non-MODULARIZE modes generate it here.
var _scriptName = (typeof document != 'undefined') ? document.currentScript?.src : undefined;
var _scriptName = (typeof document != 'undefined' && document.currentScript?.tagName.toUpperCase() === 'SCRIPT') ? document.currentScript.src : undefined;
#endif

#if ENVIRONMENT_MAY_BE_NODE
Expand Down
28 changes: 11 additions & 17 deletions test/other/test_emsize.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 12 additions & 10 deletions test/other/test_emsize.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
section size addr
TYPE 51 10
IMPORT 31 63
FUNCTION 14 96
GLOBAL 9 112
EXPORT 9 123
ELEM 9 134
CODE 1556 146
DATA 77 1704
JS 6756 0
Total 8512
TYPE 41 10
IMPORT 7 53
FUNCTION 9 62
TABLE 5 73
MEMORY 6 80
GLOBAL 8 88
EXPORT 17 98
ELEM 9 117
CODE 1083 129
DATA 72 1214
JS 4799 0
Total 6056
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its nice to see the progress we've made on overall code size here since this test was last rebaselined

Binary file modified test/other/test_emsize.wasm
100644 → 100755
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion tools/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2414,7 +2414,7 @@ def modularize():
if settings.EXPORT_ES6 and settings.USE_ES6_IMPORT_META:
script_url = 'import.meta.url'
else:
script_url = "typeof document != 'undefined' ? document.currentScript?.src : undefined"
script_url = "(typeof document != 'undefined' && document.currentScript?.tagName.toUpperCase() === 'SCRIPT') ? document.currentScript.src : undefined"
if shared.target_environment_may_be('node'):
script_url_node = "if (typeof __filename != 'undefined') _scriptName = _scriptName || __filename;"
src = '''%(node_imports)s
Expand Down