fix(extract): prevent raced symlink writes outside cwd#456
fix(extract): prevent raced symlink writes outside cwd#456isaacs merged 1 commit intoisaacs:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens tar extraction against a TOCTOU symlink race by ensuring extracted file writes don’t follow a symlink that appears after an initial lstat() check, and adds regression coverage for the scenario.
Changes:
- Update
getWriteFlag()to useO_NOFOLLOW(non-Windows, when available) for extracted file writes. - Adjust
getWriteFlagtests to account for numeric flag returns on Unix platforms. - Add a new regression test that simulates a raced-in destination symlink and asserts extraction does not overwrite a file outside the extraction directory.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/symlink-race-extract.ts | New regression test for symlink swap race during extraction. |
| test/get-write-flag.js | Updates Unix expectations to match the new O_NOFOLLOW-based flags. |
| src/get-write-flag.ts | Adds O_NOFOLLOW-based write flags on non-Windows platforms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'm fine with adding Technically "race conditions where attacker has direct access to the filesystem" is not a case that tar can ever be 100% resilient to (as mentioned in the docs), but a crash is for sure better than any other kind of misbehavior, and it's simple enough to just provide the flag. |
Fix a race in file extraction where a destination path could be swapped to a symlink after the existing `lstat()` check but before the file was opened. This change uses `O_NOFOLLOW` for extracted file writes on non-Windows platforms so the final open does not follow a symlink at the destination path. Also adds a regression test that reproduces the race and checks that extraction does not overwrite a file outside the target directory. PR-URL: isaacs#456 Credit: @Jvr2022 Close: isaacs#456 Reviewed-by: @isaacs
|
Everything should be fixed @isaacs |
Fix a race in file extraction where a destination path could be swapped to a symlink after the existing `lstat()` check but before the file was opened. This change uses `O_NOFOLLOW` for extracted file writes on non-Windows platforms so the final open does not follow a symlink at the destination path. Also adds a regression test that reproduces the race and checks that extraction does not overwrite a file outside the target directory. PR-URL: isaacs#456 Credit: @Jvr2022 Close: isaacs#456 Reviewed-by: @isaacs
|
Landed and published, thanks! |
Fix a race in file extraction where a destination path could be swapped to a symlink after the existing
lstat()check but before the file was opened.This change uses
O_NOFOLLOWfor extracted file writes on non-Windows platforms so the final open does not follow a symlink at the destination path.Also adds a regression test that reproduces the race and checks that extraction does not overwrite a file outside the target directory.