apache
fix arbitrary file access during archive extraction ("Zip Slip")
https://github.com/apache/beam/blob/75cf7e182c9af656db147050f2cab1b7b374ee97/sdks/go/pkg/beam/core/runtime/xlangx/expansionx/download.go#L142-L164
To fix the issue, we need to validate the file paths extracted from the zip archive to ensure they do not contain directory traversal elements (..) and are confined to the intended destination directory. This can be achieved by resolving the absolute path of the constructed fileName and ensuring it is a subpath of the dest directory. If the validation fails, the file should be skipped or an error should be raised.
The fix involves:
- Resolving the absolute path of
fileNameusingfilepath.Abs. - Ensuring that the resolved path starts with the absolute path of the
destdirectory. - Skipping or rejecting files that fail this validation.
Extracting files from a malicious zip file, or similar type of archive, is at risk of directory traversal attacks if filenames from the archive are not properly validated. archive paths.
Zip archives contain archive entries representing each file in the archive. These entries include a file path for the entry, but these file paths are not restricted and may contain unexpected special elements such as the directory traversal element (..). If these file paths are used to create a filesystem path, then a file operation may happen in an unexpected location. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.
For example, if a zip file contains a file entry ..\beam-file, and the zip file is extracted to the directory c:\output, then naively combining the paths would result in an output file path of c:\output\..\beam-file, which would cause the file to be written to c:\beam-file.
In this an archive is extracted without validating file paths. If archive.zip contained relative paths (for instance, if it were created by something like zip archive.zip ../file.txt) then executing this code could write to locations outside the destination directory.
package main
import (
"archive/zip"
"io/ioutil"
"path/filepath"
)
func unzip(f string) {
r, _ := zip.OpenReader(f)
for _, f := range r.File {
p, _ := filepath.Abs(f.Name)
// BAD: This could overwrite any file on the file system
ioutil.WriteFile(p, []byte("present"), 0666)
}
}
To fix this vulnerability, we need to check that the path does not contain any ".." elements in it.
package main
import (
"archive/zip"
"io/ioutil"
"path/filepath"
"strings"
)
func unzipGood(f string) {
r, _ := zip.OpenReader(f)
for _, f := range r.File {
p, _ := filepath.Abs(f.Name)
// GOOD: Check that path does not contain ".." before using it
if !strings.Contains(f.Name, "..") {
ioutil.WriteFile(p, []byte("present"), 0666)
}
}
}
References
Zip Slip Vulnerability Path Traversal CWE-22
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] Mention the appropriate issue in your description (for example:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead. - [x] Update
CHANGES.mdwith noteworthy changes. - [x] If this contribution is large, please file an Apache Individual Contributor License Agreement.
See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.
Assigning reviewers:
R: @jrmccluskey for label go.
Note: If you would like to opt out of this review, comment assign to next reviewer.
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:
R: @lostluck for label go.
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
Well yes. They were passing before. Because we didn't have any tests that validate this zip slip problem.
That just means that your change didn't break existing tests. What I'd like to see is for there to be a test that catches this zip slip behavior so we don't break the safety you're adding in the future.
The "unit test" Commit just does a refactor and removes a comment. It doesn't actually add a unit test. Did you miss submitting something client side?
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:
R: @shunping for label go.
Available commands:
stop reviewer notifications- opt out of the automated review toolingremind me after tests pass- tag the comment author after tests passwaiting on author- shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
waiting on author
An unrelated helper function was changed, but no new unit test was added.
