fix: prevent path traversal during plugin extraction#755
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the ZIP extraction logic in ZipPluginDecoder to prevent path traversal vulnerabilities (Zip Slip) by introducing path validation helpers (safeExtractPath and pathIsInside) and adds corresponding unit tests. The review feedback suggests wrapping the file reader in an io.LimitReader to protect against Zip Bomb attacks, and explicitly checking for .. in the path validation to ensure robust path traversal prevention.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if _, err := io.Copy(writer, reader); err != nil { | ||
| reader.Close() | ||
| writer.Close() | ||
| return err | ||
| } |
There was a problem hiding this comment.
To protect against Zip Bomb (Denial of Service) attacks where a malicious ZIP file contains manipulated headers (e.g., declaring a small UncompressedSize64 but actually decompressing to gigabytes of data), you should wrap the file reader in an io.LimitReader using the declared UncompressedSize64. This ensures that the extraction process will not decompress more data than the header claims, preventing disk exhaustion.
limitedReader := io.LimitReader(reader, int64(file.UncompressedSize64))
if _, err := io.Copy(writer, limitedReader); err != nil {
reader.Close()
writer.Close()
return err
}| if entryPath == "." || | ||
| path.IsAbs(entryPath) || | ||
| strings.HasPrefix(entryPath, "../") || | ||
| strings.Contains(entryPath, "/../") || | ||
| strings.Contains(entryPath, `\`) || | ||
| filepath.IsAbs(filepath.FromSlash(entryPath)) { |
There was a problem hiding this comment.
The current path traversal validation checks if entryPath starts with ../ using strings.HasPrefix(entryPath, "../"). However, if entryPath is exactly .. (which can happen if the entry name is .. or resolves to it via path.Clean), it will bypass this check because it does not start with ../. While pathIsInside will eventually catch and reject it, it is safer, more explicit, and more efficient to reject .. directly in the first validation block.
| if entryPath == "." || | |
| path.IsAbs(entryPath) || | |
| strings.HasPrefix(entryPath, "../") || | |
| strings.Contains(entryPath, "/../") || | |
| strings.Contains(entryPath, `\`) || | |
| filepath.IsAbs(filepath.FromSlash(entryPath)) { | |
| if entryPath == "." || | |
| entryPath == ".." || | |
| path.IsAbs(entryPath) || | |
| strings.HasPrefix(entryPath, "../") || | |
| strings.Contains(entryPath, "/../") || | |
| strings.Contains(entryPath, `\\`) || | |
| filepath.IsAbs(filepath.FromSlash(entryPath)) { |
| return nil | ||
| } | ||
|
|
||
| func safeExtractPath(dst, entryName string) (string, error) { |
There was a problem hiding this comment.
func safeExtractPath(dst, entryName string) (string, error) {
if entryName == "" || strings.Contains(entryName, `\`) {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
for _, part := range strings.Split(entryName, "/") {
if part == ".." {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
}
entryPath := path.Clean(entryName)
if entryPath == "." || path.IsAbs(entryPath) {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
targetPath := filepath.Join(dst, filepath.FromSlash(entryPath))
if !pathIsInside(dst, targetPath) {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
return targetPath, nil
}
| filename = filepath.Join(workingPath, filename) | ||
| if z.reader == nil { | ||
| return z.err | ||
| } |
There was a problem hiding this comment.
root, err := os.OpenRoot(dst)
if err != nil {
return err
}
defer root.Close()
for _, file := range z.reader.File {
entryPath, err := safeEntryPath(file.Name)
if err != nil {
return err
}
if file.FileInfo().IsDir() {
if err := root.MkdirAll(entryPath, 0755); err != nil {
return err
}
continue
}
if err := root.MkdirAll(path.Dir(entryPath), 0755); err != nil {
return err
}
if err := extractZipFile(root, entryPath, file); err != nil {
return err
}
}
return nil
Summary
Tests
go test ./pkg/plugin_packager/decoder ./pkg/plugin_packager/packagerNotes:
go test ./pkg/plugin_packageris blocked locally becausepkg/license/private_key/PRIVATE_KEY.pemis not present in the checkout.go test -race ./pkg/plugin_packager/decoderis blocked locally because the current Go environment has cgo disabled.