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(.\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
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Security
Milestone

Comments

@Unrud
Copy link

Unrud commented Apr 21, 2022

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

gopherbot commented Apr 22, 2022

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

ianlancetaylor commented Apr 22, 2022

@gopherbot Please open backport issues.

Avoid confusion with ./C: paths.

@gopherbot
Copy link

gopherbot commented Apr 22, 2022

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
Copy link

gopherbot commented May 9, 2022

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

gopherbot commented May 9, 2022

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
…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
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Security
Projects
None yet
Development

No branches or pull requests

5 participants