From 212c0346d8c47d04ae73fc7d898f5104bf891df5 Mon Sep 17 00:00:00 2001 From: Shiwei Zhang Date: Mon, 15 Apr 2019 10:59:06 +0800 Subject: [PATCH] Default path validation on push (#66) Validate name by default --- cmd/oras/push.go | 13 +++++--- pkg/content/file.go | 6 +++- pkg/oras/errors.go | 8 +++++ pkg/oras/push.go | 13 ++++++-- pkg/oras/push_opts.go | 65 +++++++++++++++++++++++++++++++++++++- pkg/oras/push_opts_test.go | 64 +++++++++++++++++++++++++++++++++++++ 6 files changed, 160 insertions(+), 9 deletions(-) create mode 100644 pkg/oras/push_opts_test.go diff --git a/cmd/oras/push.go b/cmd/oras/push.go index 14d144601..10f20c192 100644 --- a/cmd/oras/push.go +++ b/cmd/oras/push.go @@ -20,10 +20,11 @@ const ( ) type pushOptions struct { - targetRef string - fileRefs []string - manifestConfigRef string - manifestAnnotations string + targetRef string + fileRefs []string + manifestConfigRef string + manifestAnnotations string + pathValidationDisabled bool debug bool configs []string @@ -60,6 +61,7 @@ Example - Push file "hi.txt" with the custom manifest config "config.json" of th cmd.Flags().StringVarP(&opts.manifestConfigRef, "manifest-config", "", "", "manifest config file") cmd.Flags().StringVarP(&opts.manifestAnnotations, "manifest-annotations", "", "", "manifest annotation file") + cmd.Flags().BoolVarP(&opts.pathValidationDisabled, "disable-path-validation", "", false, "skip path validation") cmd.Flags().BoolVarP(&opts.debug, "debug", "d", false, "debug mode") cmd.Flags().StringArrayVarP(&opts.configs, "config", "c", nil, "auth config path") cmd.Flags().StringVarP(&opts.username, "username", "u", "", "registry username") @@ -100,6 +102,9 @@ func runPush(opts pushOptions) error { file.Annotations = nil pushOpts = append(pushOpts, oras.WithConfig(file)) } + if opts.pathValidationDisabled { + pushOpts = append(pushOpts, oras.WithNameValidation(nil)) + } for _, fileRef := range opts.fileRefs { filename, mediaType := parseFileRef(fileRef, "") name := filepath.Clean(filename) diff --git a/pkg/content/file.go b/pkg/content/file.go index 1e0600a6c..3e5b7fde2 100644 --- a/pkg/content/file.go +++ b/pkg/content/file.go @@ -235,7 +235,11 @@ func (s *FileStore) resolveWritePath(name string) (string, error) { return "", err } rel, err := filepath.Rel(base, target) - if err != nil || strings.HasPrefix(filepath.ToSlash(rel), "../") { + if err != nil { + return "", ErrPathTraversalDisallowed + } + rel = filepath.ToSlash(rel) + if strings.HasPrefix(rel, "../") || rel == ".." { return "", ErrPathTraversalDisallowed } } diff --git a/pkg/oras/errors.go b/pkg/oras/errors.go index e86d8a54f..a045adc46 100644 --- a/pkg/oras/errors.go +++ b/pkg/oras/errors.go @@ -7,3 +7,11 @@ var ( ErrResolverUndefined = errors.New("resolver_undefined") ErrEmptyDescriptors = errors.New("empty_descriptors") ) + +// Path validation related errors +var ( + ErrDirtyPath = errors.New("dirty_path") + ErrPathNotSlashSeparated = errors.New("path_not_slash_separated") + ErrAbsolutePathDisallowed = errors.New("absolute_path_disallowed") + ErrPathTraversalDisallowed = errors.New("path_traversal_disallowed") +) diff --git a/pkg/oras/push.go b/pkg/oras/push.go index c2a3b4796..7ce35ff46 100644 --- a/pkg/oras/push.go +++ b/pkg/oras/push.go @@ -19,19 +19,26 @@ func Push(ctx context.Context, resolver remotes.Resolver, ref string, provider c if len(descriptors) == 0 { return ErrEmptyDescriptors } - var opt pushOpts + opt := pushOptsDefaults() for _, o := range opts { - if err := o(&opt); err != nil { + if err := o(opt); err != nil { return err } } + if opt.validateName != nil { + for _, desc := range descriptors { + if err := opt.validateName(desc); err != nil { + return err + } + } + } pusher, err := resolver.Pusher(ctx, ref) if err != nil { return err } - desc, provider, err := pack(provider, descriptors, &opt) + desc, provider, err := pack(provider, descriptors, opt) if err != nil { return err } diff --git a/pkg/oras/push_opts.go b/pkg/oras/push_opts.go index 022f3fb72..eac3b2d45 100644 --- a/pkg/oras/push_opts.go +++ b/pkg/oras/push_opts.go @@ -1,11 +1,25 @@ package oras -import ocispec "github.com/opencontainers/image-spec/specs-go/v1" +import ( + "path/filepath" + "strings" + + orascontent "github.com/deislabs/oras/pkg/content" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/pkg/errors" +) type pushOpts struct { config *ocispec.Descriptor configAnnotations map[string]string manifestAnnotations map[string]string + validateName func(desc ocispec.Descriptor) error +} + +func pushOptsDefaults() *pushOpts { + return &pushOpts{ + validateName: ValidateNameAsPath, + } } // PushOpt allows callers to set options on the oras push @@ -34,3 +48,52 @@ func WithManifestAnnotations(annotations map[string]string) PushOpt { return nil } } + +// WithNameValidation validates the image title in the descriptor. +// Pass nil to disable name validation. +func WithNameValidation(validate func(desc ocispec.Descriptor) error) PushOpt { + return func(o *pushOpts) error { + o.validateName = validate + return nil + } +} + +// ValidateNameAsPath validates name in the descriptor as file path in order +// to generate good packages intended to be pulled using the FileStore or +// the oras cli. +// For cross-platform considerations, only unix paths are accepted. +func ValidateNameAsPath(desc ocispec.Descriptor) error { + // no empty name + path, ok := orascontent.ResolveName(desc) + if !ok || path == "" { + return orascontent.ErrNoName + } + + // path should be clean + if target := filepath.ToSlash(filepath.Clean(path)); target != path { + return errors.Wrap(ErrDirtyPath, path) + } + + // path should be slash-separated + if strings.Contains(path, "\\") { + return errors.Wrap(ErrPathNotSlashSeparated, path) + } + + // disallow absolute path: covers unix and windows format + if strings.HasPrefix(path, "/") { + return errors.Wrap(ErrAbsolutePathDisallowed, path) + } + if len(path) > 2 { + c := path[0] + if path[1] == ':' && path[2] == '/' && ('a' <= c && c <= 'z' || 'A' <= c && c <= 'Z') { + return errors.Wrap(ErrAbsolutePathDisallowed, path) + } + } + + // disallow path traversal + if strings.HasPrefix(path, "../") || path == ".." { + return errors.Wrap(ErrPathTraversalDisallowed, path) + } + + return nil +} diff --git a/pkg/oras/push_opts_test.go b/pkg/oras/push_opts_test.go new file mode 100644 index 000000000..829716024 --- /dev/null +++ b/pkg/oras/push_opts_test.go @@ -0,0 +1,64 @@ +package oras + +import ( + "testing" + + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/stretchr/testify/suite" +) + +type PushOptsSuite struct { + suite.Suite +} + +func (suite *PushOptsSuite) TestValidateNameAsPath() { + var err error + + // valid path + err = ValidateNameAsPath(descFromName("hello.txt")) + suite.NoError(err, "valid path") + err = ValidateNameAsPath(descFromName("foo/bar")) + suite.NoError(err, "valid path with multiple sub-directories") + + // no empty name + err = ValidateNameAsPath(descFromName("")) + suite.Error(err, "empty path") + + // path should be clean + err = ValidateNameAsPath(descFromName("./hello.txt")) + suite.Error(err, "dirty path") + err = ValidateNameAsPath(descFromName("foo/../bar")) + suite.Error(err, "dirty path") + + // path should be slash-separated + err = ValidateNameAsPath(descFromName("foo\\bar")) + suite.Error(err, "path not slash separated") + + // disallow absolute path + err = ValidateNameAsPath(descFromName("/foo/bar")) + suite.Error(err, "unix: absolute path disallowed") + err = ValidateNameAsPath(descFromName("C:\\foo\\bar")) + suite.Error(err, "windows: absolute path disallowed") + err = ValidateNameAsPath(descFromName("C:/foo/bar")) + suite.Error(err, "windows: absolute path disallowed") + + // disallow path traversal + err = ValidateNameAsPath(descFromName("..")) + suite.Error(err, "path traversal disallowed") + err = ValidateNameAsPath(descFromName("../bar")) + suite.Error(err, "path traversal disallowed") + err = ValidateNameAsPath(descFromName("foo/../../bar")) + suite.Error(err, "path traversal disallowed") +} + +func TestPushOptsSuite(t *testing.T) { + suite.Run(t, new(PushOptsSuite)) +} + +func descFromName(name string) ocispec.Descriptor { + return ocispec.Descriptor{ + Annotations: map[string]string{ + ocispec.AnnotationTitle: name, + }, + } +}