Vulnerability scanning shows unsanitized input, path traversal issue in zap

[candita]

Describe the bug
Snyk code vulnerability scanner was run on vendored uber-go code and found an issue:

Error: SNYK_CODE_WARNING (CWE-23):
vendor/go.uber.org/zap/sink.go:139:9: error[go/PT]: Unsanitized input from the request URL flows into os.OpenFile, where it is used as a path. This may result in a Path Traversal vulnerability and allow an attacker to open arbitrary files.

This is an old line number reference, but the issue is still visible in the repo head, at https://github.com/uber-go/zap/blob/master/sink.go#L158C33-L158C33.

To Reproduce
Run any scanner that picks up Path Traversal or CWE vulnerabilities. CWE-23 will appear and reference https://github.com/uber-go/zap/blob/master/sink.go#L158C33-L158C33.

Expected behavior
No vulnerabilities seen.

Additional context
CWE-23 is in the top 25 Common Weaknesses. Zap "... uses external input to construct a pathname that should be within a restricted directory, but it does not properly neutralize sequences such as ".." that can resolve to a location that is outside of that directory. This allows attackers to traverse the file system to access files or directories that are outside of the restricted directory."

Thanks for your attention.

[abhinav]

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:

zap/sink.go

Line 70 in 5acd569

_ = sr.RegisterSink(schemeFile, sr.newFileSinkFromURL)

(schemeFile is "file" here.)

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

zap/writer.go

Line 71 in 5acd569

sink, err := _sinkRegistry.newSink(path)

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.

[candita]

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") ?

[abhinav]

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

[candita]

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

[candita]

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

[r-hang]

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.

[abhinav]

@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.