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.

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

path/filepath: insecure parsing of Windows paths #63713

Closed
neild opened this issue Oct 24, 2023 · 5 comments
Closed

path/filepath: insecure parsing of Windows paths #63713

neild opened this issue Oct 24, 2023 · 5 comments
Assignees
@neild
Labels
FrozenDueToAge release-blocker Security
Milestone
Go1.22

Comments

@neild
Copy link
Contributor

neild commented Oct 24, 2023 •
edited by bcmills
Loading

path/filepath: recognize \??\ as a Root Local Device path prefix.

On Windows, a path beginning with \??\ is a Root Local Device path equivalent
to a path beginning with \\?\. Paths with a \??\ prefix may be used to
access arbitrary locations on the system. For example, the path \??\c:\x
is equivalent to the more common path c:\x.

The filepath package did not recognize paths with a \??\ prefix as special.

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 did not report paths beginning with \??\ as absolute.
It now does so.

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.

This is CVE-2023-45283 and https://go.dev/issue/63713.

path/filepath: recognize device names with trailing spaces and superscripts

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.

This is CVE-2023-45284 and https://go.dev/issue/63713.

/cc @golang/security and @golang/release
@neild neild self-assigned this Oct 24, 2023
@neild neild added this to the Go1.22 milestone Oct 24, 2023
@neild
Copy link
Contributor Author

neild commented Oct 24, 2023

@gopherbot please open backport issues for this security fix

This was referenced Oct 24, 2023
Closed
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #63714 (for 1.20), #63715 (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/540276 mentions this issue: [release-branch.go1.21] path/filepath: fix various issues in parsing Windows paths

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/539276 mentions this issue: [release-branch.go1.20] path/filepath: fix various issues in parsing Windows paths

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/540277 mentions this issue: path/filepath: fix various issues in parsing Windows paths

gopherbot pushed a commit that referenced this issue Nov 7, 2023
@neild @gopherbot
[release-branch.go1.21] path/filepath: fix various issues in parsing …
9e933c1 
…Windows paths

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 #63713
Fixes #63715
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 <bracewell@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2072596
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/540276
Auto-Submit: Heschi Kreinick <heschi@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue Nov 7, 2023
@neild @gopherbot
[release-branch.go1.20] path/filepath: fix various issues in parsing …
46fb781 
…Windows paths

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 #63713
Fixes #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 <bracewell@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2072597
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/539276
Auto-Submit: Heschi Kreinick <heschi@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@heschi heschi changed the title security: fix CVE-2023-45283 and CVE-2023-45284 path/filepath: insecure parsing of Windows paths Nov 7, 2023
@shoenig shoenig mentioned this issue Nov 7, 2023
Merged
This was referenced Nov 7, 2023
Merged
Merged
Merged
@yosifkit yosifkit mentioned this issue Nov 7, 2023
Merged
ncw added a commit to rclone/rclone that referenced this issue Nov 13, 2023
@ncw
lib/file: fix MkdirAll after go1.21.4 stdlib update
7ffe0bc 
In ths security related issue the go1.21.4 stdlib changed the parsing
of volume names on Windows.

golang/go#63713

This had the consequences of breaking the MkdirAll tests which were
looking for specific error messages which changed and using invalid
paths.

In particular under go1.21.3:

    filepath.VolumeName(`\\?\C:`) == `\\?\C:`

But under go1.21.4 it is:

    filepath.VolumeName(`\\?\C:`) == `\\?`

The path `\\?\C:` isn't actually a valid Windows path. I reported this
as a FYI bug upstream - I'm not expecting it to be fixed.

See: golang/go#64101
ncw added a commit to rclone/rclone that referenced this issue Nov 14, 2023
@ncw
lib/file: fix MkdirAll after go1.21.4 stdlib update
acf1e2d 
In ths security related issue the go1.21.4 stdlib changed the parsing
of volume names on Windows.

golang/go#63713

This had the consequences of breaking the MkdirAll tests which were
looking for specific error messages which changed and using invalid
paths.

In particular under go1.21.3:

    filepath.VolumeName(`\\?\C:`) == `\\?\C:`

But under go1.21.4 it is:

    filepath.VolumeName(`\\?\C:`) == `\\?`

The path `\\?\C:` isn't actually a valid Windows path. I reported this
as a FYI bug upstream - I'm not expecting it to be fixed.

See: golang/go#64101
@kiashok kiashok mentioned this issue Nov 15, 2023
Merged
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: rcrozean@amazon.com
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 <bracewell@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2072597
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/539276
Auto-Submit: Heschi Kreinick <heschi@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

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 <dneil@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
(cherry picked from commit 6e43407)
Reviewed-on: https://go-review.googlesource.com/c/go/+/519655
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>

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 <dneil@google.com>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Joedian Reid <joedian@golang.org>
@liggitt liggitt mentioned this issue Apr 2, 2024
Merged
@golang golang locked and limited conversation to collaborators Nov 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@neild neild

Labels
FrozenDueToAge release-blocker Security
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
2 participants
@neild @gopherbot