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
Comments
/cc @bradfitz |
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? |
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. |
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 However |
@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 |
I was thinking about that today in the office. I think it most definitely should be done at the request level as 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 Maybe i'll write my first package for this to offer an alternative query method that cleans |
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. |
Change https://golang.org/cl/125875 mentions this issue: |
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. |
@bradfitz how about we export a function to sanitize |
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
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).
The text was updated successfully, but these errors were encountered: