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

zap.Open: Invalidate relative path roots #1398

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

r-hang
Copy link
Contributor

@r-hang r-hang commented Dec 19, 2023

Add validation to ensure file schema paths passed to zap.Open are absolute since this is already documented.

Tests are also added to demonstrate that double dot segements within file schema URIs passed to zap.Open remain within the specified file hierarchy.

This change addresses https://cwe.mitre.org/data/definitions/23.html

ref #1390

This PR succeeds #1397

Add validation to ensure file schema paths passed to zap.Open are
absolute since this is already documented.

Curently, file schema URIs with relative roots e.g. "file://../"
are parsed to "" by url.Parse and lead to errors when opening a "" path.
Additional validation makes this problem easier to correct.

Tests are also added to demonstrate that double dot segements within
file schema URIs passed to zap.Open remaining within the specified file
hierarchy.

This change addresses https://cwe.mitre.org/data/definitions/23.html

ref #1390

This PR succeeds #1397
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d27427d) 98.28% compared to head (582d335) 98.42%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1398      +/-   ##
==========================================
+ Coverage   98.28%   98.42%   +0.14%     
==========================================
  Files          53       53              
  Lines        3495     3498       +3     
==========================================
+ Hits         3435     3443       +8     
+ Misses         50       46       -4     
+ Partials       10        9       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abhinav
Copy link
Collaborator

abhinav commented Dec 20, 2023

Okay, so I had a chance to go over the issue again and the url.Parse behavior you mentioned.
While it's correct that .Path from url.Parse will never give us a relative path, #1390 specifically wants to stop us from allowing .. in that path.

I see that you added the check to newSink instead of newFileSinkFromURL because we still want zap.Open to support relative paths when file:// is not specified.
This should've been explained in a comment there, ideally, but I think it might be better if we just diverge those two code paths: newSink should call newFileSinkFromURL if a scheme was specified, and newFileSinkFromPath directly if not. That keeps this simple.

I've pushed a commit that does that but:

  • I've removed some invalid file:foo/bar tests -- that's not a valid URL.
  • I didn't understand the purpose of the TestOpenRelativeValidated test, so I've skipped it for now. I'll let you fix/remove it based on what you were trying to do there.

@r-hang
Copy link
Contributor Author

r-hang commented Dec 22, 2023

@abhinav by TestOpenRelativeValidated, I think that's TestOpenDotSegmentsSanitized because as that is the one that you skipped.

This test is no longer required if we are updating the validation to reject all double dot segments from file schema paths. I've deleted that test wholesale given your updates to the validation to make it reject double dot segments.

My original intention was to use that test to demonstrate that zap.Open would still function in a safe way even when provided with double dot segments in the path (this was because url.Parse only spits out absolute paths as mentioned above).

@@ -145,6 +147,10 @@ func (sr *sinkRegistry) newFileSinkFromURL(u *url.URL) (Sink, error) {
return nil, fmt.Errorf("file URLs must leave host empty or use localhost: got %v", u)
}

if strings.Contains(u.Path, "..") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A filename can contain .., so I don't think this check is right

$ vim foo..bar

$ cat foo..bar
hello

should we instead look for an absolute path before calling newFileSinkFromPath?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newFileSinkFromPath must support relative paths. Paths without the file:// form are allowed to be relative. This is documented and tested already.

Paths that take the file:// form will use net/url, and the path will always be absolute (because u.Path will always start with / for them).

The ask was to specifically reject path traversals with the URLs. We could check for /../ instead.

TBH, I'm not convinced that there's a vulnerability here, but I'll believe that a security expert could use some combination of symlinks and .. to get arbitrary file access.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path traversal security issue may be a false positive, since we have the hostname check above(file://foo/bar parses to host=foo, path=/bar).

Stepping back, what is the security vulnerability exactly? If the user has control over the paths, they can specify relative paths like ../foo which is a valid supported path -- does it matter if it's via URL or a relative path?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said, I'm not convinced that there's a vulnerability here.
But you're right, given that the user has full control of this input, this is even less a question of sanitization.
If we want to appease the linter, we can add a filepath.Clean there, but the actual check may be unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented in #1390 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants