Skip to content

path/filepath: Clean on some invalid Windows paths can lose .. components #61866

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

Jump to bottom
Closed
neild opened this issue Aug 8, 2023 · 5 comments
Closed

path/filepath: Clean on some invalid Windows paths can lose .. components #61866

neild opened this issue Aug 8, 2023 · 5 comments
Assignees
@neild
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone
Go1.22

Comments

@neild
Copy link
Contributor

neild commented Aug 8, 2023

On Windows:

filepath.Clean("a/../b:/../../c") == "c" // expect "../c"

This is due to a bug in CL 468123, which ensures Clean will not convert a relative path into one starting with a drive reference. (Clean("a/../b:") == "./b:", not "b:".)

An effect of this is that filepath.IsLocal(p) and filepath.IsLocal(filepath.Clean(p)) may be different, with Clean converting a non-local relative path into a local one as in the above example. I don't think this is a vulnerability as such, but it's worth noting.

Thanks to Juho Nurminen of Mattermost for reporting this issue.
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/517216 mentions this issue: path/filepath: don't drop .. elements when cleaning invalid Windows paths

@bcmills bcmills added OS-Windows NeedsFix The path to resolution is known, but the work has not been done. Proposal-FinalCommentPeriod and removed Proposal-FinalCommentPeriod labels Aug 8, 2023
@bcmills bcmills added this to the Go1.22 milestone Aug 8, 2023
@neild
Copy link
Contributor Author

neild commented Aug 8, 2023

@gopherbot please open backport issues. This is a bug with no good workarounds.

This was referenced Aug 8, 2023
Closed
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #61867 (for 1.20), #61868 (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/519655 mentions this issue: [release-branch.go1.21] path/filepath: don't drop .. elements when cleaning invalid Windows paths

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/519636 mentions this issue: [release-branch.go1.20] path/filepath: don't drop .. elements when cleaning invalid Windows paths

gopherbot pushed a commit that referenced this issue Aug 23, 2023
@neild @cagedmantis
[release-branch.go1.20] path/filepath: don't drop .. elements when cl…
f0b8768 
…eaning invalid Windows paths

Fix a bug where Clean could improperly drop .. elements from a
path on Windows, when the path contains elements containing a ':'.

For example, Clean("a/../b:/../../c") now correctly returns "..\c"
rather than "c".

For #61866.
Fixes #61867.

Change-Id: I97b0238953c183b2ce19ca89c14f26700008ea72
Reviewed-on: https://go-review.googlesource.com/c/go/+/517216
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Quim Muntal <[email protected]>
(cherry picked from commit 6e43407)
Reviewed-on: https://go-review.googlesource.com/c/go/+/519636
Reviewed-by: Dmitri Shuralyov <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
gopherbot pushed a commit that referenced this issue Aug 23, 2023
@neild @cagedmantis
[release-branch.go1.21] path/filepath: don't drop .. elements when cl…
45b98bf 
…eaning invalid Windows paths

Fix a bug where Clean could improperly drop .. elements from a
path on Windows, when the path contains elements containing a ':'.

For example, Clean("a/../b:/../../c") now correctly returns "..\c"
rather than "c".

For #61866.
Fixes #61868.

Change-Id: I97b0238953c183b2ce19ca89c14f26700008ea72
Reviewed-on: https://go-review.googlesource.com/c/go/+/517216
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Quim Muntal <[email protected]>
(cherry picked from commit 6e43407)
Reviewed-on: https://go-review.googlesource.com/c/go/+/519655
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>
rcrozean pushed a commit to rcrozean/go that referenced this issue Dec 7, 2023
@neild @rcrozean
path/filepath: fix various issues in parsing Windows paths
daff43d 
# AWS EKS

Backported To: go-1.19.13-eks
Backported On: Tue, 07 Nov 2023
Backported By: [email protected]
Backported From: release-branch.go1.20
Source Commit: golang@46fb781 \
  golang@45b98bf \
  golang@6d0bf43

In addition to the CVE fix golang@46fb781,

golang@45b98bf &
golang@6d0bf43 were
cherry-picked to include expected functions and tests expected in the CVE fix commit.
Additionally, for the purpose of tests I added the line to api/go1.19.txt which is used for checking the
function calls exist as expected.

# Original Information

On Windows, A root local device path is a path which begins with
\\?\ or \??\.  A root local device path accesses the DosDevices
object directory, and permits access to any file or device on the
system. For example \??\C:\foo is equivalent to common C:\foo.

The Clean, IsAbs, IsLocal, and VolumeName functions did not
recognize root local device paths beginning with \??\.

Clean could convert a rooted path such as \a\..\??\b into
the root local device path \??\b. It will now convert this
path into .\??\b.

IsAbs now correctly reports paths beginning with \??\
as absolute.

IsLocal now correctly reports paths beginning with \??\
as non-local.

VolumeName now reports the \??\ prefix as a volume name.

Join(`\`, `??`, `b`) could convert a seemingly innocent
sequence of path elements into the root local device path
\??\b. It will now convert this to \.\??\b.

In addition, the IsLocal function did not correctly
detect reserved names in some cases:

  - reserved names followed by spaces, such as "COM1 ".
  - "COM" or "LPT" followed by a superscript 1, 2, or 3.

IsLocal now correctly reports these names as non-local.

For golang#63713
Fixes golang#63714
Fixes CVE-2023-45283
Fixes CVE-2023-45284

Change-Id: I446674a58977adfa54de7267d716ac23ab496c54
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2040691
Reviewed-by: Roland Shoemaker <[email protected]>
Reviewed-by: Tatiana Bradley <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2072597
Reviewed-by: Cherry Mui <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/539276
Auto-Submit: Heschi Kreinick <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

commit: golang@6d0bf43

[release-branch.go1.21] path/filepath: don't drop .. elements when cleaning invalid Windows paths

Fix a bug where Clean could improperly drop .. elements from a
path on Windows, when the path contains elements containing a ':'.

For example, Clean("a/../b:/../../c") now correctly returns "..\c"
rather than "c".

For golang#61866.
Fixes golang#61868.

Change-Id: I97b0238953c183b2ce19ca89c14f26700008ea72
Reviewed-on: https://go-review.googlesource.com/c/go/+/517216
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Quim Muntal <[email protected]>
(cherry picked from commit 6e43407)
Reviewed-on: https://go-review.googlesource.com/c/go/+/519655
Reviewed-by: Damien Neil <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Run-TryBot: Dmitri Shuralyov <[email protected]>

commit: golang@45b98bf

path/filepath: add IsLocal

IsLocal reports whether a path lexically refers to a location
contained within the directory in which it is evaluated.
It identifies paths that are absolute, escape a directory
with ".." elements, and (on Windows) paths that reference
reserved device names.

For golang#56219.

Change-Id: I35edfa3ce77b40b8e66f1fc8e0ff73cfd06f2313
Reviewed-on: https://go-review.googlesource.com/c/go/+/449239
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Joseph Tsai <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
Reviewed-by: Joedian Reid <[email protected]>
@golang golang locked and limited conversation to collaborators Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@neild neild

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
3 participants
@neild @bcmills @gopherbot