From ba41f97162bfc4ecbc3670bb311011a52fdf7954 Mon Sep 17 00:00:00 2001 From: Ryan Hang Date: Sat, 16 Dec 2023 15:10:37 -0800 Subject: [PATCH 1/3] zap.Open: Invalidate relative path roots 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 https://github.com/uber-go/zap/issues/1390 This PR succeeds https://github.com/uber-go/zap/pull/1397 --- sink.go | 5 +++ writer_test.go | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/sink.go b/sink.go index 499772a00..85ce697f7 100644 --- a/sink.go +++ b/sink.go @@ -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 } diff --git a/writer_test.go b/writer_test.go index 20e00b74b..2420d39e7 100644 --- a/writer_test.go +++ b/writer_test.go @@ -224,6 +224,92 @@ 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 { + msg string + paths []string + toWrite []byte + wantFileContents string + }{ + { + msg: "no hostname one double dot segment", + paths: []string{"file:/.." + tempName}, + toWrite: []byte("a"), + wantFileContents: "a", + }, + { + msg: "no hostname two double dot segments", + paths: []string{"file:/../.." + tempName}, + toWrite: []byte("b"), + wantFileContents: "ab", + }, + { + msg: "empty host name one double dot segment", + paths: []string{"file:///.." + tempName}, + toWrite: []byte("c"), + wantFileContents: "abc", + }, + { + msg: "empty hostname two double dot segments", + paths: []string{"file:///../.." + tempName}, + toWrite: []byte("d"), + wantFileContents: "abcd", + }, + } + + for _, tt := range tests { + t.Run(tt.msg, func(t *testing.T) { + 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 From a5ce90738220f0928e720fca71cae9a46343241b Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Wed, 20 Dec 2023 05:49:00 -0800 Subject: [PATCH 2/3] Sanitize in newFileSinkFromURL --- sink.go | 11 ++++++----- writer_test.go | 16 ++++++++-------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/sink.go b/sink.go index 85ce697f7..88b5ba704 100644 --- a/sink.go +++ b/sink.go @@ -104,12 +104,9 @@ func (sr *sinkRegistry) newSink(rawURL string) (Sink, error) { 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) - } - + // No scheme specified. Assume absolute or relative file path. if u.Scheme == "" { - u.Scheme = schemeFile + return sr.newFileSinkFromPath(rawURL) } sr.mu.Lock() @@ -150,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, "..") { + return nil, fmt.Errorf("file URLs must not contain '..': got %v", u) + } + return sr.newFileSinkFromPath(u.Path) } diff --git a/writer_test.go b/writer_test.go index 2420d39e7..f4ad616c2 100644 --- a/writer_test.go +++ b/writer_test.go @@ -231,20 +231,18 @@ func TestOpenRelativeValidated(t *testing.T) { wantErr string }{ { - msg: "invalid relative path root", + msg: "invalid double dot as the host element", paths: []string{ - "file:../some/path", + "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`, + wantErr: `open sink "file://../some/path": file URLs must leave host empty or use localhost: got file://../some/path`, }, { - msg: "invalid double dot as the host element", + msg: "dots not allowed", paths: []string{ - "file://../some/path", + "file:///../../../yoursecret", }, - wantErr: `open sink "file://../some/path": file URLs must leave host empty or use localhost: got file://../some/path`, + wantErr: `open sink "file:///../../../yoursecret": file URLs must not contain '..': got file:///../../../yoursecret`, }, } @@ -257,6 +255,8 @@ func TestOpenRelativeValidated(t *testing.T) { } func TestOpenDotSegmentsSanitized(t *testing.T) { + t.Skip("TODO") + tempName := filepath.Join(t.TempDir(), "test.log") assert.False(t, fileExists(tempName)) require.True(t, filepath.IsAbs(tempName), "Expected absolute temp file path.") From 582d335d10ded2d2d933aae5de9450fc36c1042b Mon Sep 17 00:00:00 2001 From: Ryan Hang Date: Thu, 21 Dec 2023 18:48:53 -0800 Subject: [PATCH 3/3] remove sanitization test --- writer_test.go | 60 ++------------------------------------------------ 1 file changed, 2 insertions(+), 58 deletions(-) diff --git a/writer_test.go b/writer_test.go index f4ad616c2..4f1d81f17 100644 --- a/writer_test.go +++ b/writer_test.go @@ -224,7 +224,7 @@ func TestOpenOtherErrors(t *testing.T) { } } -func TestOpenRelativeValidated(t *testing.T) { +func TestOpenDoubleDotSegmentsDisallowed(t *testing.T) { tests := []struct { msg string paths []string @@ -238,7 +238,7 @@ func TestOpenRelativeValidated(t *testing.T) { wantErr: `open sink "file://../some/path": file URLs must leave host empty or use localhost: got file://../some/path`, }, { - msg: "dots not allowed", + msg: "double dot segments not allowed", paths: []string{ "file:///../../../yoursecret", }, @@ -254,62 +254,6 @@ func TestOpenRelativeValidated(t *testing.T) { } } -func TestOpenDotSegmentsSanitized(t *testing.T) { - t.Skip("TODO") - - 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 { - msg string - paths []string - toWrite []byte - wantFileContents string - }{ - { - msg: "no hostname one double dot segment", - paths: []string{"file:/.." + tempName}, - toWrite: []byte("a"), - wantFileContents: "a", - }, - { - msg: "no hostname two double dot segments", - paths: []string{"file:/../.." + tempName}, - toWrite: []byte("b"), - wantFileContents: "ab", - }, - { - msg: "empty host name one double dot segment", - paths: []string{"file:///.." + tempName}, - toWrite: []byte("c"), - wantFileContents: "abc", - }, - { - msg: "empty hostname two double dot segments", - paths: []string{"file:///../.." + tempName}, - toWrite: []byte("d"), - wantFileContents: "abcd", - }, - } - - for _, tt := range tests { - t.Run(tt.msg, func(t *testing.T) { - 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