-
Notifications
You must be signed in to change notification settings - Fork 510
Fix: Make it impossible to load dynamically generated shared libraries #5910
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -239,6 +239,17 @@ _REPLACEMENTS = [ | |
| "var tableBase=metadata.tableSize?wasmTable.length:0;" + | ||
| "Module.snapshotDebug && console.log('loadWebAssemblyModule', libName, memoryBase, tableBase);", | ||
| ], | ||
| [ | ||
| "function loadLibData(){", | ||
| """ | ||
| function loadLibData(){ | ||
| var f = findLibraryFS(libName, flags.rpath); | ||
| var libData = Module.patched_loadLibData(Module, f); | ||
| return flags.loadAsync ? Promise.resolve(libData) : libData; | ||
| } | ||
| function loadLibData1(){ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I'm trying to replace the definition of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. How about using a more meaningful name such as |
||
| """, | ||
| ], | ||
| ] | ||
|
|
||
| def _python_bundle(version, *, pyodide_asm_wasm = None, pyodide_asm_js = None, python_stdlib_zip = None, emscripten_setup_override = None): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ import { | |
| import { loadPackages } from 'pyodide-internal:loadPackage'; | ||
| import { default as MetadataReader } from 'pyodide-internal:runtime-generated/metadata'; | ||
| import { TRANSITIVE_REQUIREMENTS } from 'pyodide-internal:metadata'; | ||
| import { getTrustedReadFunc } from 'pyodide-internal:readOnlyFS'; | ||
|
|
||
| /** | ||
| * After running `instantiateEmscriptenModule` but before calling into any C | ||
|
|
@@ -206,6 +207,24 @@ export function clearSignals(Module: Module): void { | |
| } | ||
| } | ||
|
|
||
| function patched_loadLibData(Module: Module, path: string): WebAssembly.Module { | ||
ryanking13 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const { node } = Module.FS.lookupPath(path); | ||
| // Get the trusted read function from our private Map, not from the node | ||
| // or filesystem object (which could have been tampered with by user code) | ||
|
Comment on lines
+212
to
+213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really something we should worry about? As long as users can access the Emscripten module object (I guess they can?), they can do anything. I wonder if this additional barrier is really helpful.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Users can access the Module object. However, they cannot access objects that are captured by closures. So |
||
| const trustedRead = getTrustedReadFunc(node); | ||
| if (!trustedRead) { | ||
| throw new Error( | ||
| 'Can only load shared libraries from read only file systems.' | ||
| ); | ||
| } | ||
| const stat = node.node_ops.getattr(node); | ||
| const buffer = new Uint8Array(stat.size); | ||
| // Create a minimal stream object and read using trusted read function | ||
| const stream = { node, position: 0 }; | ||
| trustedRead(stream, buffer, 0, stat.size, 0); | ||
| return UnsafeEval.newWasmModule(buffer); | ||
| } | ||
|
|
||
| export function loadPyodide( | ||
| isWorkerd: boolean, | ||
| lockfile: PackageLock, | ||
|
|
@@ -216,6 +235,7 @@ export function loadPyodide( | |
| const Module = enterJaegerSpan('instantiate_emscripten', () => | ||
| SetupEmscripten.getModule() | ||
| ); | ||
| Module.patched_loadLibData = patched_loadLibData; | ||
| Module.API.config.jsglobals = globalThis; | ||
| if (isWorkerd) { | ||
| Module.API.config.indexURL = indexURL; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| def use(x): | ||
| pass | ||
|
|
||
|
|
||
| def test_dynlib_loading(tmp_path, monkeypatch): | ||
| # fmt: off | ||
| Path(tmp_path / "a.so").write_bytes( | ||
| bytes( | ||
| [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add a comment about the content of this file? |
||
| 0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00, 0x00, 0x0f, 0x08, 0x64, | ||
| 0x79, 0x6c, 0x69, 0x6e, 0x6b, 0x2e, 0x30, 0x01, 0x04, 0x00, 0x00, 0x00, | ||
| 0x00, 0x01, 0x04, 0x01, 0x60, 0x00, 0x00, 0x02, 0x38, 0x03, 0x03, 0x65, | ||
| 0x6e, 0x76, 0x06, 0x6d, 0x65, 0x6d, 0x6f, 0x72, 0x79, 0x02, 0x00, 0x00, | ||
| 0x03, 0x65, 0x6e, 0x76, 0x0d, 0x5f, 0x5f, 0x6d, 0x65, 0x6d, 0x6f, 0x72, | ||
| 0x79, 0x5f, 0x62, 0x61, 0x73, 0x65, 0x03, 0x7f, 0x00, 0x03, 0x65, 0x6e, | ||
| 0x76, 0x0c, 0x5f, 0x5f, 0x74, 0x61, 0x62, 0x6c, 0x65, 0x5f, 0x62, 0x61, | ||
| 0x73, 0x65, 0x03, 0x7f, 0x00, 0x03, 0x02, 0x01, 0x00, 0x07, 0x15, 0x01, | ||
| 0x11, 0x5f, 0x5f, 0x77, 0x61, 0x73, 0x6d, 0x5f, 0x63, 0x61, 0x6c, 0x6c, | ||
| 0x5f, 0x63, 0x74, 0x6f, 0x72, 0x73, 0x00, 0x00, 0x0a, 0x04, 0x01, 0x02, | ||
| 0x00, 0x0b | ||
| ] | ||
| ) | ||
| ) | ||
| # fmt: on | ||
| monkeypatch.syspath_prepend(tmp_path) | ||
| with pytest.raises( | ||
| ImportError, match="Can only load shared libraries from read only file systems" | ||
| ): | ||
| import a | ||
|
|
||
| use(a) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findLibraryFSprobably doesn't exist in old Emscripten versions? I guess I added that in 4.XThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. My thought was to get it to work for 0.28.2 first and then work on 0.26 once that's working.