Fix: Zip Slip path traversal in POST /file-upload: ZIP entry paths are only checked with a substring match, allowing arbitrary file write anywhere under the application root - #368
Conversation
Co-Authored-By: Wes Convery <2wconvery@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Hi @WesternConcrete, thank you for your contribution! 🙌 Unfortunately, this PR does not meet our contributing guidelines and has been closed:
Please address the above and open a new PR. If you have questions, check our contributing guidelines |
|
Hi @WesternConcrete, thank you for your contribution! 🙌 Unfortunately, this PR does not meet our contributing guidelines and has been closed:
Please address the above and open a new PR. If you have questions, check our contributing guidelines |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| challengeUtils.solveIf(challenges.fileWriteChallenge, () => { return absolutePath === path.resolve('ftp/legal.md') }) | ||
| if (absolutePath.includes(path.resolve('.'))) { | ||
| entry.pipe(fs.createWriteStream('uploads/complaints/' + fileName).on('error', function (err) { next(err) })) | ||
| if (absolutePath.startsWith(uploadDir + path.sep)) { |
There was a problem hiding this comment.
🟡 Blocked overwrite still solves challenge
A traversal entry targeting ftp/legal.md triggers solveIf before the containment check drains it. The legal file stays unchanged, but the Arbitrary File Write challenge is awarded.
Learn more
The challenge completion predicate runs when the parser sees a target path, not when extraction writes that path. The new containment guard rejects the only path matching ftp/legal.md, but it runs after solveIf. The challenge definition still requires overwriting the legal file in the challenge metadata. Consequently, the endpoint reports success for the challenge without performing its objective.
Example: An archive containing ../../ftp/legal.md resolves to the legal file. solveIf marks the challenge solved, then the guard drains the entry. A subsequent GET /ftp/legal.md returns the original content although the scoreboard shows completion.
Recommended fix: Retire or disable fileWriteChallenge when traversal is blocked, or redesign its objective and completion condition around a safe training target. Do not call solveIf merely from the rejected entry path; completion must verify the challenge's documented effect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const uploadDir = path.resolve('uploads/complaints') | ||
| const absolutePath = path.resolve(uploadDir, fileName) | ||
| challengeUtils.solveIf(challenges.fileWriteChallenge, () => { return absolutePath === path.resolve('ftp/legal.md') }) | ||
| if (absolutePath.includes(path.resolve('.'))) { | ||
| entry.pipe(fs.createWriteStream('uploads/complaints/' + fileName).on('error', function (err) { next(err) })) | ||
| if (absolutePath.startsWith(uploadDir + path.sep)) { | ||
| entry.pipe(fs.createWriteStream(absolutePath).on('error', function (err) { next(err) })) |
There was a problem hiding this comment.
🔍 Containment fix lacks regression tests
No test covers accepted entries, rejected traversal, or challenge completion. Repository rules require tests and RSN verification for challenge-related changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| challengeUtils.solveIf(challenges.fileWriteChallenge, () => { return absolutePath === path.resolve('ftp/legal.md') }) | ||
| if (absolutePath.includes(path.resolve('.'))) { | ||
| entry.pipe(fs.createWriteStream('uploads/complaints/' + fileName).on('error', function (err) { next(err) })) | ||
| if (absolutePath.startsWith(uploadDir + path.sep)) { | ||
| entry.pipe(fs.createWriteStream(absolutePath).on('error', function (err) { next(err) })) | ||
| } else { | ||
| entry.autodrain() |
There was a problem hiding this comment.
🔍 Challenge neutralization lacks lifecycle changes
The PR intentionally neutralizes the challenge while leaving it enabled and documented as solvable. Its metadata and training lifecycle need follow-up.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Description
Finding: Zip Slip path traversal in POST /file-upload: ZIP entry paths are only checked with a substring match, allowing arbitrary file write anywhere under the application root
Repo: COG-GTM/juice-shop
Fix approach:
handleZipFileUploadinroutes/fileUpload.tsnow resolves each entry againstuploads/complaintsand only writes it when the resolved path starts with that directory (absolutePath.startsWith(uploadDir + path.sep)), replacing theincludes(path.resolve('.'))substring check; entries that escape the directory are drained.Note: this intentionally neutralises the
fileWriteChallengetraining challenge, which relies on this behaviour.Resolved or fixed issue: none
AI Tool Disclosure
DevinClaudeDaily security sweep: find and fix substantiated vulnerabilities in COG-GTM/juice-shopAffirmation
Link to Devin session: https://app.devin.ai/sessions/4b1b2e7dcc0a41848f4c6a189d79dfef
Open in Devin Desktop: https://app.devin.ai/desktop/session/4b1b2e7dcc0a41848f4c6a189d79dfef?variant=devin
Requested by: @WesternConcrete