-
-
Notifications
You must be signed in to change notification settings - Fork 133
fix: implement symlink security for vendor operations (CVE-2025-8959) #1446
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
83131e0
e9f90ca
ace78a4
b7fd021
8e7b61a
a4d27f8
c3c1f0e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,6 +191,12 @@ | |
atmosConfig.Vendor.BasePath = vendorBasePath | ||
} | ||
|
||
vendorPolicySymlinks := os.Getenv("ATMOS_VENDOR_POLICY_SYMLINKS") | ||
Check failureCode scanning / golangci-lint use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv" Error
use of os.Getenv forbidden because "Use viper.BindEnv for new environment variables instead of os.Getenv"
|
||
if len(vendorPolicySymlinks) > 0 { | ||
log.Debug(foundEnvVarMessage, "ATMOS_VENDOR_POLICY_SYMLINKS", vendorPolicySymlinks) | ||
atmosConfig.Vendor.Policy.Symlinks = vendorPolicySymlinks | ||
} | ||
|
||
Comment on lines
+194
to
+199
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Do not introduce new os.Getenv; bind and read via Viper. This violates the repo rule and fails golangci-lint. Use viper.BindEnv and viper.GetString for ATMOS_VENDOR_POLICY_SYMLINKS, then set atmosConfig from that value. Apply this diff within this hunk: - vendorPolicySymlinks := os.Getenv("ATMOS_VENDOR_POLICY_SYMLINKS")
- if len(vendorPolicySymlinks) > 0 {
- log.Debug(foundEnvVarMessage, "ATMOS_VENDOR_POLICY_SYMLINKS", vendorPolicySymlinks)
- atmosConfig.Vendor.Policy.Symlinks = vendorPolicySymlinks
- }
+ // Read via Viper (ENV override should be bound in load.go).
+ vendorPolicySymlinks := viper.GetString("vendor.policy.symlinks")
+ if vendorPolicySymlinks != "" {
+ log.Debug(foundEnvVarMessage, "ATMOS_VENDOR_POLICY_SYMLINKS", vendorPolicySymlinks)
+ atmosConfig.Vendor.Policy.Symlinks = vendorPolicySymlinks
+ } Add the import in this file: import (
// ...
"github.com/spf13/viper"
) And bind the ENV once (outside this file) in pkg/config/load.go using the project’s helper: // Somewhere in setEnv(...) alongside other bindings.
bindEnv(v, "vendor.policy.symlinks", "ATMOS_VENDOR_POLICY_SYMLINKS") 🧰 Tools🪛 GitHub Check: golangci-lint[failure] 194-194: 🤖 Prompt for AI Agents
|
||
stacksBasePath := os.Getenv("ATMOS_STACKS_BASE_PATH") | ||
if len(stacksBasePath) > 0 { | ||
log.Debug(foundEnvVarMessage, "ATMOS_STACKS_BASE_PATH", stacksBasePath) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,40 +1,43 @@ | ||
package downloader | ||
|
||
import ( | ||
"fmt" | ||
"net/url" | ||
"os" | ||
"path/filepath" | ||
|
||
log "github.com/charmbracelet/log" | ||
"github.com/hashicorp/go-getter" | ||
|
||
"github.com/cloudposse/atmos/pkg/security" | ||
) | ||
|
||
// CustomGitGetter is a custom getter for git (git::) that removes symlinks. | ||
// CustomGitGetter is a custom getter for git (git::) that validates symlinks based on security policy. | ||
type CustomGitGetter struct { | ||
getter.GitGetter | ||
// Policy defines how symlinks should be handled. If not set, defaults to PolicyAllowSafe. | ||
Policy security.SymlinkPolicy | ||
} | ||
|
||
// Get implements the custom getter logic removing symlinks. | ||
// Get implements the custom getter logic with symlink validation. | ||
func (c *CustomGitGetter) Get(dst string, url *url.URL) error { | ||
// Normal clone | ||
if err := c.GetCustom(dst, url); err != nil { | ||
return err | ||
return fmt.Errorf("failed to clone %s to %s: %w", url, dst, err) | ||
} | ||
|
||
// Validate symlinks based on policy (default to allow_safe if not configured) | ||
policy := c.Policy | ||
if policy == "" { | ||
policy = security.PolicyAllowSafe | ||
} | ||
// Remove symlinks | ||
return removeSymlinks(dst) | ||
|
||
if err := security.ValidateSymlinks(dst, policy); err != nil { | ||
return fmt.Errorf("symlink validation failed for %s at %s: %w", url, dst, err) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// removeSymlinks walks the directory and removes any symlinks it encounters. | ||
// Deprecated: Use security.ValidateSymlinks instead. | ||
func removeSymlinks(root string) error { | ||
return filepath.Walk(root, func(path string, info os.FileInfo, err error) error { | ||
if err != nil { | ||
return err | ||
} | ||
if info.Mode()&os.ModeSymlink != 0 { | ||
log.Debug("Removing symlink", "path", path) | ||
// Symlinks are removed for the entire repo, regardless if there are any subfolders specified | ||
return os.Remove(path) | ||
} | ||
return nil | ||
}) | ||
return security.ValidateSymlinks(root, security.PolicyRejectAll) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Fix boundary root when copying a single local file.
Using
p.uri
as the baseDir breaks boundary checks when the source is a file (base must be a directory). This can misclassify symlinks and weaken protections.Apply:
Add a test that copies a single local symlinked file where the target escapes the parent directory and confirm it is skipped under allow_safe/reject_all and allowed under allow_all.
🏁 Script executed:
Length of output: 1959
🏁 Script executed:
Length of output: 10010
🏁 Script executed:
Length of output: 3304
Fix symlink boundary for single-file copy in vendor_model.go
🤖 Prompt for AI Agents