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 return 404 on temporary FS errors (such as »too many open files«) #46394

Open
Xjs opened this issue May 26, 2021 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Xjs
Copy link
Contributor

Xjs commented May 26, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16.4 darwin/arm64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="arm64"
GOOS="darwin"

(for completeness's sake, I first observed it on a linux server running a go1.14 binary)

What did you do?

Compile the following program into an executable (say http-ulimit-404 by doing go build -o http-ulimit-404 http-ulimit-404.go).

package main

import (
	"net/http"
	"path/filepath"
)

func main() {
	http.ListenAndServe(":8080", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		http.ServeFile(w, r, filepath.Join(".", r.URL.Path))
	}))
}

Set the number of open files very low using e. g. ulimit -n 8 (I found that on Darwin, 8 works well to reproduce the issue. Note that on Darwin this only works as root user.)

Request a file that exists in the working directory, if necessary, using multiple parallel instances of curl, e. g. curl http://localhost:8080/http-ulimit-404.go. For me, using one single curl instance with ulimit -n 8 works already.

Observe the resulting error to be 404.

What did you expect to see?

Package net/http returning a 5xx error indicating to clients that the request cannot be served due to a temporary condition (in this case number of open files), and may be retryable.

What did you see instead?

Package net/http returning a 404 error, indicating to clients that the request cannot be served due to a permanent condition, possibly causing cache posioning or other downstream failures.

(In the real world scenario where I found this, this caused a 404 on an extant file due to a high number of concurrent requests, leading to this file not being processed – and not retried – in a workload where it should've been.)

I believe this could easily be mitigated by checking if originalErr implements interface { Temporary() bool } in mapDirOpenError (https://golang.org/src/net/http/fs.go#L48), but I'm not 100% sure yet if I'm overlooking something.

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 26, 2021
@seankhliao
Copy link
Member

cc @fraenkel @neild @empijei @bradfitz

@neild
Copy link
Contributor

neild commented May 26, 2021

There are several questions here:

Should http.ServeFile and http.FileServer serve a 5xx error for potentially-transient errors in general, and ENFILE specifically? This seems like a reasonable feature request to me.

What errors should be transient by default? ENFILE and EMFILE are obvious candidates, and the other syscall errors for which we return a "temporary" error (see #45729 for an exhaustive list) probably make sense.

Most interestingly, how should we communicate to the http package that an error should be served as a 5xx? The Temporary status as implemented by interface { Temporary() bool } is not well-defined and is being deprecated--see #45729. So I don't think we should add additional uses of this method.

One possibility is to add a sentinel error to io/fs representing errors which can be resolved by retrying a call, such as ErrTemporary, ErrTransient, or ErrRetryable.

@Xjs
Copy link
Contributor Author

Xjs commented May 27, 2021

Since Temporary is going to be deprecated, since mapDirOpenError already queries specific file system errors (using os.IsNotExist and os.IsPermission), we could just extend the list with concrete errors. However, neither package os nor package io/fs expose a platform-independent »too many open files« error. This might be a sensible addition.

@empijei
Copy link
Contributor

empijei commented May 27, 2021

Drive-by security comment:

The fact that ServeFile already returns different codes depending on the underlying condition that caused a file to not be accessible is already bad.
Adding yet another information leak that allows potential attackers to discover file handlers/descriptors limits and other generic temporary errors would be a bit too much in my opinion.

When working on Go SafeWeb we had to write quite a bit of code to try and remove the current behavior that leaks information on existing files that are not accessible by the user who runs the server process.

ServeFile and FileServer are, by nature, exposed to attackers and, as such, they shouldn't have such a deliberate information leak by default.

If anything, I'd argue any error encountered by those functions should cause a 404 to be emitted. If users want to expose their server's internals they should have to write custom code themselves.

I opened #46413 for reference.

@Xjs
Copy link
Contributor Author

Xjs commented May 27, 2021

In my concrete case, ServeFile is only exposed to internal services, which would benefit from a reliability perspective if they could use standard procedure (assume 4xx errors are client errors, retry 5xx errors) on the ServeFile service. As I noted in #46413, maybe there could be variants to http.Dir that do or do not expose this information.

As far as I'm aware, ServeFile and FileServer are often used to serve content that doesn't live in a filesystem on a physical disk anyway. Arguably, it may be worth while for these services to be able to differentiate between server-side and client-side errors, and different io.FS implementations may have their own individual security concerns that cannot be covered in a catch-all by package http.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
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

4 participants