-
Notifications
You must be signed in to change notification settings - Fork 252
SECVULN-24969 - resolve improper validation CVE-2025-22868 #541
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
1326b80
801ba8d
52d9a08
24c2f2f
7f99559
9d9fdd1
3cd0d65
d03ff24
e568742
c463ee5
9d87af7
8f468c1
d33a17f
727af94
3e6804d
3f06e97
0a0d7d1
951aee7
074727e
a860b9f
000f712
6cbc44e
9632307
31747c7
e62e131
2de52c6
f36f43d
8e32f51
4ff6c06
0d763f2
49752e1
978d263
4e9643a
4444d31
048ef73
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 |
---|---|---|
|
@@ -12,8 +12,7 @@ jobs: | |
strategy: | ||
matrix: | ||
go-version: | ||
- 1.18 | ||
- 1.19 | ||
- 1.23 | ||
permissions: | ||
id-token: write | ||
contents: read | ||
|
@@ -91,8 +90,7 @@ jobs: | |
strategy: | ||
matrix: | ||
go-version: | ||
- 1.18 | ||
- 1.19 | ||
- 1.23 | ||
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. I see that the workflow now only tests on Go 1.23, and support for Go 1.18/1.19 was removed. This effectively drops CI coverage for older Go versions. Is this intentional? 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. Yes, this change is intentional. The required Go version has been updated to 1.23 in go.mod due to the upgrade of the oauth dependency to version 0.27.0. As a result, it's necessary to align the CI and builds with Go 1.23 or later. We can certainly add Go 1.24 to the matrix for testing as well. However, support for older versions like 1.18 and 1.19 is no longer viable, as they fail linting and other checks. 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. Thanks for clarification 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. Similar comment as https://github.com/hashicorp/go-getter/pull/541/files#r2294091217, we should rely on the stable versions of go which are 1.24 and 1.25. |
||
permissions: | ||
id-token: write | ||
contents: read | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ jobs: | |
- name: Setup go | ||
uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 | ||
with: | ||
go-version: '^1.15' | ||
go-version: '^1.23' | ||
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. Similar comment as https://github.com/hashicorp/go-getter/pull/541/files#r2294091217, we should rely on the stable versions of go which are 1.24 and 1.25. |
||
- name: Setup signore | ||
uses: hashicorp/setup-signore@v2 | ||
with: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
"fmt" | ||
"os" | ||
"path/filepath" | ||
"runtime" | ||
"strings" | ||
) | ||
|
||
|
@@ -16,6 +17,26 @@ func mode(mode, umask os.FileMode) os.FileMode { | |
return mode & ^umask | ||
} | ||
|
||
func safeEvalSymlinks(path string) (string, error) { | ||
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. There's nothing in the PR description that explains why we're doing this, just a vague reference to stdlib libraries. It doesn't appear to be at all related to CVE-2025-22868 which is an oauth library bug. We shouldn't be pushing in unrelated work into a security PR; if there's another CVE to cover that should be its own PR. 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. Hi Tim, the version upgrade was required as minimum supported version for oauth 0.27.0 (patched version) is go version 1.23 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. The way I understand this is:
If my understanding is right, won't this introduce a new SECVULN? Adding @james-warren0 as well so that he can share his views on this. 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. @dduzgun-security has spent a lot of time with Nomad and go-getter tracking down this kind of problem so he might have more thoughts. He'll probably suggest that we look at using 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. I agree. I think 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. Yes, we definitely should be cautious here. I would strongly recommend working on the bump (without the code changes on how we deal with symlinks) and having another work item / task to explore migrating to os.Root. |
||
resolved, err := filepath.EvalSymlinks(path) | ||
if err == nil || runtime.GOOS != "windows" { | ||
return resolved, err | ||
} | ||
|
||
// On Windows with Go 1.23+, EvalSymlinks may not resolve junctions | ||
fi, statErr := os.Lstat(path) | ||
if statErr != nil { | ||
return "", err // fallback to original error | ||
} | ||
Comment on lines
+27
to
+30
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. off the top of my head - should the 2 errors be joined |
||
|
||
if fi.Mode()&os.ModeSymlink != 0 || fi.Mode()&os.ModeIrregular != 0 { | ||
return path, nil | ||
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. Just adding a note here for visibility - maybe someone with more context can clarify, but here are my observations:
In all cases, if EvalSymlinks failed, the function just hands back the original path with nil error. This feels risky to me - it might be safer to fail or add stricter checks instead of treating all these cases as safe |
||
} | ||
|
||
// It's a normal directory or file — return it as-is | ||
return path, nil | ||
} | ||
|
||
// copyDir copies the src directory contents into dst. Both directories | ||
// should already exist. | ||
// | ||
|
@@ -24,7 +45,7 @@ func copyDir(ctx context.Context, dst string, src string, ignoreDot bool, disabl | |
// We can safely evaluate the symlinks here, even if disabled, because they | ||
// will be checked before actual use in walkFn and copyFile | ||
var err error | ||
resolved, err := filepath.EvalSymlinks(src) | ||
resolved, err := safeEvalSymlinks(src) | ||
if err != nil { | ||
return err | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,12 +84,13 @@ func init() { | |
// src is a URL, whereas dst is always just a file path to a folder. This | ||
// folder doesn't need to exist. It will be created if it doesn't exist. | ||
func Get(dst, src string, opts ...ClientOption) error { | ||
return (&Client{ | ||
result := (&Client{ | ||
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. Is this really required, can we revert back to the good olden code? |
||
Src: src, | ||
Dst: dst, | ||
Dir: true, | ||
Options: opts, | ||
}).Get() | ||
return result | ||
} | ||
|
||
// GetAny downloads a URL into the given destination. Unlike Get or | ||
|
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.
could we maybe bump up to 1.25 already since that's the latest version available?
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.
This is a matrix, so we want the currently supported versions: 1.24 and 1.25.
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.
Yep, the stable versions of go are
1.24.X
and1.25.X
https://go.dev/dl/ meaning that1.23.X
won't get updates. We should fix this.