update newBinaryCmd URL path handling (microsoft#2041)

anmaxvl · kiashok · commit fe8c673755df · 2024-03-04T16:47:34.000-08:00
Signed-off-by: Maksim An &lt;maksiman@microsoft.com&gt;
diff --git a/internal/cmd/io_binary.go b/internal/cmd/io_binary.go
@@ -28,6 +28,8 @@ const (
 	binaryCmdStartTimeout = 10 * time.Second
 )
 
+var ErrUnsafePath = errors.New("path is unsafe")
+
 // NewBinaryIO runs a custom binary process for pluggable shim logging driver.
 //
 // Container's IO will be redirected to the logging driver via named pipes, which are
@@ -122,14 +124,19 @@ func NewBinaryIO(ctx context.Context, id string, uri *url.URL) (_ UpstreamIO, er
 }
 
 // sanitizePath parses the URL object and returns a clean path to the logging driver
-func sanitizePath(uri *url.URL) string {
+func sanitizePath(uri *url.URL) (string, error) {
 	path := filepath.Clean(uri.Path)
 
+	// avoid UNC paths (e.g. `\\server\share\`)
+	if strings.HasPrefix(path, `\\`) {
+		return "", ErrUnsafePath
+	}
+
 	if strings.Contains(path, `:\`) {
-		return strings.TrimPrefix(path, "\\")
+		return strings.TrimPrefix(path, "\\"), nil
 	}
 
-	return path
+	return path, nil
 }
 
 func newBinaryCmd(ctx context.Context, uri *url.URL, envs []string) (*exec.Cmd, error) {
@@ -145,7 +152,10 @@ func newBinaryCmd(ctx context.Context, uri *url.URL, envs []string) (*exec.Cmd,
 		}
 	}
 
-	execPath := sanitizePath(uri)
+	execPath, err := sanitizePath(uri)
+	if err != nil {
+		return nil, err
+	}
 
 	cmd := exec.CommandContext(ctx, execPath, args...)
 	cmd.Env = append(cmd.Env, envs...)
diff --git a/internal/cmd/io_binary_test.go b/internal/cmd/io_binary_test.go
@@ -4,6 +4,7 @@ package cmd
 
 import (
 	"context"
+	"errors"
 	"net/url"
 	"testing"
 )
@@ -24,11 +25,6 @@ func Test_newBinaryCmd_Key_Value_Pair(t *testing.T) {
 			urlString: "binary:///executable?-key=value",
 			expected:  `\executable -key value`,
 		},
-		{
-			name:      "Path_With_Back_Slashes",
-			urlString: `binary:///\executable?-key=value`,
-			expected:  `\executable -key value`,
-		},
 		{
 			name:      "Clean_Path_With_Dots_And_Multiple_Fwd_Slashes",
 			urlString: "binary:///../path/to///to/../executable",
@@ -70,6 +66,45 @@ func Test_newBinaryCmd_Key_Value_Pair(t *testing.T) {
 	}
 }
 
+func Test_newBinaryCmd_Unsafe_Path(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	type config struct {
+		name          string
+		urlString     string
+		expectedError error
+	}
+
+	for _, cfg := range []*config{
+		{
+			name:          "UNC_Path_With_Back_Slashes",
+			urlString:     `binary:///\server\share\executable`,
+			expectedError: ErrUnsafePath,
+		},
+		{
+			name:          "UNC_Path_With_Forward_Slashes",
+			urlString:     `binary:////server/share/executable`,
+			expectedError: ErrUnsafePath,
+		},
+	} {
+		t.Run(cfg.name, func(t *testing.T) {
+			u, err := url.Parse(cfg.urlString)
+			if err != nil {
+				t.Fatalf("failed to parse url: %s", cfg.urlString)
+			}
+
+			_, err = newBinaryCmd(ctx, u, nil)
+			if err == nil {
+				t.Fatalf("no error was returned")
+			}
+			if !errors.Is(err, cfg.expectedError) {
+				t.Fatalf("expected error: %s, actual: %s", cfg.expectedError, err)
+			}
+		})
+	}
+}
+
 func Test_newBinaryCmd_Empty_Path(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
