/ go Public

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.

# path/filepath: mishandling of Windows UNC paths#56336

Closed
opened this issue Oct 19, 2022 · 14 comments
Closed

# path/filepath: mishandling of Windows UNC paths #56336

opened this issue Oct 19, 2022 · 14 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.

### neild commented Oct 19, 2022

 UNC paths with a . host are not recognized as UNC paths: filepath.Clean(\\.\C:\a) // \C:\a filepath.Clean(\\.\NUL) // \NUL filepath.Join(\\.\C:, a) // \C:\a  This causes Abs to produce incorrect results when cleaning the path of reserved device names: filepath.Abs(NUL) // \\NUL  Clean converts a path starting with \\ to one starting with only one \ if the path doesn't contain non-zero host and share components, and Join will drop the leading \\ from the first joined element: filepath.Clean(\\a\) // \a filepath.Join(\\a, b) // \a\b filepath.Join(\\a\, b) // \a\b  VolumeName does not normalize separator characters; this is inconsistent with most other filepath functions which Clean their results: filepath.VolumeName("//a/b/c") // //a/b  The proper handling of incomplete UNC paths is somewhat ambiguous, since \\a (for example) is not a valid Windows filename. Ideally, we'd return an error for invalid paths, but most of these functions do not return errors. I think the simplest behavior is to always preserve a leading \\ when present, so // Correct (IMO) behavior filepath.Clean(\\a) // \\a filepath.Join(\\, host, share) // \\host\share  @qmuntal The text was updated successfully, but these errors were encountered:
mentioned this issue Oct 19, 2022

### gopherbot commented Oct 19, 2022

 Change https://go.dev/cl/444280 mentions this issue: path/filepath: handle all paths starting with \\ as UNC on Windows

### qmuntal commented Oct 20, 2022 • edited

 Another mishandling: one can skip UNC path normalization by prefixing a UNC path (host\share) with \\?\UNC\, so filepath.VolumeName("\\?\UNC\a\b\c") should return \\?\UNC\a\b, not \\?\UNC. You can test this in the command line: echo 1234>\\?\UNC\localhost\c$\temp\foo.txt type \\?\UNC\localhost\c$\temp\foo.txt The same applies to the \\.\UNC\ prefix.

added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 20, 2022
closed this as completed in  be9d78c  Nov 9, 2022
mentioned this issue Nov 29, 2022

### francislavoie commented Feb 3, 2023

 This seems like a breaking change to me. At Caddy, we're trying to build on Go 1.20, but our tests are failing on Windows now. caddyserver/caddy#5353 We run this code: filepath.Clean("/"+input)  Before, on Go 1.19.5: In: /../bar | Out: \bar In: /../../ | Out: \  After, on Go 1.20.0: In: /../bar | Out: \\..\bar In: /../../ | Out: \\..\..\  This is a problem for us, because this would allow escaping the root. The reason we add "/" before cleaning is that the input is an HTTP request path, and we need to guarantee that the input didn't have the leading slash dropped by some code that came before it. We relied on Clean() to get rid of the duplicate leading slash if present. This doesn't seem to work anymore on Windows, with these changes. I figure we might be able to fix it by cleaning once, then prepending the slash and cleaning again. But that seems like a bad idea.

### martin-sucha commented Feb 3, 2023

 Using Clean alone is not sufficient to avoid escaping the root though. For example, Windows has reserved file names. See filepath.IsLocal

### francislavoie commented Feb 3, 2023

 IsLocal is not a useful check for this usecase because a URL path will always be absolute (start with a /) so that function will always return false. And also it's only available in 1.20 but we need to retain support for 1.18 at minimum for now (yes we could copy the source from stdlib but that's not a solution). The point is, this was a breaking change. It wasn't properly documented in the release notes as such, and is surprising behaviour.

### martin-sucha commented Feb 3, 2023

 Clean returns the shortest path name equivalent to path by purely lexical processing was not true for UNC paths before and now it is. The point is, this was a breaking change. It wasn't properly documented in the release notes as such, and is surprising behaviour. Yes, I agree with that. The documentation of filepath.Clean does not mention UNC paths at all and says that it will Replace multiple Separator elements with a single one. and Eliminate .. elements that begin a rooted path, which clearly does not happen for a UNC path at the start of the string.

### mholt commented Feb 3, 2023

 Can this issue be reopened?

### ianlancetaylor commented Feb 3, 2023

 @francislavoie It sounds like you are using path/filepath with a URL, which sounds odd to me. URLs are normally handled by path or by net/url. path/filepath has always been intended to work with paths in the file system. Since paths starting with \\ have a special meaning on Windows, if you use path/filepath with such a path it shouldn't change. The only room for adjustment I see here is that your path starts with // rather than \\. But my understanding is that on Windows paths starting with // are just like paths starting with \\. So you're correct that the behavior changed, but it seems to me that it became more correct. I'm sorry for the breakage, but from a compatibility point of view this we've always reserved the right to change behavior in order to fix cases where the code does not match its documentation. I agree that the release notes needs to be updated (ping @neild) but I'm not yet seeing an argument for rolling back this change. Such an argument would show us why the new behavior contradicts the documented behavior of the filepath functions.

### francislavoie commented Feb 3, 2023

 We're doing this to take a user input HTTP URL path (which happens to always be using / separators), and map it to a filesystem path (i.e. a file server). I'm pretty sure our usage is correct for this usecase. We're working at the intersection of those two path systems.

### bcmills commented Feb 3, 2023

 We're doing this to take a user input HTTP URL path (which happens to always be using / separators), and map it to a filesystem path (i.e. a file server). I'm pretty sure our usage is correct for this usecase. That turns out to be surprisingly subtle — see #57151.

### francislavoie commented Feb 3, 2023

 I agree, it's a very subtle problem. See our function to try to do a safe join of a configured "webroot" with an input URL, mapping to a filesystem path: https://github.com/caddyserver/caddy/blob/94b8d56096b2581d6739b057655e7b895c8fd3bb/modules/caddyhttp/caddyhttp.go#L223-L246 We'd gladly take a properly supported stdlib function to do this same operation in a smarter way.

### neild commented Feb 3, 2023

 Also before, on Go 1.19.5: In: /a/b/c | Out: \\a\b\c In: /a/b/../../c | Out: \\a\b\c  What's going on here is quite subtle. On Windows, filepath.Clean preserves the volume name when present. Volume names are given special handling so, for example, C:\..\a is cleaned to C:\a rather than \a. Previous versions of Go recognize paths such as \\server\share\a as having a volume name of \\server\share. However, they did not recognize the volume name in paths such as \\.\C:\a (a valid path with a volume name of \\.\C:) or \\?\UNC\server\share\a. Part of the pre-Go 1.20 behavior was to detect no volume name when the third character of the path is ".", and to truncate the volume name to just before the first "." following the first four characters of the path. So in Go 1.19.5: \\.\C:\a no volume name \\a.b\c\ volume name is \\a.b\c \\a\b.c\ volume name is \\a\b  This parsing behavior doesn't really correspond to the actual Windows path semantics, but it does have the property which is that a volume name, considered as a slash-separated string, will never contain a ".." component. Caddy contains this function: func SanitizedPathJoin(root, reqPath string) string { if root == "" { root = "." } path := filepath.Join(root, filepath.Clean("/"+reqPath)) if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 { path += separator } return path }  I do not believe filepath.Clean("/"+reqPath) is correct. reqPath in this case is a /-separated path from a URL. The filepath.Clean function operates on operating system paths, and therefore attempts to handle volume names within reqPath as well as interpreting both \ and / as separator characters. However, while incorrect, prior to Go 1.20 this function is (I think) safe from directory traversal attacks thanks to the fact that we never recognize .. as part of a volume name. It's not safe from surprising behaviors, though: SanitizedPathJoin("root", "/a/b/../../c") = root\a\b\c (not root\c as one might expect)  I think this function should be changed to use path.Clean instead: path := filepath.Join(root, path.Clean("/"+reqPath))  Note that we're still going from a /-separated path to an OS path here. We're still open to surprising behaviors when reqPath names a Windows device file, however. (Note that a/b/COM1 is still the COM1 device.) So better yet would be to use the new IsLocal function to further vet the path: relPath := path.Clean("/"+reqPath)[1:] // clean path and trim the leading / if !filepath.IsLocal(relPath) { return "", errors.New("path is unsafe") } path = filepath.Join(root, relPath)  If #57151 is accepted, we'll have a better approach to converting /-separated paths to OS paths: reqOSPath, err := filepath.FromFS(reqPath) if err != nil { return "", errors.New("path is unsafe") } // reqOSPath will not contain a volume name component or device name. path := filepath.Join(root, filepath.Clean("/" + reqOSPath))  All that notwithstanding, I think we can do a better job of defending against mistakes by changing Windows volume name detection to explicitly reject any volume name where a path component is "..". So far as I know, no valid Windows volume names contain ".." (unlike "."), and this will ensure that filepath.Clean(s) won't unexpectedly return a path containing .. components.

reopened this Feb 3, 2023

### francislavoie commented Feb 3, 2023

 I think this function should be changed to use path.Clean instead: path := filepath.Join(root, path.Clean("/"+reqPath))  Ah yeah, that definitely makes more sense. 🤦‍♂️ Thanks for pointing that out. So better yet would be to use the new IsLocal function to further vet the path: If #57151 is accepted, we'll have a better approach to converting /-separated paths to OS paths: Makes sense. We won't be able to change to minimum Go 1.20 for another 6 months to a year for packaging reasons, so that's not a solution we can use right now. Looking forwards to it, though.

added a commit to caddyserver/caddy that referenced this issue Feb 3, 2023
 Better fix for cleaning the path 
 bf78d60 
Recommendation from golang/go#56336 (comment)
mentioned this issue Feb 3, 2023
mentioned this issue Feb 5, 2023
mentioned this issue Feb 6, 2023
mentioned this issue Feb 9, 2023

### neild commented Feb 9, 2023

 Created a new issue #58451 to reject volume names containing a ".." path component, closing this issue in favor of that one.

closed this as completed Feb 9, 2023
mentioned this issue Feb 19, 2023
mentioned this issue Feb 28, 2023
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.
Projects
None yet
Development

No branches or pull requests

9 participants