Skip to content

Commit

Permalink
zap.Open: Invalidate relative path roots
Browse files Browse the repository at this point in the history
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 remaining within the specified file
hierarchy.

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

ref #1390
  • Loading branch information
r-hang committed Dec 19, 2023
1 parent d27427d commit 3436595
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 0 deletions.
5 changes: 5 additions & 0 deletions sink.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ 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)
}

if u.Scheme == schemeFile && !filepath.IsAbs(u.Path) {
return nil, fmt.Errorf("file URI %q attempts a relative path", rawURL)
}

if u.Scheme == "" {
u.Scheme = schemeFile
}
Expand Down
79 changes: 79 additions & 0 deletions writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,85 @@ func TestOpenOtherErrors(t *testing.T) {
}
}

func TestOpenRelativeValidated(t *testing.T) {
tests := []struct {
msg string
paths []string
wantErr string
}{
{
msg: "invalid relative path root",
paths: []string{
"file:../some/path",
},
// url.Parse's Path for this path value is "" which would result
// in a file not found error if not validated.
wantErr: `open sink "file:../some/path": file URI "file:../some/path" attempts a relative path`,
},
{
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)
})
}
}

func TestOpenDotSegmentsSanitized(t *testing.T) {
tempName := filepath.Join(t.TempDir(), "test.log")
assert.False(t, fileExists(tempName))
require.True(t, filepath.IsAbs(tempName), "Expected absolute temp file path.")

tests := []struct {
paths []string
toWrite []byte
wantFileContents string
}{
{
paths: []string{"file:/.." + tempName},
toWrite: []byte("a"),
wantFileContents: "a",
},
{
paths: []string{"file:/../.." + tempName},
toWrite: []byte("b"),
wantFileContents: "ab",
},
{
paths: []string{"file:///.." + tempName},
toWrite: []byte("c"),
wantFileContents: "abc",
},
{
paths: []string{"file:///../.." + tempName},
toWrite: []byte("d"),
wantFileContents: "abcd",
},
}

for _, tt := range tests {
ws, cleanup, err := Open(tt.paths...)
require.NoError(t, err)
defer cleanup()

_, err = ws.Write(tt.toWrite)
require.NoError(t, err)

b, err := os.ReadFile(tempName)
require.NoError(t, err)

assert.Equal(t, string(b), tt.wantFileContents)
}
}

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

0 comments on commit 3436595

Please sign in to comment.