From c57770ac6d95376ceab90f5d4ae481b1a9c6122a Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Fri, 21 Aug 2026 09:35:07 +0200 Subject: [PATCH] OCPNODE-4526: Strengthen store path validation Add component-level '..' traversal blocking, max-length (256) validation, cross-store-type path uniqueness with normalization, and boundary tests. Signed-off-by: Sascha Grunert --- .../container-runtime-config/helpers.go | 41 +++++++- .../container-runtime-config/helpers_test.go | 98 +++++++++++++++++++ 2 files changed, 135 insertions(+), 4 deletions(-) diff --git a/pkg/controller/container-runtime-config/helpers.go b/pkg/controller/container-runtime-config/helpers.go index 77aecad860..a3bf47400e 100644 --- a/pkg/controller/container-runtime-config/helpers.go +++ b/pkg/controller/container-runtime-config/helpers.go @@ -728,27 +728,60 @@ func validateUserContainerRuntimeConfig(cfg *mcfgv1.ContainerRuntimeConfig) erro } } + // Cross-store-type path uniqueness: the same path must not appear in + // multiple store type lists since they have different directory + // structure expectations and lockfile locations. + layerPaths := make(map[string]bool, len(ctrcfg.AdditionalLayerStores)) + for _, s := range ctrcfg.AdditionalLayerStores { + layerPaths[filepath.Clean(string(s.Path))] = true + } + imagePaths := make(map[string]bool, len(ctrcfg.AdditionalImageStores)) + for _, s := range ctrcfg.AdditionalImageStores { + cleaned := filepath.Clean(string(s.Path)) + imagePaths[cleaned] = true + if layerPaths[cleaned] { + errs = append(errs, fmt.Errorf("path %q is used in both AdditionalLayerStores and AdditionalImageStores", s.Path)) + } + } + for _, s := range ctrcfg.AdditionalArtifactStores { + cleaned := filepath.Clean(string(s.Path)) + if layerPaths[cleaned] { + errs = append(errs, fmt.Errorf("path %q is used in both AdditionalLayerStores and AdditionalArtifactStores", s.Path)) + } + if imagePaths[cleaned] { + errs = append(errs, fmt.Errorf("path %q is used in both AdditionalImageStores and AdditionalArtifactStores", s.Path)) + } + } + return errors.Join(errs...) } // storePathRegexp matches the CRD CEL rule: ^/[a-zA-Z0-9/._-]+$ var storePathRegexp = regexp.MustCompile(`^/[a-zA-Z0-9/._-]+$`) -// validateStorePath validates that a storage path is absolute, only contains -// allowed characters (a-z, A-Z, 0-9, '/', '.', '_', '-'), and does not -// contain consecutive forward slashes. This mirrors the CRD-level CEL -// validation so that invalid config is caught early. +// storePathMaxLength mirrors the CRD MaxLength=256 for StorePath. +const storePathMaxLength = 256 + +// validateStorePath mirrors the CRD-level CEL validation for StorePath. func validateStorePath(p mcfgv1.StorePath, field string) error { path := string(p) if path == "" { return fmt.Errorf("invalid %s: path must not be empty", field) } + if len(path) > storePathMaxLength { + return fmt.Errorf("invalid %s path %q: must not exceed %d characters", field, path, storePathMaxLength) + } if !storePathRegexp.MatchString(path) { return fmt.Errorf("invalid %s path %q: must be an absolute path containing only alphanumeric characters, '/', '.', '_', and '-'", field, path) } if strings.Contains(path, "//") { return fmt.Errorf("invalid %s path %q: must not contain consecutive forward slashes", field, path) } + for _, component := range strings.Split(path, "/") { + if component == ".." { + return fmt.Errorf("invalid %s path %q: must not contain '..' components", field, path) + } + } return nil } diff --git a/pkg/controller/container-runtime-config/helpers_test.go b/pkg/controller/container-runtime-config/helpers_test.go index 2c8e9564ac..6c48efaf72 100644 --- a/pkg/controller/container-runtime-config/helpers_test.go +++ b/pkg/controller/container-runtime-config/helpers_test.go @@ -8,6 +8,7 @@ import ( "fmt" "os" "reflect" + "strings" "testing" "time" @@ -1548,6 +1549,10 @@ func TestValidateStorePath(t *testing.T) { name: "valid path with dots and dashes", path: "/mnt/nfs-images/cache_v1.0", }, + { + name: "double dots within filename", + path: "/var/lib/foo..bar", + }, { name: "empty path", path: "", @@ -1578,6 +1583,35 @@ func TestValidateStorePath(t *testing.T) { wantErr: true, errMsg: "must not contain consecutive forward slashes", }, + { + name: "dot-dot traversal", + path: "/var/lib/../../etc", + wantErr: true, + errMsg: "must not contain '..' components", + }, + { + name: "dot-dot at end", + path: "/mnt/store/..", + wantErr: true, + errMsg: "must not contain '..' components", + }, + { + name: "path with colon", + path: "/var/lib/store:ref", + wantErr: true, + errMsg: "must be an absolute path", + }, + { + name: "path at max length", + path: mcfgv1.StorePath("/" + strings.Repeat("a", 255)), + wantErr: false, + }, + { + name: "path exceeds max length", + path: mcfgv1.StorePath("/" + strings.Repeat("a", 256)), + wantErr: true, + errMsg: "must not exceed 256 characters", + }, } for _, test := range tests { @@ -1656,6 +1690,70 @@ func TestValidateUserContainerRuntimeConfigAdditionalStores(t *testing.T) { }, wantErr: true, }, + { + name: "cross-store duplicate layer and image", + cfg: &mcfgv1.ContainerRuntimeConfig{ + Spec: mcfgv1.ContainerRuntimeConfigSpec{ + ContainerRuntimeConfig: &mcfgv1.ContainerRuntimeConfiguration{ + AdditionalLayerStores: []mcfgv1.AdditionalLayerStore{ + {Path: "/mnt/shared"}, + }, + AdditionalImageStores: []mcfgv1.AdditionalImageStore{ + {Path: "/mnt/shared"}, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "cross-store duplicate layer and artifact", + cfg: &mcfgv1.ContainerRuntimeConfig{ + Spec: mcfgv1.ContainerRuntimeConfigSpec{ + ContainerRuntimeConfig: &mcfgv1.ContainerRuntimeConfiguration{ + AdditionalLayerStores: []mcfgv1.AdditionalLayerStore{ + {Path: "/mnt/shared"}, + }, + AdditionalArtifactStores: []mcfgv1.AdditionalArtifactStore{ + {Path: "/mnt/shared"}, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "cross-store duplicate image and artifact", + cfg: &mcfgv1.ContainerRuntimeConfig{ + Spec: mcfgv1.ContainerRuntimeConfigSpec{ + ContainerRuntimeConfig: &mcfgv1.ContainerRuntimeConfiguration{ + AdditionalImageStores: []mcfgv1.AdditionalImageStore{ + {Path: "/mnt/shared"}, + }, + AdditionalArtifactStores: []mcfgv1.AdditionalArtifactStore{ + {Path: "/mnt/shared"}, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "cross-store duplicate with trailing slash normalization", + cfg: &mcfgv1.ContainerRuntimeConfig{ + Spec: mcfgv1.ContainerRuntimeConfigSpec{ + ContainerRuntimeConfig: &mcfgv1.ContainerRuntimeConfiguration{ + AdditionalLayerStores: []mcfgv1.AdditionalLayerStore{ + {Path: "/mnt/shared"}, + }, + AdditionalImageStores: []mcfgv1.AdditionalImageStore{ + {Path: "/mnt/shared/"}, + }, + }, + }, + }, + wantErr: true, + }, } for _, test := range tests {