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.

Sign up for GitHub

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
Open

zap.Open: Invalidate relative path roots #1398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba41f97
zap.Open: Invalidate relative path roots
r-hang Dec 16, 2023
a5ce907
Sanitize in newFileSinkFromURL
abhinav Dec 20, 2023
582d335
remove sanitization test
r-hang Dec 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion sink.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,10 @@ func (sr *sinkRegistry) newSink(rawURL string) (Sink, error) {
if err != nil {
return nil, fmt.Errorf("can't parse %q as a URL: %v", rawURL, err)
}

// No scheme specified. Assume absolute or relative file path.
if u.Scheme == "" {
u.Scheme = schemeFile
return sr.newFileSinkFromPath(rawURL)
}

sr.mu.Lock()
Expand Down Expand Up @@ -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)

return nil, fmt.Errorf("file URLs must not contain '..': got %v", u)
}

return sr.newFileSinkFromPath(u.Path)
}

Expand Down
30 changes: 30 additions & 0 deletions writer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,36 @@ func TestOpenOtherErrors(t *testing.T) {
}
}

func TestOpenDoubleDotSegmentsDisallowed(t *testing.T) {
tests := []struct {
msg string
paths []string
wantErr string
}{
{
msg: "invalid double dot as the host element",
paths: []string{
"file://../some/path",
},
wantErr: `open sink "file://../some/path": file URLs must leave host empty or use localhost: got file://../some/path`,
},
{
msg: "double dot segments not allowed",
paths: []string{
"file:///../../../yoursecret",
},
wantErr: `open sink "file:///../../../yoursecret": file URLs must not contain '..': got file:///../../../yoursecret`,
},
}

for _, tt := range tests {
t.Run(tt.msg, func(t *testing.T) {
_, _, err := Open(tt.paths...)
assert.EqualError(t, err, tt.wantErr)
})
}
}

type testWriter struct {
expected string
t testing.TB
Expand Down