Repository navigation
Locking down the "process" and "Buffer" globals #235
Description
Activity
It can be guaranteed that a module only has access to the global and imported bindings.
A module also has access to every global variable, which in both node and browsers is a large number. I believe you can also do
Function('foo = 3')(); console.log(foo)in a Module and 3 will be logged.I don't think that it's possible to lock down any globals via modules, nor do I think it would be in any way a good idea to make different non-module-related globals available in CJS vs ESM.
The proper way to make this sort of change is in CJS itself, at which point, ESM would be able to leverage the change for free.
Reacted by Wesley Wighamthis isn't actually a change to be made to modules, its a change to be made to regular node core. you would have to remove
processandBufferfrom the global object, possibly re-injecting them in cjs.Reacted by Jordan Harband and Myles BorinsWouldn't it be better to run un- or semi-trusted code in separate contexts or isolates?
But I'm generally +1 on deprecating the globals in favor of explicit imports/requires everywhere. We could even have a flag that opts into the removal.
this isn't actually a change to be made to modules, its a change to be made to regular node core. you would have to remove process and Buffer from the global object, possibly re-injecting them in cjs.
I disagree - it's already bad enough that for some reason
requireand__filenamebeing context-sensitive psuedo-globals is apparently reason for them to be excluded from the esm global scope (unless you read cjs wrapper spec or assume implementation details, you access them like they're globals in your script and can usually assume they mostly behave like fancy globals up to some edge cases) - making more things into psudo-globals isn't the solution - proper node-wide deprecation and removal is. (I really dislike anything that leads to extra divergence between cjs and esm scripts) The global scope is the global scope - what appears to be global today should continue to appear to be global in a module.And I think @jkrems is right - if you really want module-level security, you should really be trying to run untrusted code in a new context/isolate. Proxy or wrapper based sandboxing is a kind of half-effort, and doesn't prevent a large class of issues (ie, denial of service through infinite work loops).
It can be guaranteed that a module only has access to the global and imported bindings.
That's doesn't seem to be quite true, if I'm understanding you right. Just like how a
requirefunction can be passed around in cjs, so, too, can a dynamicimport- I can create a module such asexport default id => import(id)and thenimport fromOther from "./other"; export let x; fromOther("some-other-package").then(mod => x = mod);@weswigham the key of my statement was "its a change to be made to regular node core. you would have to remove process and Buffer from the global object". i agree that we shouldn't make more pseudo-globals. my overarching point is that this wouldn't be a change the modules group makes, it would be a change node collaborators in general make because it would be a change to all of node.
Reacted by Jordan HarbandForcing ESM users to explicitly import "@node/Buffer" or "@node/process" seems like a small impact given users are writing for a whole new module system.
The benefit is significant: we help encourage the creation of Web-compatible modules, increase the visibility of dependencies, and set a clear precedent for a more secure/explicit way of accessing functionality.
I struggle to believe we'll ever get much traction or benefit from eliminating them from CJS modules, so wouldn't want to tie this improvement to changing the CJS world.
The benefit is significant: we help encourage the creation of Web-compatible modules, increase the visibility of dependencies, and set a clear precedent for a more secure/explicit way of accessing functionality.
On a non-technical note, I disagree with this point - there's little practical difference between depending on an
@node/Bufferimport specifier and a potentially polyfilled globalBuffer(in both cases, in the browser, you'll need to have made additional code to provideBufferavailable - either via the import or the global). Except you can't feature detect the import for a conditional fallback (this is a known downside to static imports) without relying on dynamic import, which taints your program with asynchronicity. So in some ways, an import is worse than a global. The usual reason to avoid globals is to avoid conflicting globals, butnodehas implicitly solidly reservedprocessandBufferin the global scope with it's popularity already - it's not much of a concern here.Reacted by Jordan Harband@devsnek We could solve this by using a separate global object for ESM. Is this what you had in mind @guybedford?
@weswigham I think there is a practical difference in the way the browser error is surfaced to users: load time vs potentially late runtime failure. And there's a difference in static analysis to detect usage: it cannot be safely done on globals, whereas you can for import assuming you avoid dynamic import of non-string literals.
Also polyfilling is likely to be solved at the loader level last time I checked import-maps/layered-apis.We could solve this by using a separate global object for ESM
that's not... a thing. the global is the global. you would need to propose a spec change which adds... "non-global globals"? i'm not really sure what the semantics of this would be but it sounds like a rabbit hole of evil.
Reacted by Jordan HarbandI don’t think ESM should be taken as an opportunity to clean up unrelated things you don’t like in node. If something is subpar, it should be fixed for all node users.
Reacted by Wesley Wigham, Sven Sauleau and Chengzhong WuHere's a new proposal for Node in general that could work. Please can everyone let me know feedback and if you would support this and I can begin the process of bringing this into Node.js core. I'm glad we're all coming around on security awareness :)
@ljharb @devsnek you're exactly right - so, yes the tricky part here is that ES modules don't have a "separate" global. So that's why the previous approach needed proxies and magic to get it to work.
-
Add "process" and "Buffer" into the CJS wrapper itself, so that they are locals. This means code assuming
processorBufferin CJS can continue to work always. Breaking change risk: Assignments toprocessandBufferwill no longer override the global but only the module-scope values. -
Then start a deprecation path for
processandBufferonglobalby adding a warning to theglobal.processandglobal.Buffergetters. -
Land modules alongside the above deprecation warnings so that we can start to build the modules ecosystem without these globals, allowing a better security footing moving forward.
-
Eventually remove
global.Bufferandglobal.processentirely.
Reacted by Jimmy Wärting-
@guybedford seeing as we're on the same page, i think you should bring this issue to node core, since there are no actual changes to modules happening here.
Thanks @devsnek I wanted to at least have some support from the modules group in this direction as it will be somewhat of an undertaking, but certainly I will do that!
Per the separated globals, non-context based isolation is being looked at by Realms and things like the WindowProxy effectively are being used in other scenarios where the global is shared but given different views.
Realms is looking at this to help with what they call "identity discontinuity" which is the problem where
Arrayis not shared between isolated pieces of code soarr instanceof Arrayis unreliable. On last weeks call the Realms group were thinking of making a JS spec PR to formally allow hosts to setup per-source text GlobalThisValue instead of a single shared one.WindowProxy is... complicated, but not going to change anytime. It is unknown if the change from the Realms group has any problems with it, but it seems unlikely.
If someone wanted to change the behavior of globals I'd suggest they get together with the Realms group (feel free to email myself or @erights). I'd agree with @devsnek that this might better be done outside of the Modules WG.
5 remaining items
@bmeck great point... I point out examples in case someone else has been working towards a proposal because it is helps make my point less vague.
I followed realms sparsely in the past and was eager to catch up again given their lastest and hopefully final spur. Yes, the last thing we want is another proposal getting in the way of finally getting it through.
Are you aware of people who are looking into importing of built-ins (static or dynamic)?
@SMotaal no dynamically generated import values are being looked at to my knowledge. A new importable standard library might be what you are looking for? Though to make it safe against a variety of historical issues the actual library itself will likely not be usable in the same sense that most existing built-ins are due to things like freezing and shared identity across Realms.
Reacted by Saleh Abdel Motaalmy 2c:
Buffershould never have been global- deprecating 'global-like access' for either in CJS contexts is not feasible
- I recall us trying this at some point for
Bufferin early Node / io.js but I can't find any refs...
- I recall us trying this at some point for
@Fishrock123 thanks for the feedback - as mentioned my approach was to seek to add deprecation warnings to
global.Bufferandglobal.processaccess for the next Node, while adding them into the CJS wrapper. The wrapper handling should cover the major case. Would be interesting to hear your concerns further here...Reacted by Charles Samborskiit might be useful to move this conversation to core, for more feedback.
Reacted by Jordan Harband, Jeremiah Senkpiel and Michaël Zasso@guybedford do we want to move this to Core or can we close this as a non-Modules WG issue?
I've finally created a Node.js core PR for this in nodejs/node#26334.
I'm adding the module agenda label as we have some concerns on this feature from group members that requires some discussion further.
I'm going to close this due to staleness and hopes that Realms/Compartments solve this issue.
ES modules provide a strong encapsulation. It can be guaranteed that a module only has access to the global and imported bindings. This means module-level security becomes a very real possibility by providing import permissions per-module, which is quite exciting (and possible future directions for this group and related modules work in Node).
One thing that puts a spanner in all this is the
processandBufferglobals in Node.js. They are always available in ES modules and if we ship modules with these then they could turn out difficult to deprecate. For example,processgives access to high-resolution timers, OS details, base-level hooks and all native bindings. All of these are huge access vectors inhibiting security of modules.I previously attempted to lock down these globals in nodejs/ecmascript-modules#5 but this was shot down pretty quickly for being a bad approach in terms of performance.
I'd like to reopen this discussion though, because if we can stop the ecosystem from using the
processandBufferglobals, this will put us on a strong path for enabling modular security in Node.js going forward, and there will be no easier time to make this change than in the switch to modules. Once code in the wild relies on this stuff, deprecation gets much harder.