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

Jump to bottom

net/http: ServeFile and FileServer information leak #46413

Open
empijei opened this issue May 27, 2021 · 5 comments
Open

net/http: ServeFile and FileServer information leak #46413

empijei opened this issue May 27, 2021 · 5 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@empijei
Copy link
Contributor

empijei commented May 27, 2021

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
@empijei empijei mentioned this issue May 27, 2021
Open
@Xjs
Copy link
Contributor

Xjs commented May 27, 2021

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
Copy link
Contributor Author

empijei commented May 27, 2021

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 mknyszek added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 27, 2021
@mknyszek mknyszek added this to the Backlog milestone May 27, 2021
@mknyszek
Copy link
Contributor

CC @neild

@neild
Copy link
Contributor

neild commented May 27, 2021

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
Copy link
Contributor Author

empijei commented Jun 11, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@neild @Xjs @mknyszek @empijei