Thanks for the report, @candita!

I think this is a false positive.
The URL there is used to distinguish between different output sources in configuration.
That code path will be hit only if you use the URL file://... when configuring your logger.

This is where that sink gets registered:

(schemeFile is "file" here.)

So when someone specified zap.Open("file://path/to/file"), that'll hit this line:

Which will dispatch on the file:// to open enter newSinkFromURL, which will call the vulnerable line linked above.

The input to the line vulnerable line will not come from an HTTP external request.
If someone did zap.Open("http://../foo"), it would dispatch to a sink registered for the HTTP protocol, not the file sink.
So the only way to hit this with a request input is if someone specifically did: zap.Open("file://" + req.URL.Path).

Is there something we can do to help address this false positive?
We are willing to make changes to the code that'll address the false positive as long as it doesn't break the API or the core behavior.

I'm not well-versed in the exploitation of the vulnerability itself, but I think the vulnerability is more along the lines of gaining access to a file outside of the authorized hierarchy. The file path should be absolute, not relative, and not allowed to contain ...

Does it currently prevent someone from doing zap.Open("file://../../../yoursecret") ?

I see. I assumed the vulnerability was concerned with untrusted input from HTTP requests feeding into the path, and being able to access the filesystem.

No, the system doesn't currently prevent someone from explicitly doing zap.Open("file://../../../secret").
We can guard against that. In fact, looking at the documentation for zap.Open:

URLs with the "file" scheme must use absolute paths on the local filesystem.

This should already require that paths be absolute. I'm surprised that this isn't already checked.

CC @sywhang @r-hang

More info here: https://cwe.mitre.org/data/definitions/23.html with some newer exploits.

@abhinav @sywhang @r-hang just checking in to understand your plans on this.

Hey @candita, we plan to update the implementation of zap.Open to ensure "file:" prefix paths arguments are absolute or else we'll throw an error since that's already documented.

I should be able to put up a PR for this by Monday.

@candita After some discussion in #1398, we still think that this is a false positive.

The relevant function supports relative paths (zap.Open("relative/path/to/file")) and absolute paths (zap.Open("file://localhost/absolute/path/to/file")).
The absolute path will always start with / when we get it from url.Parse.
It's never joined into another path, so there's no risk of something like ../../../yoursecret turning into $HOME/yoursecret or similar. It'll be interpreted as /../../../yoursecret, which will become /yoursecret.

https://cwe.mitre.org/data/definitions/23.html seems concerned with relative path traversals. I couldn't find information about how an absolute path could be exploited there.