net/http: ServeFile and FileServer information leak

[empijei]

ServeFile and FileServer are used to serve static files.

These files have to exists on the server or the two functions will return a 404.

The issue is that both function don't just return 404 by default, but they try to return an error that gives information on the server's local filesystem:

go/src/net/http/fs.go

Lines 597 to 610 in 3075ffc

	f, err := fs.Open(name)
	if err != nil {
	msg, code := toHTTPError(err)
	Error(w, msg, code)
	return
	}
	defer f.Close()
	d, err := f.Stat()
	if err != nil {
	msg, code := toHTTPError(err)
	Error(w, msg, code)
	return
	}

This is the current implementation of toHTTPError:

go/src/net/http/fs.go

Lines 672 to 681 in 3075ffc

	func toHTTPError(err error) (msg string, httpStatus int) {
	if errors.Is(err, fs.ErrNotExist) {
	return "404 page not found", StatusNotFound
	}
	if errors.Is(err, fs.ErrPermission) {
	return "403 Forbidden", StatusForbidden
	}
	// Default:
	return "500 Internal Server Error", StatusInternalServerError
	}

This means that an attacker can probe the server's filesystem and discover potentially existing files that the server can't access, or otherwise infer information on the current directory tree (even if those paths are not accessible by the user running the server).

I propose we change this behavior to make all errors returned by serveFile be a 404.

Note that there already are security-sensitive implementations out there trying to get around this kind of behavior. It would be nice if people didn't have to wrap the response writer in order to avoid introducing OWASP TESTING GUIDE-4.8.1: Improper error handling in their code.

/cc @FiloSottile @katiehockman @rolandshoemaker @bradfitz @neild

[Xjs]

Be aware that the http.Dir filesystem already has a measure in place (mapDirOpenError) which attempts to obscure some of the information that can be gained through this attack path. It might be worth while to consider whether it is ServeFile's or a FS's job to avoid this kind of information leak.

[empijei]

fs.FS can't know if the information it gives is gonna be exposed to attackers, while ServeFile does.

ServeFile will always be exposed and it could be called by user code after parsing a request parameter, so it doesn't necessarily mean it comes from a http.Dir call.

I think the right place to put this protection in is as close as possible to where the error is generated.

[mknyszek]

CC @neild

[neild]

I am not convinced that ServeFile should return a 404 for error conditions other then ErrNotExist and maybe ErrPermission. There is a meaningful distinction between a client requesting a resource that does not exist and the server encountering a fault that prevented it from serving the request. The cited guide on improper error handling argues that error responses should not contain "stack traces, database dumps, and error codes" and should be "a specifically designed result that is helpful to the user without revealing unnecessary internal details". This is the case for ServeFile today: All errors are reported to the client as one of "404 page not found", "403 forbidden", or "500 Internal Server Error". Additional error detail is discarded.

I think there's a good argument that ServeFile should not return 403 Forbidden for ErrPermission errors. A 403 indicates that the remote endpoint is not permitted to access a URL. ErrPermission indicates that the server process is not permitted to access a file. These are different concepts.

I am dubious, however, about the concern that returning a 403 Forbidden leaks information about the local filesystem. ServeFile inherently provides information about the filesystem.

[empijei]

I think that not returning a 403 for ErrPermission would already be a win.

That said, if an attacker can probe the filesystem and find out which files exist based on the server returning 404 or 500 this would still be an issue, but we might decide to accept the risk for the sake of usability.