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

net/http: document filepath sanitization for ServeFile #18837

Closed
FlorianUekermann opened this issue Jan 29, 2017 · 10 comments

Comments

Projects
None yet
8 participants
@FlorianUekermann
Copy link
Contributor

commented Jan 29, 2017

This is a petty formulation issue, but the subject is quite sensitive so I thought I would bring it up. The documentation for http.ServeFile(w ResponseWriter, r *Request, name string) states that name needs to be sanitized to prevent ascension into parent directories. It further states that ServeFile rejects requests that contain ".." as path elements.

I assume the implication of both statements combined is that patterns like

http.ServeFile(w, r, filepath.Join(".", r.URL.Path))
http.ServeFile(w, r, filepath.Join("dir", r.URL.Path))

are "safe" in the sense that ascension into the respective parent directories is not possible.
However this is not obvious from the current documentation as there may more obscure ways of ascending than ".." that are less obvious (I am honestly not sure).

Imo it would be a good idea to state explicitly whether the file path in the above patterns is sufficiently sanitized or not because I see only two scenarios:
-If the patterns above are not safe the current documentation may cause security problems
-If the patterns are safe some users won't take advantage of this great function because it is not explicitly stated (this is me right now).

@odeke-em odeke-em changed the title http: documentation for ServeFile net/http: document filepath sanitization for ServeFile Jan 29, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jan 29, 2017

/cc @bradfitz

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

Sorry, I lost this bug.

r.URL.Path can arrive with ".." in it, so it's not safe to use with filepath.Join directly to ServeFile.

Consider: https://play.golang.org/p/5GlPtcESJm

I suppose we could document that r.URL.Path can contain "..". Would that be sufficient?

@bradfitz bradfitz added this to the Go1.9 milestone Mar 21, 2017

@FlorianUekermann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2017

ServeFile prevents your attack by checking the URL.Path. See: https://play.golang.org/p/kVXs8aOBNz

Note that in the second example the return value of Join doesn't contain any "..", since Join "cleans that up", but ServeFile checks the URL.Path directly. It seems to me that ascension to parent directories can't be caused by the URL, which would make the pattern I used above safe.

@broady broady modified the milestones: Go1.10, Go1.9 Jul 17, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018

@Hawkinsonb

This comment has been minimized.

Copy link

commented Jul 10, 2018

Also note that this vulnerability is even more present when using templates. Our Example: a path from some predetermined directory is passed through the URL via ?path=/in/side/directory. parsed with URL.Query(), into some function to return the contents of our file. Our options to read the contents of this would be ioutil.ReadFile(filepath.Join(directoryPath, filePathFromDirectory)) or ioutil.ReadFile(directoryPath + "/" + filePathFromDirectory).

However ioutil.ReadFile(filepath.Join(directoryPath, path)) with the query ?path=../../../etc/passwd grabs the contents of the passwd file and outputs it into the template leaving the host vulnerable. We're using directoryPath + "/" + filePathFromDirectory to come up with our path as it's not exposed, but this is not "proper" way to handle this. This is without using ServeFile where the current layer of security lies, shouldn't there be some layer of protection at the *http.Request level?

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

@Hawkinsonb In your specific example, I don't think there would be an easy to protect against that automatically. the net/http library has no way of checking whether a query parameter is a path or not.

We should have a exported function to sanitize paths from untrusted sources somewhere. It's pretty much just ensuring the path is rooted and then path.Cleaning it.

I can see the argument that it should consider requests where the request path itself contains .. invalid. That is something we should do too. Not sure how that would be exposed to users of net/http though. A callback in net/http seems simplest but wouldn't be backwards compatible. We might not be able to do this because of the compatibility guarantee actually. Though, I think the security gains definitely outweigh the compatibility guarantee. I cannot imagine any genuinely useful code that isn't buggy or insecure that relies on the path containing .. elements.

@Hawkinsonb

This comment has been minimized.

Copy link

commented Jul 11, 2018

I was thinking about that today in the office. I think it most definitely should be done at the request level as filepath.Join() needs to maintain its functionality to allow the environment to navigate the file structure cleanly. Thankfully most filepaths look the same, and I can't imagine case where ../ would be a particularly useful value from a request. If it is I think that program has some pretty big security holes in its design and should be discouraged by the net/http library.

Curious as what you mean that the net/http library doesn't have a way to check if the query parameter is a path or not? At some point the URL values are parsed and returned in request.URL.Query() right? While i'm no expert in Golang design standards so please feel free to ignore this recommendation in regards to backwards compatibility: deprecate Query() and add an additional func SecureQuery() that sanitizes the vals?

Maybe i'll write my first package for this to offer an alternative query method that cleans URL.Query() :)

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2018

Curious as what you mean that the net/http library doesn't have a way to check if the query parameter is a path or not? At some point the URL values are parsed and returned in request.URL.Query() right? While i'm no expert in Golang design standards so please feel free to ignore this recommendation in regards to backwards compatibility: deprecate Query() and add an additional func SecureQuery() that sanitizes the vals?

The url query params can contain arbitrary data. Paths or whatever. It'd be a big breaking change to change this behaviour and very unexpected imo.

Such sanitization if decided upon should only apply to http.Request.URL.Path.

@gopherbot

This comment has been minimized.

Copy link

commented Jul 25, 2018

Change https://golang.org/cl/125875 mentions this issue: net/http: try to document ServeFile security more

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 25, 2018

I'm not exactly what else to document, but I took a stab at it in https://go-review.googlesource.com/c/go/+/125875 ... is that sufficient?

Reviewers welcome.

@gopherbot gopherbot closed this in 902fc11 Jul 27, 2018

@nhooyr

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

@bradfitz how about we export a function to sanitize r.URL.Path? Right now its not clear how exactly to safely do that. As far as I can tell, its as simple as a call to path.Clean() and then making sure we add back a trailing slash if there was one. This is safe because the path is always rooted when it comes out of *http.Server so path.Clean() will remove all .. elements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.