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: ServeFile may be insufficiently hardened against misuse #53946

Open
neild opened this issue Jul 18, 2022 · 5 comments
Open

net/http: ServeFile may be insufficiently hardened against misuse #53946

neild opened this issue Jul 18, 2022 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@neild
Copy link
Contributor

neild commented Jul 18, 2022

The http.ServeFile function replies to a request with the contents of a named file on the local filesystem.

As a hardening measure, ServeFile attempts to guard against cases where it is called with an untrusted input as the source filename. In particular, ServeFile rejects requests where r.URL.Path contains a .. path element to protect callers who might use r.URL.Path as a component of the filename.

This does not protect users who use a query parameter to construct the filename.

Perhaps we could additionally harden ServeFile by checking for .. elements anywhere in the request URI, including query parameters. (What about form values? Would we check them too?)

Or perhaps there are some other measures we could take to harden ServeFile against misuse.

@rsc
Copy link
Contributor

rsc commented Jul 19, 2022

We could potentially reject names containing ? and % in ServeFile, as evidence of passing an unsanitized, unescaped RequestURI through.

@toothrot toothrot added this to the Backlog milestone Jul 20, 2022
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 20, 2022
@siadat
Copy link
Contributor

siadat commented Jul 28, 2022

Would it be a good idea if we reject all requested files that are not within the current working directory tree?

If yes, a potential solution could look like this:

 func ServeFile(w ResponseWriter, r *Request, name string) {
-       if containsDotDot(r.URL.Path) {
-               // Too many programs use r.URL.Path to construct the argument to
-               // serveFile. Reject the request under the assumption that happened
-               // here and ".." may not be wanted.
-               // Note that name might not contain "..", for example if code (still
-               // incorrectly) used filepath.Join(myDir, r.URL.Path).
-               Error(w, "invalid URL path", StatusBadRequest)
-               return
-       }
        dir, file := filepath.Split(name)
+       wd, err := os.Getwd()
+       if err != nil {
+               Error(w, "cannot determine current directory", StatusInternalServerError)
+               return
+       }
+       dir := filepath.Join(wd, filepath.FromSlash(path.Clean("/"+dir)))
        serveFile(w, r, Dir(dir), file, false)
 }

i.e., the given name is split into dir and file, clean dir, file is already cleaned in http.Dir#Open.

@gopherbot
Copy link

Change https://go.dev/cl/420414 mentions this issue: net/http: [wip] check dir if contains DotDot in ServeFile

@ianwoolf
Copy link
Contributor

ianwoolf commented Jul 31, 2022

We could potentially reject names containing ? and % in ServeFile, as evidence of passing an unsanitized, unescaped RequestURI through.

i'm working on it.

But I'm not quite sure. Does it mean we should check and reject the case if u.Path and u.RawQuery inclusion ? and %?

@ianwoolf
Copy link
Contributor

i send a cl, just check containsDotDot for the dir from name to check for .. elements anywhere in the request.

As for checking the unescaped param, is it appropriate to call url.shouldEscape(c, url.encodeQueryComponent)? or just check if the name contains %? ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants