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

Jump to bottom

path/filepath: Clean(.\c:) returns c: on Windows #52476

Closed
Unrud opened this issue Apr 21, 2022 · 8 comments
Closed

path/filepath: Clean(.\c:) returns c: on Windows #52476

Unrud opened this issue Apr 21, 2022 · 8 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Security
Milestone
Go1.19

Comments

@Unrud
Copy link

Unrud commented Apr 21, 2022 •
edited

What version of Go are you using (go version)?

$ go version
go version go1.18.1 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\user\AppData\Local\go-build
set GOENV=C:\Users\user\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\user\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\user\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.18.1
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\user\AppData\Local\Temp\go-build3120958786=/tmp/go-build -gno-record-gcc-switches

What did you do?

filepath.Clean(`.\c:`)     // `c:`     Input is an invalid path; Output points to drive C
filepath.Clean(`.\c:\foo`) // `c:\foo` Input is an invalid path; Output is an absolute path
filepath.Clean(`.\c:foo`)  // `c:foo`  Input points to ADS `foo` of file `.\c`; Output points to `foo` on drive C

What did you expect to see?

That the paths are equivalent.

What did you see instead?

They are not equivalent.

Implications

It can lead to unexpected behavior:

package main

import "net/http"

func main() {
	http.ListenAndServe(":8080", http.FileServer(http.Dir(".")))
}

http://localhost:8080/c:/Users/ lists the directory content of C:\Users\.
@ianlancetaylor ianlancetaylor added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. help wanted labels Apr 21, 2022
@ianlancetaylor ianlancetaylor added this to the Go1.19 milestone Apr 21, 2022
@mattn
Copy link
Member

mattn commented Apr 22, 2022

Clean must NOT remove prefix .\ when following element is possibly absolute paths.

diff --git a/src/path/filepath/path.go b/src/path/filepath/path.go
index ec9e6d8a1f..d6257b9fe5 100644
--- a/src/path/filepath/path.go
+++ b/src/path/filepath/path.go
@@ -117,9 +117,18 @@ func Clean(path string) string {
 		case os.IsPathSeparator(path[r]):
 			// empty path element
 			r++
-		case path[r] == '.' && (r+1 == n || os.IsPathSeparator(path[r+1])):
+		case path[r] == '.' && r+1 == n:
 			// . element
 			r++
+		case path[r] == '.' && os.IsPathSeparator(path[r+1]):
+			// ./ element
+			r++
+			for os.IsPathSeparator(path[r]) {
+				r++
+			}
+			if out.w == 0 && IsAbs(path[r:]) {
+				return originalPath
+			}
 		case path[r] == '.' && path[r+1] == '.' && (r+2 == n || os.IsPathSeparator(path[r+2])):
 			// .. element: remove to last separator
 			r += 2

@mattn
Copy link
Member

mattn commented Apr 22, 2022

Hmm, this patch still have problem. When the base path is C and requested path is :, result become C:\. To completely fix this issue, we should fix . in http.Dir to be absolute path.

@gopherbot
Copy link

Change https://go.dev/cl/401595 mentions this issue: path/filepath: do not remove prefix "." when following element is absolute paths.

@mattn
Copy link
Member

mattn commented Apr 22, 2022

@ianlancetaylor This is security issue. So we have to make backports for older versions.

@ianlancetaylor
Copy link
Contributor

@gopherbot Please open backport issues.

Avoid confusion with ./C: paths.

@gopherbot gopherbot mentioned this issue Apr 22, 2022
Closed
@gopherbot
Copy link

Backport issue(s) opened: #52478 (for 1.17), #52479 (for 1.18).

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 gopherbot mentioned this issue Apr 22, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/405234 mentions this issue: [release-branch.go1.18] path/filepath: do not remove prefix "." when following path contains ":"

@gopherbot
Copy link

Change https://go.dev/cl/405235 mentions this issue: [release-branch.go1.17] path/filepath: do not remove prefix "." when following path contains ":"

@dmitshur dmitshur added Security 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 May 31, 2022
gopherbot pushed a commit that referenced this issue May 31, 2022
@mattn @dmitshur
[release-branch.go1.18] path/filepath: do not remove prefix "." when …
5a89bb3 
…following path contains ":".

For #52476
Fixes #52479
Fixes CVE-2022-29804

Change-Id: I9eb72ac7dbccd6322d060291f31831dc389eb9bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/401595
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/405234
Reviewed-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
gopherbot pushed a commit that referenced this issue May 31, 2022
@mattn @dmitshur
[release-branch.go1.17] path/filepath: do not remove prefix "." when …
4c69fd5 
…following path contains ":".

For #52476
Fixes #52478
Fixes CVE-2022-29804

Change-Id: I9eb72ac7dbccd6322d060291f31831dc389eb9bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/401595
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/405235
Reviewed-by: Yasuhiro Matsumoto <mattn.jp@gmail.com>
@tatianab tatianab mentioned this issue Jul 25, 2022
Closed
@gpshead gpshead mentioned this issue Dec 10, 2022
Open
@golang golang locked and limited conversation to collaborators May 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Security
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
5 participants
@mattn @dmitshur @ianlancetaylor @gopherbot @Unrud