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

Jump to bottom

zap.Open: Invalidate relative paths and paths with ".." segments #1397

Closed
wants to merge 1 commit into from
Closed

zap.Open: Invalidate relative paths and paths with ".." segments #1397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 24 additions & 0 deletions writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import (
"fmt"
"io"
"net/url"
"path/filepath"
"strings"

"go.uber.org/zap/zapcore"

Expand All @@ -48,6 +51,10 @@
// os.Stdout and os.Stderr. When specified without a scheme, relative file
// paths also work.
func Open(paths ...string) (zapcore.WriteSyncer, func(), error) {
if err := checkOpenPaths(paths); err != nil {
return nil, nil, err
}

writers, closeAll, err := open(paths)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -84,6 +91,23 @@
return writers, closeAll, nil
}

func checkOpenPaths(paths []string) error {
var openErr error
for _, path := range paths {
if !strings.HasPrefix(path, "file:") {
continue
}
Comment on lines +94 to +99
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just change the file:// sink instead?

This is how the sink is created based on scheme:

zap/sink.go

Lines 64 to 72 in 332853b

func newSinkRegistry() *sinkRegistry {
sr := &sinkRegistry{
factories: make(map[string]func(*url.URL) (Sink, error)),
openFile: os.OpenFile,
}
// Infallible operation: the registry is empty, so we can't have a conflict.
_ = sr.RegisterSink(schemeFile, sr.newFileSinkFromURL)
return sr
}

It registers the file:// sink by default.
So when a file:// URL is given, that calls newFileSinkFromURL,
which then calls newFileSinkFromPath:

zap/sink.go

Lines 150 to 159 in 332853b

func (sr *sinkRegistry) newFileSinkFromPath(path string) (Sink, error) {
switch path {
case "stdout":
return nopCloserSink{os.Stdout}, nil
case "stderr":
return nopCloserSink{os.Stderr}, nil
}
return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666)
}

That's the function that finally calls os.OpenFile (through a function reference).
That's where we should have the validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just experimented and yes this location allows for the reuse of a lot more code.

u, err := url.Parse(path)
if err != nil {
return err
}

Check warning on line 103 in writer.go

View check run for this annotation

Codecov / codecov/patch

writer.go#L102-L103 

Added lines #L102 - L103 were not covered by tests
if !(filepath.IsAbs(u.Path) && !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.

if filepath.IsAbs returns true, we're good. It's okay if an absolute path has /../ because that'll just modify the absolute path. /foo/bar/../baz is just /foo/baz.

Copy link
Contributor Author

@r-hang r-hang Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@abhinav, I think that means we don't need this feature then?

sr.openFile, given a rawUrl with a file scheme, always consumes an absolute path because url.Parse return object's Path field only contains an absolute path under this constraints. filepath.IsAbs will always return true here. The case where rawUrl isn't passed through url.Parse is also gated by a filepath.IsAbs.

Experimentation:
https://go.dev/play/p/w2VtVnkbIb8

Arbitrary prefix sequences of double dot segments like will be treated as if these sequences of double dot segments were not provided.

sink, _, err := zap.Open("file:///../../Users/rhang/gocode/src/x/zapopen/file") // acts like file:///Users/rhang/...

If that make sense I can close the issue after adding test to confirm this behavior?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@r-hang can we confirm with a test that it's not possible to provide a relative path or one with '..' to that API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1398 as a successor since I'm not able to reopen this one.

openErr = multierr.Append(openErr, fmt.Errorf(`file URI %q attempts a relative path or contains ".." dot segments`, path))
}
}
return openErr
}

// CombineWriteSyncers is a utility that combines multiple WriteSyncers into a
// single, locked WriteSyncer. If no inputs are supplied, it returns a no-op
// WriteSyncer.
Expand Down
40 changes: 40 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,46 @@ func TestOpenOtherErrors(t *testing.T) {
}
}

func TestOpenPathTraversalValidation(t *testing.T) {
tests := []struct {
msg string
paths []string
wantErr string
}{
{
msg: "invalid relative path root",
paths: []string{
"file:/../some/path",
"file:///../some/path",
},
wantErr: `file URI "file:/../some/path" attempts a relative path or contains ".." dot segments; file URI "file:///../some/path" attempts a relative path or contains ".." dot segments`,
},
{
msg: "invalid absolute path root with double dot segments",
paths: []string{
"file:/some/../../path",
"file://some/../../path",
"file:///some/../../path",
},
wantErr: `file URI "file:/some/../../path" attempts a relative path or contains ".." dot segments; file URI "file://some/../../path" attempts a relative path or contains ".." dot segments; file URI "file:///some/../../path" attempts a relative path or contains ".." dot segments`,
},
{
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`,
},
}

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