Skip to content

Commit

Permalink
update newBinaryCmd URL path handling (#2041)
Browse files Browse the repository at this point in the history
Signed-off-by: Maksim An <maksiman@microsoft.com>
  • Loading branch information
anmaxvl authored and kiashok committed Mar 5, 2024
1 parent 85086d7 commit fe8c673
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 9 deletions.
18 changes: 14 additions & 4 deletions internal/cmd/io_binary.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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...)
Expand Down
45 changes: 40 additions & 5 deletions internal/cmd/io_binary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package cmd

import (
"context"
"errors"
"net/url"
"testing"
)
Expand All @@ -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",
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit fe8c673

Please sign in to comment.