SECVULN-24969 - resolve improper validation CVE-2025-22868 #541

ritikrajdev · 2025-08-12T02:05:55Z

Task

resolve improper validation CVE-2025-22868
updates version to 1.23 for go as the oauth package CVE has been resolved in version 0.27 which requires this go version.
Please note the tests were failing because of a few internal changes in go 1.23 for windows regarding symlinks.
Due to these changes a minor logic change has been introduced
refer to - https://pkg.go.dev/os@master, https://pkg.go.dev/path/filepath@master

NOTE: Update in go version in ci and go.mod as well.

PCI review checklist

I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
Only way is to revert the pr. Should be merged by squashing for easier revert.
If applicable, I've documented the impact of any changes to security controls.
Can impact windows user a bit.

Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

util_test.go

go.mod

copy_dir.go

CreatorHead · 2025-08-20T11:10:16Z

.github/workflows/go-getter.yml

@@ -91,8 +90,7 @@ jobs:
    strategy:
      matrix:
        go-version:
-          - 1.18
-          - 1.19
+          - 1.23


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?

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.

Thanks for clarification

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.

get_file_test.go

CreatorHead · 2025-08-21T04:48:06Z

copy_dir.go

+	}
+
+	if fi.Mode()&os.ModeSymlink != 0 || fi.Mode()&os.ModeIrregular != 0 {
+		return path, nil


Just adding a note here for visibility - maybe someone with more context can clarify, but here are my observations:

If the file is a symlink --> return path, nil (pretending it’s fine).

If the file is irregular (which on Windows usually means a junction / reparse point) → return path, nil.

If it’s a normal file/dir → also return path, nil.

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

CreatorHead

Added some note for visibility and clarification

pkazmierczak · 2025-08-22T12:26:36Z

.github/workflows/go-getter.yml

@@ -12,8 +12,7 @@ jobs:
    strategy:
      matrix:
        go-version:
-          - 1.18
-          - 1.19
+          - 1.23


could we maybe bump up to 1.25 already since that's the latest version available?

This is a matrix, so we want the currently supported versions: 1.24 and 1.25.

Yep, the stable versions of go are 1.24.X and 1.25.X https://go.dev/dl/ meaning that 1.23.X won't get updates. We should fix this.

tgross · 2025-08-22T12:34:10Z

copy_dir.go

@@ -16,6 +17,26 @@ func mode(mode, umask os.FileMode) os.FileMode {
 	return mode & ^umask
 }

+func safeEvalSymlinks(path string) (string, error) {


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.

Hi Tim, the version upgrade was required as minimum supported version for oauth 0.27.0 (patched version) is go version 1.23

tgross · 2025-08-22T12:35:02Z

.github/workflows/go-getter.yml

@@ -12,8 +12,7 @@ jobs:
    strategy:
      matrix:
        go-version:
-          - 1.18
-          - 1.19
+          - 1.23


This is a matrix, so we want the currently supported versions: 1.24 and 1.25.

kkavish

@ritikrajdev Appreciate the work here! While this looks good to me I have a little concern around how windows junctions are being handled. I have made a small comment about that and looped in @james-warren0 from Sec Team to have a look as well.

Thanks!

kkavish · 2025-08-22T12:29:45Z

copy_dir.go

+	fi, statErr := os.Lstat(path)
+	if statErr != nil {
+		return "", err // fallback to original error
+	}


off the top of my head - should the 2 errors be joined errors.Join(error...)? or at least be logged? In the interest of not losing any errors.

kkavish · 2025-08-22T12:36:13Z

copy_dir.go

@@ -16,6 +17,26 @@ func mode(mode, umask os.FileMode) os.FileMode {
 	return mode & ^umask
 }

+func safeEvalSymlinks(path string) (string, error) {


The way I understand this is:

Resolve symlinks to the actual path

Keep others, including windows junctions as is. i.e. junctions won't be resolved.

If my understanding is right, won't this introduce a new SECVULN?
The whole point of resolving symlinks and junctions is to prevent a lower-privileged user from creating a symlink/junction to a privileged item and then have a field day with it from a security POV. The fact that we use this in Terraform and Nomad makes me more worried.

https://msrc.microsoft.com/blog/2025/06/redirectionguard-mitigating-unsafe-junction-traversal-in-windows/

Adding @james-warren0 as well so that he can share his views on this.

@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 os.Root here to avoid TOCTOU issues too.

I agree. I think os.Root is the correct way to address this issue and not miss edge cases. os.Root was introduced in go 1.24, so we would need to bump one more version to utilize it.

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.

kkavish · 2025-08-22T12:39:06Z

get.go

@@ -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{


Is this really required, can we revert back to the good olden code?

dduzgun-security

Thanks for the PR. This library has been the cause of few incidents in the past and we need to be cautious about the changes.

I think the OAuth change/bump is great but we should tackle the changes to how we deal with symlinks in another PR and prioritize the usage of os.Root which handles all scenarios (Windows, Linux) with less code maintenance for us.

dduzgun-security · 2025-08-22T15:48:18Z

.github/workflows/go-getter.yml

@@ -91,8 +90,7 @@ jobs:
    strategy:
      matrix:
        go-version:
-          - 1.18
-          - 1.19
+          - 1.23


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.

dduzgun-security · 2025-08-22T15:48:45Z

.github/workflows/release.yml

@@ -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'


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.

ritikrajdev · 2025-08-26T05:02:12Z

After going through all the comments in details, there are just a few concerns I'd like to share :-

oauth 0.27.0 which is the patched version for oauth package with the vulnerability resolved. it requires minimum go version of 1.23 hence the upgrade to 1.23+ is required.
handling of junctions and links were changed in go 1.23 hence the required changes. or else the tests would fail.

Action items :-

Create a PR with upgraded go version to 1.24
Create another PR dependent on the above one to upgrade oauth version to 0.27.0

Let me know if this sounds good @dduzgun-security @tgross @james-warren0 @kkavish !

SECVULN-24969 - resolve improper validation CVE-2025-22868

1326b80

ritikrajdev requested a review from a team as a code owner August 12, 2025 02:05

ritikrajdev and others added 28 commits August 12, 2025 07:50

chore: lint fixes due to version upgrade

801ba8d

test: windows test debug logs

52d9a08

chore: no check of symlink on windows

24c2f2f

test: check if dst exists

7f99559

test: dst exists

9d9fdd1

chore: back to original

3cd0d65

traceback required

d03ff24

more info on error

e568742

chore: remove toolchain

c463ee5

trial changes

9d87af7

chore: logs

8f468c1

chore: more logs

d33a17f

chore: logs

727af94

chore: logging

3e6804d

chore: logs

3f06e97

chore: logs

0a0d7d1

chor: logs

951aee7

chore: logs

074727e

chore: add more logs

a860b9f

chore: extensive logging

000f712

chore: more logs

6cbc44e

chore: logging

9632307

chore: other logs

31747c7

chore: eval symlinks log

e62e131

feat: logic update

2de52c6

chore: remove all logs

f36f43d

chore: perform all checks

8e32f51

chore: ci checks

4ff6c06

chore: lint fixes

0d763f2

ritikrajdev requested a review from schmichael August 20, 2025 10:20

CreatorHead requested changes Aug 20, 2025

View reviewed changes

ritikrajdev added 5 commits August 20, 2025 22:51

chore: go 1.23 updates

49752e1

chore: extensive checking and error handling for EvalSymlinks

978d263

chore: add logging and a fix

4e9643a

chore: a few changes in logic

4444d31

chore: remove logs and refine workflows

048ef73

ritikrajdev requested a review from CreatorHead August 21, 2025 04:03

CreatorHead reviewed Aug 21, 2025

View reviewed changes

mallikabandaru requested a review from kkavish August 22, 2025 10:07

pkazmierczak reviewed Aug 22, 2025

View reviewed changes

tgross reviewed Aug 22, 2025

View reviewed changes

kkavish reviewed Aug 22, 2025

View reviewed changes

dduzgun-security requested changes Aug 22, 2025

View reviewed changes

SECVULN-24969 - resolve improper validation CVE-2025-22868 #541

Are you sure you want to change the base?

SECVULN-24969 - resolve improper validation CVE-2025-22868 #541

Uh oh!

Conversation

ritikrajdev commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Task

PCI review checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CreatorHead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkavish left a comment

Choose a reason for hiding this comment

Uh oh!

kkavish Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dduzgun-security Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dduzgun-security left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ritikrajdev commented Aug 26, 2025

Uh oh!

Uh oh!

ritikrajdev commented Aug 12, 2025 •

edited

Loading

tgross Aug 22, 2025 •

edited

Loading

kkavish Aug 22, 2025 •

edited

Loading

dduzgun-security Aug 22, 2025 •

edited

Loading