Skip to content

Loosen up security on unzip to support the next route file pattern #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
potofpie merged 5 commits into from
Sep 5, 2025
Merged

Loosen up security on unzip to support the next route file pattern #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
730a477
fix the unzip function
potofpie Sep 5, 2025
8e6a439
fix the unzip function
potofpie Sep 5, 2025
ce143e4
Merge branch 'main' into fix-unzip
potofpie Sep 5, 2025
da16c36
fix the unzip function
potofpie Sep 5, 2025
084e652
Merge branch 'fix-unzip' of https://github.com/agentuity/go-common in…
potofpie Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions sys/io.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,15 @@ func Unzip(src, dest string, flatten bool) error {
defer r.Close()

for _, f := range r.File {
if strings.Contains(f.Name, "..") {
return fmt.Errorf("invalid file path: %s", f.Name)
// Check for directory traversal attacks by examining path components
// Split the path and check each component
pathParts := strings.Split(filepath.ToSlash(f.Name), "/")
for _, part := range pathParts {
// Block path components that could be used for directory traversal
// ".." is obvious, but also block pure dot sequences (3+ dots) that might be interpreted as ".."
if part == ".." || (len(part) >= 3 && strings.Trim(part, ".") == "") {
return fmt.Errorf("invalid file path: %s", f.Name)
}
}

rc, err := f.Open()
Expand All @@ -399,7 +406,33 @@ func Unzip(src, dest string, flatten bool) error {
f.Name = filepath.Base(f.Name)
}

// Validate file name before path construction to catch absolute paths and other attacks
if filepath.IsAbs(f.Name) {
return fmt.Errorf("invalid file path, absolute path not allowed: %s", f.Name)
}

// Check for Windows-style absolute paths and UNC paths (platform-independent)
if len(f.Name) >= 3 && f.Name[1] == ':' && (f.Name[2] == '\\' || f.Name[2] == '/') {
return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name)
}
if strings.HasPrefix(f.Name, "\\\\") {
return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name)
}

Comment on lines +409 to +421

@coderabbitai coderabbitai Bot Sep 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Harden Windows path detection: block drive-relative “C:foo” and UNC with // too.

Current check misses C:foo\bar (drive-relative). Also catch UNC prefixed by //.

-		// Check for Windows-style absolute paths and UNC paths (platform-independent)
-		if len(f.Name) >= 3 && f.Name[1] == ':' && (f.Name[2] == '\\' || f.Name[2] == '/') {
-			return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name)
-		}
-		if strings.HasPrefix(f.Name, "\\\\") {
-			return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name)
-		}
+		// Check for Windows-style paths (drive-relative like "C:foo" and absolute "C:\..."), and UNC ("\\server\share" or "//server/share")
+		name := f.Name
+		if len(name) >= 2 && ((name[1] == ':' && ((name[0] >= 'A' && name[0] <= 'Z') || (name[0] >= 'a' && name[0] <= 'z')))) {
+			return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name)
+		}
+		if strings.HasPrefix(name, "\\\\") || strings.HasPrefix(filepath.ToSlash(name), "//") {
+			return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name)
+		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Validate file name before path construction to catch absolute paths and other attacks
if filepath.IsAbs(f.Name) {
return fmt.Errorf("invalid file path, absolute path not allowed: %s", f.Name)
}
// Check for Windows-style absolute paths and UNC paths (platform-independent)
if len(f.Name) >= 3 && f.Name[1] == ':' && (f.Name[2] == '\\' || f.Name[2] == '/') {
return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name)
}
if strings.HasPrefix(f.Name, "\\\\") {
return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name)
}
// Validate file name before path construction to catch absolute paths and other attacks
if filepath.IsAbs(f.Name) {
return fmt.Errorf("invalid file path, absolute path not allowed: %s", f.Name)
}
// Check for Windows-style paths (drive-relative like "C:foo" and absolute "C:\\..."), and UNC ("\\\\server\\share" or "//server/share")
name := f.Name
if len(name) >= 2 && (name[1] == ':' && ((name[0] >= 'A' && name[0] <= 'Z') || (name[0] >= 'a' && name[0] <= 'z'))) {
return fmt.Errorf("invalid file path, Windows drive path not allowed: %s", f.Name)
}
if strings.HasPrefix(name, "\\\\") || strings.HasPrefix(filepath.ToSlash(name), "//") {
return fmt.Errorf("invalid file path, UNC path not allowed: %s", f.Name)
}
🤖 Prompt for AI Agents 
In sys/io.go around lines 409 to 421, the Windows path checks miss
drive-relative names like "C:foo" and UNC paths starting with "//"; update the
validation to reject any name where len(f.Name) >= 2 and f.Name[1] == ':'
(covers both "C:\..." and "C:foo") and also treat both backslash- and
forward-slash UNC prefixes as invalid by checking strings.HasPrefix(f.Name,
"\\\\") || strings.HasPrefix(f.Name, "//"); return the same formatted error
messages when these conditions match.

fpath := filepath.Join(dest, f.Name)

// Canonicalize paths and validate against directory traversal
destRoot := filepath.Clean(dest)
if !strings.HasSuffix(destRoot, string(os.PathSeparator)) {
destRoot += string(os.PathSeparator)
}
cleanedFpath := filepath.Clean(fpath)

// Ensure the cleaned file path is within the destination directory
if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) {
return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name)
}

Comment on lines +424 to +435

@coderabbitai coderabbitai Bot Sep 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Symlink-aware containment: resolve dest and paths before prefix compare.

Case-insensitive volumes and symlinks can bypass a pure lexical check. Normalize with Abs and resolve symlinks where possible.

-		// Canonicalize paths and validate against directory traversal
-		destRoot := filepath.Clean(dest)
+		// Canonicalize paths and validate against directory traversal
+		destRoot := filepath.Clean(dest)
+		if abs, err := filepath.Abs(destRoot); err == nil {
+			destRoot = abs
+		}
+		if resolved, err := filepath.EvalSymlinks(destRoot); err == nil {
+			destRoot = resolved
+		}
 		if !strings.HasSuffix(destRoot, string(os.PathSeparator)) {
 			destRoot += string(os.PathSeparator)
 		}
-		cleanedFpath := filepath.Clean(fpath)
+		cleanedFpath := filepath.Clean(fpath)
+		if abs, err := filepath.Abs(cleanedFpath); err == nil {
+			cleanedFpath = abs
+		}
 
 		// Ensure the cleaned file path is within the destination directory
 		if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) {
 			return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name)
 		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Canonicalize paths and validate against directory traversal
destRoot := filepath.Clean(dest)
if !strings.HasSuffix(destRoot, string(os.PathSeparator)) {
destRoot += string(os.PathSeparator)
}
cleanedFpath := filepath.Clean(fpath)
// Ensure the cleaned file path is within the destination directory
if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) {
return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name)
}
// Canonicalize paths and validate against directory traversal
destRoot := filepath.Clean(dest)
// Prefer absolute path for destRoot
if abs, err := filepath.Abs(destRoot); err == nil {
destRoot = abs
}
// Resolve any symlinks in destRoot
if resolved, err := filepath.EvalSymlinks(destRoot); err == nil {
destRoot = resolved
}
if !strings.HasSuffix(destRoot, string(os.PathSeparator)) {
destRoot += string(os.PathSeparator)
}
cleanedFpath := filepath.Clean(fpath)
// Prefer absolute path for the file being extracted
if abs, err := filepath.Abs(cleanedFpath); err == nil {
cleanedFpath = abs
}
// Ensure the cleaned file path is within the destination directory
if !strings.HasPrefix(cleanedFpath+string(os.PathSeparator), destRoot) {
return fmt.Errorf("invalid file path, outside destination directory: %s", f.Name)
}
🤖 Prompt for AI Agents 
In sys/io.go around lines 424 to 435, the current containment check uses lexical
Clean and prefix compare which can be bypassed by symlinks or case differences;
resolve this by converting dest and fpath to absolute paths and evaluating
symlinks (e.g., using filepath.Abs and filepath.EvalSymlinks) before comparison,
normalize trailing separators, and then verify containment by computing the
relative path (filepath.Rel) and ensuring it does not start with ".." (and
handle case-normalization on case-insensitive filesystems if applicable); return
the same error when the relative path escapes the destination.

if f.FileInfo().IsDir() && !flatten {
os.MkdirAll(fpath, os.ModePerm)
} else {
Expand Down
Loading
Loading