doc: clarify policy expectations #48947
Conversation
|
Review requested:
|
doc/api/permissions.md
Outdated
| * The policies guarantee the file integrity when a module is loaded using | ||
| `require()` or `import()`. | ||
| * Redirection does not prevent access to APIs through means such as direct | ||
| access to `require.cache` or through `module.constructor` which allow access to | ||
| loading modules. Policy redirection only affects specifiers to `require()` and | ||
| `import`. Other means, such as to prevent undesired access to APIs through | ||
| variables, are necessary to lock down that path of loading modules. |
There was a problem hiding this comment.
module.constructor` which allow access to
loading modules.
So this probably shouldn't be listed. The intent is to prevent loading new code without going through another module. Since Module is accessible through direct access in CJS and is not directly censorable without it likely should be considered at a similar tier to require or import since it is directly supplied and not leaked by a side channel like require.cache. I'd also be fine saying CJS isn't covering that specific object capability as designs for ESM avoided this kind of leakage (at least while I was more involved, unclear on these days).
Other means, such as to prevent undesired access to APIs through
variables, are necessary to lock down that path of loading modules.
Maybe we should state that it is expected that the approval of module integrity in policies' threat model implies they are allowed to muck with and even circumvent security features once loaded so environmental/runtime hardening is expected (this is true for other things like http as well anyway so not uncommon, but a callout might be good). Tools to do static analysis can aid here from linting to full blown security tools.
There was a problem hiding this comment.
How is it now? 32b0a1a
There was a problem hiding this comment.
@RafaelGSS it looks a lot better, though new Module() might need some workshopping though as that API doesn't actually guarantee context on its own so that goes against Policies in some way, the constructor itself doesn't actually load the code to eval. Module.prototype.load / Module._load are the paths to cause code to load, and using these without constraints is available using module.constructor._load leading to => extensions implementations that separately enforce policies such as ._compile for CJS. Lack of context means you can escape policy "dependencies" for a specific dependent if you get access to them. So it may be necessary to more clearly define how these APIs that lack a trusted context for enforcement need to be framed.
There was a problem hiding this comment.
So I think we should list the API calls allowed when using Module and policies, therefore, any variant distinct from this list isn't supported:
module.requirerequire('module').require()
What do you think? Any other suggestions to make it clear?
RafaelGSS
added
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/48947 ✔ Done loading data for nodejs/node/pull/48947 ----------------------------------- PR info ------------------------------------ Title doc: clarify policy expectations (#48947) Author Rafael Gonzaga (@RafaelGSS) Branch RafaelGSS:doc/clarify-policy-doc -> nodejs:main Labels doc, commit-queue-squash Commits 3 - doc: clarify policy expectations - fixup! mention new Module() supported and integrity approval - fixup! fixup! mention new Module() supported and integrity approval Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/48947 Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/48947 Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - fixup! fixup! mention new Module() supported and integrity approval ℹ This PR was created on Fri, 28 Jul 2023 04:07:03 GMT ✔ Approvals: 1 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/48947#pullrequestreview-1551533105 ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/6003207322 |
RafaelGSS
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 9eb84fe |
PR-URL: #48947 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs#48947 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
https://github.com/nodejs-private/node-private/issues/448