Skip to content
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

path/filepath: Clean removes ending slash for volume on Windows in Go 1.21.4 #64028

Closed
calmh opened this issue Nov 9, 2023 · 18 comments
Closed
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows release-blocker Security
Milestone

Comments

@calmh
Copy link
Contributor

calmh commented Nov 9, 2023

Go 1.21.4 fixed a problem in filepath.Clean regarding \??\ paths. However, it also removed the ending slash for volumes in \\?\ paths. Is this expected? The docs still state

The returned path ends in a slash only if it represents a root directory, such as "/" on Unix or C:\ on Windows.

The following test passes on Go 1.21.3.

func TestFilepathClean(t *testing.T) {
	cases := []struct {
		in  string
		out string
	}{
		{`C:\`, `C:\`},
		{`\\?\C:\`, `\\?\C:\`},
	}
	for _, c := range cases {
		if out := filepath.Clean(c.in); out != c.out {
			t.Errorf("filepath.Clean(%s) => %s expected %s", c.in, out, c.out)
		}
	}
}

On Go 1.21.4 this happens:

PS C:\Users\jb\dev\syncthing\lib\fs> go1.21.4 test -run FilepathClean
--- FAIL: TestFilepathClean (0.00s)
    basicfs_windows_test.go:30: filepath.Clean(\\?\C:\) => \\?\C: expected \\?\C:\
FAIL
exit status 1
FAIL    github.com/syncthing/syncthing/lib/fs   0.062s
@qmuntal
Copy link
Contributor

qmuntal commented Nov 9, 2023

Thanks for reporting. Seems like a regression.

@golang/windows @neild

@qmuntal qmuntal added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 9, 2023
@bcmills bcmills added this to the Go1.22 milestone Nov 9, 2023
@bcmills
Copy link
Contributor

bcmills commented Nov 9, 2023

@gopherbot, please backport to Go 1.21 and maybe 1.20. This appears to be a regression introduced in a security release.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #64040 (for 1.20), #64041 (for 1.21).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/541175 mentions this issue: path/filepath: consider \\?\c: as a volume on Windows

@neild
Copy link
Contributor

neild commented Nov 9, 2023

This falls out from an inadvertent change to what we consider the "volume name" to be in paths starting with \\?\. https://go.dev/cl/541175 restores the previous behavior.

@dagood
Copy link
Contributor

dagood commented Nov 9, 2023

I confirmed the CL also fixes another bit of fallout: calling filepath.EvalSymlinks on a path like \\?\Volume{45a4...f138}\ fails with go1.21.4, go1.20.11, and gotip: GetFileInformationByHandle \\?\Volume{45a4...f138}: Incorrect function.. With gotip download 541175, the call succeeds.

Reading the code, this makes sense, but I figured it might still be worth mentioning. 🙂

thaJeztah pushed a commit to thaJeztah/go that referenced this issue Nov 13, 2023
… Windows

While fixing several bugs in path handling on Windows,
beginning with \\?\.

Prior to #540277, VolumeName considered the first path component
after the \\?\ prefix to be part of the volume name.
After, it considered only the \\? prefix to be the volume name.

Restore the previous behavior.

Fixes golang#64040
Updates golang#64028

Change-Id: I6523789e61776342800bd607fb3f29d496257e68
Reviewed-on: https://go-review.googlesource.com/c/go/+/541175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit eda42f7)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah pushed a commit to thaJeztah/go that referenced this issue Nov 13, 2023
… Windows

While fixing several bugs in path handling on Windows,
beginning with \\?\.

Prior to #540277, VolumeName considered the first path component
after the \\?\ prefix to be part of the volume name.
After, it considered only the \\? prefix to be the volume name.

Restore the previous behavior.

Fixes golang#64041
Updates golang#64028

Change-Id: I6523789e61776342800bd607fb3f29d496257e68
Reviewed-on: https://go-review.googlesource.com/c/go/+/541175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit eda42f7)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah pushed a commit to thaJeztah/go that referenced this issue Nov 13, 2023
… Windows

While fixing several bugs in path handling on Windows,
beginning with \\?\.

Prior to #540277, VolumeName considered the first path component
after the \\?\ prefix to be part of the volume name.
After, it considered only the \\? prefix to be the volume name.

Restore the previous behavior.

Fixes golang#64040
Updates golang#64028

Change-Id: I6523789e61776342800bd607fb3f29d496257e68
Reviewed-on: https://go-review.googlesource.com/c/go/+/541175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit eda42f7)
thaJeztah pushed a commit to thaJeztah/go that referenced this issue Nov 13, 2023
… Windows

While fixing several bugs in path handling on Windows,
beginning with \\?\.

Prior to #540277, VolumeName considered the first path component
after the \\?\ prefix to be part of the volume name.
After, it considered only the \\? prefix to be the volume name.

Restore the previous behavior.

Fixes golang#64041
Updates golang#64028

Change-Id: I6523789e61776342800bd607fb3f29d496257e68
Reviewed-on: https://go-review.googlesource.com/c/go/+/541175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit eda42f7)
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/541520 mentions this issue: [release-branch.go1.20] path/filepath: consider \\?\c: as a volume on Windows

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/541521 mentions this issue: [release-branch.go1.21] path/filepath: consider \\?\c: as a volume on Windows

@thaJeztah
Copy link
Contributor

Opened backports for go1.20 and go1.21;

@bcmills do you know if there's an ETA for patch-releases with this fix? It's currently preventing various projects from updating (and thus missing the security fixes)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/541836 mentions this issue: [release-branch.go1.20] path/filepath: consider \\?\c: as a volume on Windows

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/541837 mentions this issue: [release-branch.go1.21] path/filepath: consider \\?\c: as a volume on Windows

@thaJeztah
Copy link
Contributor

@bcmills do you know if there's an ETAfor go1.20 and 1.21 patch releases with this fix? This regression is currently preventing various projects from updating (and thus patching the CVEs fixed in the latest release)

@thaJeztah
Copy link
Contributor

@rsc @bcmills anyone able to provide info on an ETA for a new patch release?

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 27, 2023
@bcmills
Copy link
Contributor

bcmills commented Nov 27, 2023

Generally patch releases occur monthly, although I'm not sure about the timing for the next releases due to U.S. holidays in November and December. (@golang/release may have a clearer idea of when they will occur.)

@thaJeztah
Copy link
Contributor

Thanks! Yeah, people may get itchy if they run their vulnerability scanner and see "unpatched CVE" on binaries, so was curious if there was an ETA with the patches for this regression.

@dmitshur
Copy link
Contributor

Minor releases are planned for the beginning of each month. I expect the next month's (December) minor releases to follow that pattern.

@thaJeztah
Copy link
Contributor

Gotcha. I wasn't sure if regressions introduced in security releases followed the same schedule, or if there were exceptions to that.

gopherbot pushed a commit that referenced this issue Nov 28, 2023
… Windows

While fixing several bugs in path handling on Windows,
beginning with \\?\.

Prior to #540277, VolumeName considered the first path component
after the \\?\ prefix to be part of the volume name.
After, it considered only the \\? prefix to be the volume name.

Restore the previous behavior.

For #64028.
Fixes #64041.

Change-Id: I6523789e61776342800bd607fb3f29d496257e68
Reviewed-on: https://go-review.googlesource.com/c/go/+/541175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit eda42f7)
Reviewed-on: https://go-review.googlesource.com/c/go/+/541521
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Nov 28, 2023
… Windows

While fixing several bugs in path handling on Windows,
beginning with \\?\.

Prior to #540277, VolumeName considered the first path component
after the \\?\ prefix to be part of the volume name.
After, it considered only the \\? prefix to be the volume name.

Restore the previous behavior.

For #64028.
Fixes #64040.

Change-Id: I6523789e61776342800bd607fb3f29d496257e68
Reviewed-on: https://go-review.googlesource.com/c/go/+/541175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit eda42f7)
Reviewed-on: https://go-review.googlesource.com/c/go/+/541520
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@neild
Copy link
Contributor

neild commented Nov 28, 2023

Relnote:

Go 1.20.11 and Go 1.21.4 inadvertently changed the definition of the volume name in Windows paths starting with \\?\, resulting in filepath.Clean(\\?\c:\) returning \\?\c: rather than \\?\c:\ (among other effects). The previous behavior has been restored.

This is an update to CVE-2023-45283 and https://go.dev/issue/64028.

dorileo added a commit to dorileo/guest-test-infra that referenced this issue Nov 29, 2023
We found an issue[1] in golang's path/filepath stdlib impacting the
build of at least osconfig (potentially others). The bug was fixed
and will potentially be available in 1.31.5, we'll get it update
whenever it's released.

[1] - golang/go#64028
google-oss-prow bot pushed a commit to GoogleCloudPlatform/guest-test-infra that referenced this issue Nov 29, 2023
We found an issue[1] in golang's path/filepath stdlib impacting the
build of at least osconfig (potentially others). The bug was fixed
and will potentially be available in 1.31.5, we'll get it update
whenever it's released.

[1] - golang/go#64028
rcrozean pushed a commit to rcrozean/go that referenced this issue Dec 7, 2023
While fixing several bugs in path handling on Windows,
beginning with \\?\.

Prior to #540277, VolumeName considered the first path component
after the \\?\ prefix to be part of the volume name.
After, it considered only the \\? prefix to be the volume name.

Restore the previous behavior.

For golang#64028.
Fixes golang#64040.

Change-Id: I6523789e61776342800bd607fb3f29d496257e68
Reviewed-on: https://go-review.googlesource.com/c/go/+/541175
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
(cherry picked from commit eda42f7)
Reviewed-on: https://go-review.googlesource.com/c/go/+/541520
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Windows release-blocker Security
Projects
None yet
Development

No branches or pull requests

8 participants