net/http: ServeFile may be insufficiently hardened against misuse

[neild]

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]

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

[siadat]

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]

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

[ianwoolf]

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]

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 %? ?