feat: add plugin directory trust guard to harden shared plugin caches - #7308
feat: add plugin directory trust guard to harden shared plugin caches#7308aknownuser wants to merge 6 commits into
Conversation
Nextflow loads and executes plugin code from the local plugin cache ($id-$version) without re-verifying its origin when the directory already exists. On a shared, writable plugin cache (NXF_PLUGINS_DIR or $NXF_HOME/plugins), a lower-trust local user can pre-populate an official pinned plugin coordinate with attacker-controlled code that then runs in the victim's Nextflow process. The plugin cache is expected to be a per-user, private trust boundary (default $HOME/.nextflow/plugins). This adds a defense-in-depth guard that verifies a plugin directory is trusted before loading it: the directory must be owned by the current user or root AND not writable by group or others. This accepts the two legitimate shapes (private user cache; an admin-managed read-only shared cache) while rejecting an attacker-owned or group/world-writable directory. Behaviour is controlled by NXF_PLUGINS_STRICT_MODE: warn (default) logs a warning and continues, strict aborts the run, off disables the check. The check is a no-op on filesystems without POSIX permissions and in dev mode (where plugins load from the source tree, not the cache). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Oussama El Azizi <oussama.elazizi@seqera.io>
✅ Deploy Preview for nextflow-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Address review feedback: the mode was read verbatim, so typos, different case or a blank value would silently behave as 'warn'. Normalise the value (lower-case and trim) and validate it against the known modes, logging a warning on an unrecognised value instead of ignoring it silently. Add test coverage for normalisation and the invalid/blank fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Oussama El Azizi <oussama.elazizi@seqera.io>
Address review feedback: existing tests only exercised the permission branch. Extract the trust decision into a pure isUntrusted(perms, owner, currentUser) helper so the owner branch can be tested deterministically - a temp dir in a unit test is always owned by the current user (and would be root-owned under a root CI runner), so it cannot reach that branch via real files. Add a data-driven test covering owned-by-me, root-owned, untrusted-owner (the reported scenario) and group/world-writable cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Oussama El Azizi <oussama.elazizi@seqera.io>
pditommaso
left a comment
There was a problem hiding this comment.
The trust-guard's intent is sound and the warn/strict/off plumbing is clean. My concern is the trust predicate itself: it produces false positives in the exact deployment shapes Nextflow is most used in — spurious warnings on every run in the default warn mode, and aborted runs under NXF_PLUGINS_STRICT_MODE=strict. Requesting changes to tighten the predicate, plus a note on a stronger long-term direction.
Blocking — false positives
-
Group-writable check breaks the umask-002 default —
PluginSecurity.isUntrusted(PluginSecurity.groovy:94). On stock Ubuntu/Debian/RHEL/Fedora regular-user sessions (umask002, user-private-group),FilesEx.mkdirs(root)creates~/.nextflow/pluginsasrwxrwxr-x. The guard flags the directory Nextflow just created itself — everyrun/config/info/log/clean, and every freshly-downloaded plugin dir (PluginUpdater.groovy:406). A dir the user owns is theirs to make group-writable; the threat is a foreign writer. -
Owner compared by name, not UID —
PluginSecurity.groovy:126. When the running UID has no/etc/passwdentry (containers, OpenShift, K8s, arbitrary-UID HPC — very common for Nextflow),System.getProperty('user.name')returns?whileattrs.owner().nameresolves to the numeric UID string. Mismatch → the user's own private cache is flagged. Same issue under NFS/LDAP/AD name resolution. -
Root running against a user-owned cache —
PluginSecurity.groovy:95.sudo nextflow …with the user's own0700cache:me=root, owner =alice, soalice != me && alice != 'root'→ flagged. -
Only literal
rootis exempted, but the javadoc promises admin-managed shared caches —PluginSecurity.groovy:95. A read-only shared cache owned by a service account (e.g.nextflow:nextflow 0755) is rejected for every non-admin user. Under site-widestrict, everynextflow runaborts with "refusing to load plugins" — the exact deployment the class doc says is supported. -
Per-plugin invocation floods duplicate warnings —
PluginUpdater.groovy:406.checkTrustedDirruns once per plugin/dependency inload0, so one underlying condition emits N identical warnings per run, andgetMode()re-reads the env and re-emits the "Invalid NXF_PLUGINS_STRICT_MODE" warning N times too.
Suggested fixes (cheap, keep it offline-safe)
- Compare owner by UID, not name → fixes #2 and #3.
- Treat group/other-write as untrusted only when the owner isn't you → kills the umask-002 storm (#1).
- Make the trusted-owner set configurable, or trust a read-only dir regardless of owner → #4.
- Warn once per run, not per plugin → #5.
Better long-term strategy
Worth noting for direction (not required in this PR): download-time integrity is already covered — the registry serves sha512sum (HttpPluginRepository.groovy:220) and pf4j's CompoundVerifier checks the archive on download. This guard's real job is the post-download cache-poisoning window.
A load-time "checksum vs registry" would defend that window but requires re-fetching a trusted baseline from the registry on every run — which breaks offline/air-gapped use (a documented, supported mode) and makes registry availability a hard runtime dependency.
The genuinely stronger answer to supply-chain integrity is a committed lockfile with pinned plugin hashes, verified offline at load (the go.sum / package-lock.json model): the trusted baseline lives in the pipeline repo under version control, so it survives cache poisoning and registry compromise and silent drift, with no network. That's a separate, ADR-worthy feature — but it's the direction I'd aim for. This directory guard is a fine cheap complement in the meantime, once the false positives above are fixed.
Address review feedback: the trust predicate flagged legitimate mainstream deployments. Rework it to flag a directory only when a non-trusted principal can write to it: - Drop the group-write check: a self-owned, group-writable dir (rwxrwxr-x created under the common umask 002 with a user-private group) is now trusted. Only world-writable (others-write) remains a risk for a trusted owner. Fixes spurious warnings/aborts on every run. - Compare owner by UID instead of name, so containers/OpenShift/ arbitrary-UID/NFS/LDAP (where user.name is '?') no longer flag the user's own cache. - When running as root (uid 0), trust any owner but still flag a world-writable cache (fixes sudo against a user-owned 0700 cache). - Add NXF_PLUGINS_TRUSTED_OWNERS (UIDs or names) so an admin-managed read-only cache owned by a service account is trusted. - Warn at most once per offending directory, and emit the invalid-mode warning once, so a multi-plugin run no longer floods duplicates. - Fail open (skip the check) when owner/current UID cannot be determined. World-writable directories and foreign-owned directories are still refused, so the original PoC (root-owned 777 root + attacker-owned 755 subdir) remains caught. Update docs and tests accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Oussama El Azizi <oussama.elazizi@seqera.io>
|
Yes the long term strategy you suggested is the ideal situation, an ADR for that would be perfect! I have addressed the false positives you detected. Let me know if you think this still needs work. You may have seen that this setup is opt-in, which is made this way to ovoid any breaking change to previous usage of nextflow. |
christopher-hakkaart
left a comment
There was a problem hiding this comment.
Adding some style alignment suggestions.
Co-authored-by: Chris Hakkaart <chris.hakkaart@seqera.io> Signed-off-by: aNewUser <oussamaelazizi.opt@gmail.com>
Co-authored-by: Chris Hakkaart <chris.hakkaart@seqera.io> Signed-off-by: aNewUser <oussamaelazizi.opt@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Rework the plugin lockfile to hash the extracted $id-$version/ directory (the code Nextflow actually loads and executes) rather than the downloaded archive. Hashing the unpacked tree lets a single, content-addressed check cover both integrity surfaces: - supply chain / registry compromise / silent drift (a tampered archive extracts to a different tree), and - cache poisoning of the executed code (a lower-trust user editing already -extracted files on a shared cache) — detected regardless of directory ownership or permissions, with no ownership heuristic and therefore no false-positive warnings on legitimate shared caches. This supersedes the archive-hash approach and the directory-ownership guard proposed in #7308: it is stronger (catches modifications an ownership check misses) and quieter (silent on any healthy cache; output only on a genuine mismatch). Verification runs once per plugin in load0(), before loading, on both cold and warm caches. The retained-archive machinery and the "archive absent" case are removed (the extracted tree is always present when a plugin loads). Full re-hash on every load is acceptable for now; the ADR documents a local fingerprint-cache optimisation (gated on a private, non-writable cache) as a measure-first follow-up for large plugins. Assisted-by: Claude Opus 4.8 (via Claude Code) Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
|
Sorry for the churn on this one 🙏 — thanks for bearing with the back-and-forth. Stepping back, my main worry with the ownership/permission approach is noise on shared filesystems. The check flags a directory as untrusted when it's foreign-owned or world-writable — but the standard HPC/CI setup is an admin- or service-account-owned shared cache that every user reads. For every user who isn't that owner, each plugin dir looks "untrusted", so in the default So I took a different iteration in #7394: instead of checking who owns the plugin directory, it pins and verifies a hash of the extracted plugin contents (a committed I think #7394 supersedes this PR, so I'm inclined to close this in its favour — but happy to hear if you see a gap. Really appreciate the work that got us here; it's what surfaced the whole problem. |
Summary
Nextflow loads and executes plugin code from the local plugin cache (
$id-$version) without re-verifying its origin when the directory already exists —PluginUpdater.load0()short-circuits the download (and therefore thesha512sum/CompoundVerifierpath) whenever the cached directory is present, andPluginsFacade.isAllowed()only matches plugin IDs, not content.On a shared, writable plugin cache (
NXF_PLUGINS_DIRor$NXF_HOME/plugins), a lower-trust local user can pre-populate an official pinned plugin coordinate (e.g.nf-amazon@3.10.0) with attacker-controlled code that then runs inside a victim's Nextflow process.The plugin cache is expected to be a per-user, private trust boundary (default
$HOME/.nextflow/plugins). This PR adds a defense-in-depth guard that verifies a plugin directory is trusted before loading it, addressing the exact precondition of the issue — write access to a trusted cache — with no network calls, no checksums, and no persisted state (works offline).Changes and Reasoning
New
PluginSecurityhelper (modules/nf-commons/.../plugin/PluginSecurity.groovy)A directory is considered trusted only when it is owned by the current user (or root) and not writable by group or others. This accepts the two legitimate deployment shapes — a private user cache (e.g.
0700) and an admin-managed read-only shared cache (e.g.root:root 0755) — while rejecting an attacker-owned directory or a group/world-writable cache.Call sites
PluginsFacade.init(...)— guards the plugins root at setup (bothinitoverloads), so startup-loaded plugins are covered.PluginUpdater.load0()— guards each$id-$versiondirectory immediately beforeloadPluginFromPath, covering cache reuse and the attacker-owned-subdir case.New env var
NXF_PLUGINS_STRICT_MODE(warn|strict|off)warn(default) — logs a warning and continues (zero breakage for existing setups).strict— aborts the run withAbortOperationException.off— disables the check.The guard is a no-op on filesystems without POSIX permissions and in dev mode (where plugins load from the source tree, not the cache).
Compatibility / rollout
Default
warnmeans no existing deployment breaks; the default$HOME/.nextflow/plugins(user-owned) is silent.strictis opt-in now; a future stable release can promote the default.Testing
New
PluginSecurityTest(16 cases) and existing plugin tests pass:0700/ read-only0755→ accepted in all modeswarn, throws instrictoffmode → never throwswarndefault,strict,off):nf-commons:testfull suite — no regression inPluginsFacadeTest/PluginUpdaterTestEnd-to-end (dev launcher,
NXF_PLUGINS_MODE=prod):strict+0777cachewarn+0777cachestrict+ private0700cacheDocumentation
docs/reference/env-vars.mdx— newNXF_PLUGINS_STRICT_MODEentry.docs/plugins/using-plugins.mdx— security note in the Caching section (keep the cache private; supported shared pattern is admin-owned read-only).