fix: DOM Clobbering CVE #22688
fix: DOM Clobbering CVE #22688
Conversation
|
|
Sec-ant
changed the title
|
Can you describe the vulnerability in detail in the context of Emscripten? I tried to read the linked CVE, but I was not quite able to grok it. :/ |
|
My impression from the code change is that Emscripten should not depend on Is that what is happening? Although if that is the case, couldn't an attacker do document.currentScript = {
src: 'http://rogueserver.com/that/steals/your/data/',
tagName: 'SCRIPT'
};that would also pass the changed code in this PR? |
Not exactly. The issue here isn't that attackers can directly change <img name="currentScript" src="http://rogueserver.com/malicious.js">When the The DOM element insertion (like the |
Sec-ant
force-pushed
the
security/dom-clobbering-cve
branch
from
2c68aed
to
21ff524
Compare
|
Ok, today I learned that if I add an element to the DOM with a And this happens regardless of what the string <html><body>
<img name='currentScript' src='http://google.com/foo.js'>
<img name='elements' src='http://this.destroys.elements/'>
<img name='getElementById' src='http://this.destroys.getElementById.function/'>
<img name='createElement' src='http://this.destroys.createElement.function/'>
<img name='createTextNode' src='http://this.destroys.createTextNode.function/'>
<img name='head' src='http://this.destroys.document.head.element/'>
<img name='body' src='http://this.destroys.document.body.element/'>
<img name='documentElement' src='http://this.destroys.document.documentElement/'>
<img name='querySelector' src='http://this.destroys.document.querySelector/'>
<img name='URL' src='http://this.destroys.document.URL/'>
<img name='addEventListener' src='http://this.destroys.document.addEventListener/'>
<img name='removeEventListener' src='http://this.destroys.document.removeEventListener/'>
<img name='callEventListeners' src='http://this.destroys.document.callEventListeners/'>
<img name='pointerLockElement' src='http://this.destroys.document.pointerLockElement/'>
<img name='fullscreenElement' src='http://this.destroys.document.fullscreenElement/'>
<img name='title' src='http://this.destroys.document.title/'>
<img name='styleSheets' src='http://this.destroys.document.styleSheets/'>
<img name='createEvent' src='http://this.destroys.document.createEvent/'>
<img name='fullscreenEnabled' src='http://this.destroys.document.fullscreenEnabled/'>
<img name='exitPointerLock' src='http://this.destroys.document.exitPointerLock/'>
<img name='visibilityState' src='http://this.destroys.document.visibilityState/'>
<img name='hidden' src='http://this.destroys.document.hidden/'>
<img name='hasFocus' src='http://this.destroys.document.hasFocus/'>
<img name='fireEvent' src='http://this.destroys.document.fireEvent/'>
<script>
console.log(document.currentScript.src);
console.dir(document.elements); // All these console.dir()s give an "img" element above
console.dir(document.getElementById);
console.dir(document.createElement);
console.dir(document.createTextNode);
console.dir(document.head);
console.dir(document.body);
console.dir(document.documentElement);
console.dir(document.querySelector);
console.dir(document.URL);
console.dir(document.addEventListener);
console.dir(document.removeEventListener);
console.dir(document.callEventListeners);
console.dir(document.pointerLockElement);
console.dir(document.fullscreenElement);
console.dir(document.title);
console.dir(document.styleSheets);
console.dir(document.createEvent);
console.dir(document.fullscreenEnabled);
console.dir(document.exitPointerLock);
console.dir(document.visibilityState);
console.dir(document.hidden);
console.dir(document.hasFocus);
console.dir(document.fireEvent);
</script></body></html>that can all be replaced in that manner, if an attacker has the ability to inject unsanitized DOM elements to a web page. This raises some thoughts:
<html><body>
<img name='currentScript' src='http://google.com/foo.js'>
<script>
console.log(document.currentScript.src);
</script></body></html>prints out I tried searching if there is anything but couldn't find a conversation, so ended up writing whatwg/dom#1315 . Maybe that will get closed as a duplicate if it has been discussed before.
|
IMO those behaviors are not bugs but HTML living standards:
According to wikipedia:
Discussions and proposals:
A comprehensive review of DOM clobbering: https://doi.org/10.1109/SP46215.2023.10179403
Yes, not only the
Considered that I'm not very familiar with the Emscripten codebase, and I'm also just a regular user that bumped into this discovery while checking CVE-2024-47068 alerts across my projects, I respect your decisions in every way :) |
Thanks - reading these links makes this feel like an example of perfect is the enemy of good. I opened a separate ticket to discuss narrower against this |
| CODE 1083 129 | ||
| DATA 72 1214 | ||
| JS 4799 0 | ||
| Total 6056 |
There was a problem hiding this comment.
Its nice to see the progress we've made on overall code size here since this test was last rebaselined
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Hey, just wanted to jump in here and share that there is a theoretical bypass for the In particular, when This gap can be fixed by additionally checking that - var _scriptName = (typeof document != 'undefined' && document.currentScript?.tagName === 'SCRIPT') ? document.currentScript.src : undefined
+ var _scriptName = (typeof document != 'undefined' && document.currentScript instanceof Node && document.currentScript?.tagName === 'SCRIPT') ? document.currentScript.src : undefinedHere's a PoC of the problem:A standalone using <html><body>
<iframe name="currentScript" srcdoc="<script>window.tagName='SCRIPT';window.src='http://evil.com';</script>"></iframe>
<script>
setTimeout(() =>
alert((typeof document != 'undefined' && document.currentScript?.tagName === 'SCRIPT') ? document.currentScript.src : undefined)
, 100);
</script>
</body></html>Alternatively, put the following files in the same directory and run <!-- parent.html -->
<html><body>
<iframe name="currentScript" src="./child.html"></iframe>
<script>
setTimeout(() =>
alert((typeof document != 'undefined' && document.currentScript?.tagName === 'SCRIPT') ? document.currentScript.src : undefined)
, 100);
</script>
</body></html><!-- child.html -->
<html><head><script>
window.tagName = "SCRIPT";
window.src = "http://evil.com";
</script></head></html>Footnotes
|
The
shell.jsscript has the similar DOM Clobbering vulnerability as described in CVE-2024-47068. This PR fixes it following the patches applied in therolluprepo.The
test_emsize.jsis genereted and affected by theshell.jspatch, so the test fixtures are regenerated and rebaselined using the following command as suggested intest_other.py: