Repository navigation
🌱 Use os.Root to extract and read remote repository archives - #5353
Thanatos9404 wants to merge 2 commits into
Conversation
GitHub Actions
Failure
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5353 +/- ##
===========================================
+ Coverage 66.80% 77.36% +10.56%
===========================================
Files 230 255 +25
Lines 16602 14480 -2122
===========================================
+ Hits 11091 11203 +112
+ Misses 4808 3276 -1532
+ Partials 703 1 -702 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
GitHub and GitLab tarball handling leaves input and gzip reader resources open on return paths, causing descriptor leaks and preventing temporary-directory cleanup on Windows.
2 open issues
What changed in this PR
This pull request hardens remote repository archive extraction and file access by rooting operations in temporary directories with Go’s os.Root APIs, preventing traversal through .. paths and symlinks.
Changes:
- Uses rooted extraction and file reads for GitHub, GitLab, and Azure DevOps archives.
- Retains lexical ZipSlip validation while enforcing root-relative filesystem access.
- Adds traversal and symlink-escape tests and updates the THR05 documentation.
| File | Description |
|---|---|
docs/security/threat-catalog.yaml |
Documents rooted archive extraction and file access. |
clients/gitlabrepo/tarball.go |
Roots GitLab tar extraction and file reads. |
clients/gitlabrepo/tarball_test.go |
Tests GitLab tar extraction and file-read path escapes. |
clients/githubrepo/tarball.go |
Roots GitHub tar extraction and file reads. |
clients/githubrepo/tarball_test.go |
Tests GitHub tar extraction and file-read path escapes. |
clients/azuredevopsrepo/zip.go |
Roots Azure DevOps ZIP extraction and file reads. |
clients/azuredevopsrepo/zip_test.go |
Tests Azure DevOps ZIP extraction and file-read path escapes. |
Review effort: Lite
| root, err := os.OpenRoot(handler.tempDir) | ||
| if err != nil { | ||
| return fmt.Errorf("os.OpenRoot: %w", err) | ||
| } | ||
| defer root.Close() |
There was a problem hiding this comment.
| root, err := os.OpenRoot(handler.tempDir) | ||
| if err != nil { | ||
| return fmt.Errorf("os.OpenRoot: %w", err) | ||
| } | ||
| defer root.Close() |
There was a problem hiding this comment.
Same fix here in 517a75a, including the outFile close on the io.Copy error path.
The GitHub, GitLab and Azure DevOps clients now create extracted files and open them for GetFileReader through an os.Root on the temporary directory, the same way the local directory client opens files since it moved to os.OpenInRoot. The ZipSlip checks stay in place, so their errors are unchanged. Signed-off-by: Yashvardhan Thanvi <[email protected]>
extractTarball opened the downloaded tarball and a gzip reader but never closed them, and an output file stayed open when io.Copy failed. On Windows the open handle stopped cleanup from removing the temp directory. Signed-off-by: Yashvardhan Thanvi <[email protected]>
10e8310 to
517a75a
Compare
GitHub Actions
Inactive
|
Did some testing. The new outsideTempDir tests fail on main and pass on the branch, and I ran against sylva-core and two of my own test projects with and without the change: scores and show-details output were identical for every check that ran. ZipSlip still runs before os.Root, so escaped names never reach it. I also fed a "../evil" entry to the GitHub, GitLab, and Azure DevOps extractors on main and on the branch, and all three return the same error as before and write nothing outside the temp dir, because the existing lexical path check still runs before the os.Root calls. One caveat on my side: the symlink-escape subtests skip on my Windows box for lack of symlink privilege, so I could not exercise those myself. |
|
Thanks for testing this so thoroughly, including the |
What kind of change does this PR introduce?
Hardening of the remote clients' archive handling, with no score changes apart from the edge cases below.
What is the current behavior?
The GitHub, GitLab and Azure DevOps clients rely on a lexical ZipSlip check before plain
oscalls when extracting, andgetFileopensfilepath.Join(tempDir, name)unchecked. Callers only pass names fromListFilesor constants.What is the new behavior (if this is a feature change)?
Like #4785 did for
localdir,getFilenow usesos.OpenInRoot, and extraction creates entries through anos.Rooton the temp dir, so THR05 no longer rests on the lexical check alone (its text is updated). The ZipSlip check still runs first, so its errors, the skipped entries and the file lists are unchanged.getFilenow refuses names that leave the temp dir through..or a symlink, and no longer opens names relative to the working directory when no tarball was downloaded. On Windows, an entry named like a reserved device such asNUL, or containing a colon, now makes extraction fail with an error.New tests in each client fail on main and pass with this change on Linux.
Which issue(s) this PR fixes
NONE
Special notes for your reviewer
This overlaps #5342 and #5125, so I'll resolve conflicts with whichever lands first. Happy to add an openspec change for the
getFileand Windows behavior if you'd like one.Does this PR introduce a user-facing change?