fix: prevent world-readable race window when storing local secrets#7307
fix: prevent world-readable race window when storing local secrets#7307aknownuser wants to merge 2 commits into
Conversation
✅ Deploy Preview for nextflow-docs canceled.
|
pditommaso
left a comment
There was a problem hiding this comment.
The core file-level fix (create → chmod → write, atomically owner-only before content) is correct and worth shipping. But the directory-hardening additions can throw where the old code succeeded, and can bite exactly the shared-filesystem scenario this PR targets. Requesting changes on that part.
Risk 1 — parent.setPermissions('rwx------') on a dir the user doesn't own (real regression)
storeSecrets() now runs parent.setPermissions(ONLY_OWNER_DIR_PERMS) unconditionally, on every write. chmod(2) requires you to own the directory (or be root) — being able to write into it is not enough.
- Old contract: storing secrets only needed write+execute on the parent dir.
- New contract: you must also own the parent dir, or the chmod fails with
FileSystemException: Operation not permitted(EPERM), which propagates out ofclose()→nextflow secrets setaborts.
This regresses precisely the HPC/CI shared filesystem case the PR motivates: NXF_SECRETS_FILE pointing into a group-writable directory the user can write to but doesn't own. Previously worked, now throws.
The same call in makeStoreFile() is lower risk — it's guarded to a directory we just mkdirs'd, so we own it.
Risk 2 — non-POSIX filesystem (pre-existing, slightly widened)
setPermissions / asFileAttribute(fromString(...)) throw UnsupportedOperationException on filesystems without POSIX support (CIFS/SMB, some FUSE/network mounts). The class already assumed POSIX (loadSecrets() reads getPermissions()), so this isn't new — but note makeStoreFile() now chmods in the constructor on first run, moving the failure from "when loading/storing" to "at provider construction."
Side effect even when it doesn't throw
parent.setPermissions('rwx------') on every store silently reclamps the directory to 700. If a user intentionally set the secrets dir to 750, or pointed NXF_SECRETS_FILE at a shared ~/.nextflow, every write strips group/other bits — destructive and surprising for a step labelled "defense in depth."
Suggested change
Directory hardening is explicitly defense-in-depth; it should never abort the operation the file-level fix already secures. Two minimal fixes:
- Only chmod the dir when we create it (mirror
makeStoreFile's guard), not on everystoreSecrets. - Make the dir chmod non-fatal — the security guarantee lives on the file perms, not the directory.
// storeSecrets: only harden the dir we create, and never let it abort the write
if( !parent.exists() ) {
if( !parent.mkdirs() )
throw new IOException("Unable to create directory: $parent -- Check file system permissions")
try { parent.setPermissions(ONLY_OWNER_DIR_PERMS) }
catch( Exception e ) { log.warn "Cannot restrict permissions on secrets dir '$parent' - $e.message" }
}Keep the file-level createFile + setPermissions fatal — that one is the security guarantee.
The local secrets provider wrote the secrets JSON with default umask permissions and only afterwards tightened them to 0600. Under the common umask 022 the store file was briefly world-readable (0644) while it already held secret values, letting another local user on a shared filesystem race-read them. Create the store file atomically with owner-only permissions before writing any secret content (0600 has no group/other bits, so umask cannot widen it), and restrict the secrets directory to the owner. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Oussama El Azizi <oussama.elazizi@seqera.io>
Signed-off-by: Oussama El Azizi <oussama.elazizi@seqera.io>
f5783ee to
0fb1e92
Compare
Summary
The local secrets provider wrote the secrets JSON file with the process's default umask permissions and only tightened them to
0600afterwards. Under the commonumask 022, the store file was briefly world-readable (0644)while it already contained secret values, allowing another local user on ashared filesystem (HPC/CI) to race-read secrets created by
nextflow secrets set. This closes that window.Changes and Reasoning
LocalSecretsProvider.storeSecrets()— the store file is now created atomically with owner-only permissions before any secret content is written, using a POSIX file-attribute at creation time. Becauserw-------has no group/other bits, the umask cannot widen it, so the file is never observable as world-readable while holding secrets. Permissions are also re-asserted in case the file already existed with broader access. This mirrors the already-correct create→chmod→write ordering used by the siblingmakeTempSecretsFile()method.rwx------both inmakeStoreFile()(constructor path) and instoreSecrets(). Directory traversal only leaks secret names, not values,but tightening it removes the traversal precondition the advisory relies on.
Testing
should create the store file with owner-only permissions— asserts the store file isrw-------and the secrets directory isrwx------.should never expose secret content through world-readable permissions during write— a watcher thread busy-polls the file whilestoreSecrets()runs 100 times; the test fails if the file is ever observed with group/other read bits. Reproduces the advisory's PoC race and confirms it can no longer win.LocalSecretsProviderTestsuite still passes (./gradlew :nextflow:test --tests "nextflow.secret.LocalSecretsProviderTest").