From 332853b7abcbe3e5019664cd0ca5d7fe628b213b Mon Sep 17 00:00:00 2001
From: Ryan Hang <rhang@uber.com>
Date: Sat, 16 Dec 2023 15:10:37 -0800
Subject: [PATCH] zap.Open: Invalidate relative paths and paths with ".."
 segments

Currently, zap.Open doesn't prevent someone from explicitly doing something
like zap.Open("file://../../../secret").

zap.Open already documents that paths passed to it must be absolute. Add
validation to error if zap.Open is called with a relative paths that could
write files outside of intended file directory hierarchy.

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

ref https://github.com/uber-go/zap/issues/1390
---
 writer.go      | 24 ++++++++++++++++++++++++
 writer_test.go | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/writer.go b/writer.go
index 06768c679..b30a9ada6 100644
--- a/writer.go
+++ b/writer.go
@@ -23,6 +23,9 @@ package zap
 import (
 	"fmt"
 	"io"
+	"net/url"
+	"path/filepath"
+	"strings"
 
 	"go.uber.org/zap/zapcore"
 
@@ -48,6 +51,10 @@ import (
 // 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
@@ -84,6 +91,23 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
 	return writers, closeAll, nil
 }
 
+func checkOpenPaths(paths []string) error {
+	var openErr error
+	for _, path := range paths {
+		if !strings.HasPrefix(path, "file:") {
+			continue
+		}
+		u, err := url.Parse(path)
+		if err != nil {
+			return err
+		}
+		if !(filepath.IsAbs(u.Path) && !strings.Contains(u.Path, "..")) {
+			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.
diff --git a/writer_test.go b/writer_test.go
index 20e00b74b..c18222d07 100644
--- a/writer_test.go
+++ b/writer_test.go
@@ -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