Fix independently-verified findings from a security audit - #7503
Conversation
A security audit flagged several latent issues; each was independently reproduced before fixing: - docker/entry.sh built its command line by hand-wrapping each arg in single quotes, then handed the string to `bash -c`. An arg containing a single quote breaks out of that quoting into the shell, so any caller able to pass through arbitrary Nextflow command-line args (e.g. a container invoker) could inject shell commands. - TowerXAuth logged raw bearer/refresh JWTs at debug/trace level. Those levels get enabled during routine troubleshooting, and support logs get pasted into tickets/chat more often than anyone would like — the tokens shouldn't be there to leak in the first place. - RemoteSession's zip extraction resolved entry names directly against the target directory with no containment check (a classic Zip Slip), so a crafted `../../` entry could write outside the target. No production caller reaches it today, but the method is reachable and the fix is cheap enough that "currently unused" isn't a reason to leave it exploitable. - The Gradle wrapper had no distributionSha256Sum, so a compromised or MITM'd services.gradle.org response would be used unverified. Pinned after downloading the distribution and independently computing its checksum myself, rather than trusting the published hash. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Phil Ewels <phil.ewels@seqera.io>
✅ Deploy Preview for nextflow-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Phil Ewels <phil.ewels@seqera.io>
pditommaso
left a comment
There was a problem hiding this comment.
Thanks for picking these up — three of the four changes look correct to me. I verified that the pinned distributionSha256Sum matches the published checksum for gradle-9.6.1-bin.zip, and the Zip-Slip guard in RemoteSession is sound (Path.startsWith is element-wise so the /tmp/foo vs /tmp/foobar prefix trap doesn't apply, both sides are normalized, and absolute entry names like /etc/passwd are caught because resolve returns them verbatim). Token redaction in TowerXAuth is good too.
The blocker is docker/entry.sh: as written the %q re-quoting does not close the injection in the NXF_USRMAP branch — I reproduced it locally. Details inline.
Replace the su/bash -c string round-trip with runuser so argv passes through without a third shell parse, and install util-linux so runuser is available in the image. Keep refresh response bodies on non-200 only so failure diagnostics remain without logging JWTs on success. Assisted-by: Cursor Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-authored-by: Cursor <cursoragent@cursor.com>
A security audit of this repo surfaced a handful of latent issues. I reproduced each one independently before touching anything, and only fixed what I could confirm was real.
Why these matter, not just what changed:
None of these needed new dependencies or behavior changes — each was a small, targeted correction once confirmed.