Addressed PR feedback

Signed-off-by: Harsh Rawat <[email protected]>

Author: rawahars

cmd/containerd-shim-runhcs-v1/pod.go

@@ -171,7 +171,7 @@ func createPod(ctx context.Context, events publisher, req *task.CreateTaskReques
 
 	var parent *uvm.UtilityVM
 	var lopts *uvm.OptionsLCOW
-	if oci.IsIsolated(s) || oci.IsIsolatedJobContainer(s) {
+	if oci.IsIsolated(s) {
 		// Create the UVM parent
 		opts, err := oci.SpecToUVMCreateOpts(ctx, s, fmt.Sprintf("%s@vm", req.ID), owner)
 		if err != nil {

cmd/containerd-shim-runhcs-v1/task_hcs_test.go

@@ -332,7 +332,6 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) {
 		specs            *specs.Process
 		expectedCmdLine  string
 		expectedArgs     []string
-		initialUsername  string
 		expectedUsername string
 	}{
 		{
@@ -454,10 +453,6 @@ func Test_handleProcessArgsForIsolatedJobContainer(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			taskSpec := &specs.Spec{Annotations: tt.taskAnnotations}
-			// Ensure initial Username is set if provided
-			if tt.initialUsername != "" {
-				tt.specs.User.Username = tt.initialUsername
-			}
 
 			handleProcessArgsForIsolatedJobContainer(taskSpec, tt.specs)
 

internal/hcsoci/hcsdoc_wcow.go

@@ -14,6 +14,7 @@ import (
 	"strings"
 
 	"github.com/Microsoft/go-winio/pkg/fs"
+	"github.com/Microsoft/hcsshim/internal/ospath"
 	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sirupsen/logrus"
 
@@ -496,7 +497,12 @@ func createWindowsContainerDocument(ctx context.Context, coi *createOptionsInter
 
 		// If the customer specified a custom rootfs path then use that instead of default c:\hpc.
 		if customRootFsPath, ok := coi.Spec.Annotations[annotations.HostProcessRootfsLocation]; ok {
-			v2Container.Storage.PrivilegedContainerRootPath = customRootFsPath
+			customRootFsPathSanitized, err := ospath.Sanitize(customRootFsPath, ospath.DisallowedUVMMountPrefixes)
+			if err != nil {
+				return nil, nil, err
+			}
+
+			v2Container.Storage.PrivilegedContainerRootPath = customRootFsPathSanitized
 		}
 	}
 

internal/ospath/join.go

@@ -0,0 +1,67 @@
+package ospath
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path"
+	"path/filepath"
+	"strings"
+)
+
+var (
+	errUnsafePath = errors.New("unsafe path detected")
+
+	// DisallowedUVMMountPrefixes represents common locations within UVM
+	// where we do not want mounts to happen from customer perspective.
+	DisallowedUVMMountPrefixes = []string{
+		`C:\Windows`,
+		`C:\mounts`,
+		`C:\EFI`,
+		`C:\SandboxMounts`,
+	}
+)
+
+// Join joins paths using the target OS's path separator.
+func Join(os string, elem ...string) string {
+	if os == "windows" {
+		return filepath.Join(elem...)
+	}
+	return path.Join(elem...)
+}
+
+// Sanitize validates and normalizes a Windows path.
+func Sanitize(path string, disallowedPrefixes []string) (string, error) {
+	if path == "" {
+		return "", errUnsafePath
+	}
+
+	// Normalize the path.
+	cleanPath := filepath.Clean(path)
+
+	// Reject UNC paths (\\server\share or //server/share)
+	if strings.HasPrefix(cleanPath, `\\`) || strings.HasPrefix(cleanPath, `//`) {
+		return "", errUnsafePath
+	}
+
+	// Check if the path is not in the disallowed paths.
+	for _, prefix := range disallowedPrefixes {
+		if strings.HasPrefix(cleanPath, prefix) {
+			return "", errUnsafePath
+		}
+	}
+
+	// Reject if the path already exists (file/dir/symlink/junction).
+	// Use Lstat so we do not follow symlinks.
+	var err error
+	if _, err = os.Lstat(cleanPath); err == nil {
+		// Path exists
+		return "", fmt.Errorf("%s: path %q already exists", errUnsafePath, cleanPath)
+	}
+	if !os.IsNotExist(err) {
+		// Unexpected error (e.g., permission issues)
+		return "", fmt.Errorf("%s: error checking existence for %q: %w", errUnsafePath, cleanPath, err)
+	}
+
+	return cleanPath, nil
+}

internal/ospath/path.go

@@ -0,0 +1,84 @@
+package ospath
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestSanitize(t *testing.T) {
+	// Create a temp folder and file to simulate "already exists"
+	existingDir := t.TempDir()
+	existingFile := filepath.Join(existingDir, "exists.txt")
+	if err := os.WriteFile(existingFile, []byte("data"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		name               string
+		path               string
+		disallowedPrefixes []string
+		expectedPath       string
+		expectedErrPrefix  string
+	}{
+		{
+			name:         "valid path",
+			path:         filepath.Join(existingDir, "test"),
+			expectedPath: filepath.Join(existingDir, "test"),
+		},
+		{
+			name:              "empty path",
+			path:              "",
+			expectedErrPrefix: errUnsafePath.Error(),
+		},
+		{
+			name:               "path traversal",
+			path:               `C:\foo\..\Windows`,
+			disallowedPrefixes: []string{`C:\Windows`},
+			expectedErrPrefix:  errUnsafePath.Error(),
+		},
+		{
+			name:              "UNC path",
+			path:              `\\server\share`,
+			expectedErrPrefix: errUnsafePath.Error(),
+		},
+		{
+			name:               "disallowed prefix",
+			path:               `C:\Windows\System32`,
+			disallowedPrefixes: []string{`C:\Windows`},
+			expectedErrPrefix:  errUnsafePath.Error(),
+		},
+		{
+			name:              "existing folder",
+			path:              existingDir,
+			expectedErrPrefix: "already exists",
+		},
+		{
+			name:              "existing file",
+			path:              existingFile,
+			expectedErrPrefix: "already exists",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := Sanitize(tt.path, tt.disallowedPrefixes)
+
+			if tt.expectedErrPrefix != "" {
+				if err == nil {
+					t.Fatalf("expected error containing %q, got nil", tt.expectedErrPrefix)
+				}
+				if !strings.Contains(err.Error(), tt.expectedErrPrefix) {
+					t.Fatalf("expected error to contain %q, got %v", tt.expectedErrPrefix, err)
+				}
+			} else if err != nil {
+				t.Fatalf("expected no error, got %v", err)
+			}
+
+			if !strings.EqualFold(got, tt.expectedPath) {
+				t.Errorf("expected path %q, got %q", tt.expectedPath, got)
+			}
+		})
+	}
+}