From d6d60287ee3a8a064340582d65c131181ae77127 Mon Sep 17 00:00:00 2001 From: David Calavera Date: Tue, 20 Oct 2015 13:09:48 -0400 Subject: [PATCH] Move volume name validation to the local driver. Delegate validation tasks to the volume drivers. It's up to them to decide whether a name is valid or not. Restrict volume names for the local driver to prevent creating mount points outside docker's volumes directory. Signed-off-by: David Calavera --- daemon/daemon.go | 5 ++-- daemon/volumes_linux_unit_test.go | 4 --- daemon/volumes_unix.go | 7 ----- errors/daemon.go | 8 +++--- utils/names.go | 9 +++++++ volume/local/local.go | 23 ++++++++++++++-- volume/local/local_test.go | 45 +++++++++++++++++++++++++++++++ 7 files changed, 81 insertions(+), 20 deletions(-) create mode 100644 utils/names.go diff --git a/daemon/daemon.go b/daemon/daemon.go index 91af20ac66..c840f593e9 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -12,7 +12,6 @@ import ( "io/ioutil" "os" "path/filepath" - "regexp" "runtime" "strings" "sync" @@ -59,8 +58,8 @@ import ( ) var ( - validContainerNameChars = `[a-zA-Z0-9][a-zA-Z0-9_.-]` - validContainerNamePattern = regexp.MustCompile(`^/?` + validContainerNameChars + `+$`) + validContainerNameChars = utils.RestrictedNameChars + validContainerNamePattern = utils.RestrictedNamePattern errSystemNotSupported = errors.New("The Docker daemon is not supported on this platform.") ) diff --git a/daemon/volumes_linux_unit_test.go b/daemon/volumes_linux_unit_test.go index 63181bf278..6f4ab882dc 100644 --- a/daemon/volumes_linux_unit_test.go +++ b/daemon/volumes_linux_unit_test.go @@ -24,10 +24,6 @@ func TestParseBindMount(t *testing.T) { {"name:/tmp:ro", "local", "/tmp", "", "name", "local", false, false}, {"local/name:/tmp:rw", "", "/tmp", "", "local/name", "local", true, false}, {"/tmp:tmp", "", "", "", "", "", true, true}, - {"./name:/tmp", "", "", "", "", "", true, true}, - {"../name:/tmp", "", "", "", "", "", true, true}, - {"./:/tmp", "", "", "", "", "", true, true}, - {"../:/tmp", "", "", "", "", "", true, true}, } for _, c := range cases { diff --git a/daemon/volumes_unix.go b/daemon/volumes_unix.go index 9fea1cf3a5..3b32c65f9a 100644 --- a/daemon/volumes_unix.go +++ b/daemon/volumes_unix.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "os" "path/filepath" - "regexp" "sort" "strings" @@ -103,12 +102,6 @@ func parseBindMount(spec, volumeDriver string) (*mountPoint, error) { } if len(source) == 0 { - //validate the name of named volume - nameRegex := regexp.MustCompile(`(^.+[^0-9A-Za-z_]+$)|(/)`) - if nameRegex.MatchString(name) { - return nil, derr.ErrorCodeVolumeName.WithArgs(name) - } - bind.Driver = volumeDriver if len(bind.Driver) == 0 { bind.Driver = volume.DefaultDriverName diff --git a/errors/daemon.go b/errors/daemon.go index 88024bc1ab..ea31580094 100644 --- a/errors/daemon.go +++ b/errors/daemon.go @@ -387,10 +387,10 @@ var ( // ErrorCodeVolumeName is generated when the name of named volume isn't valid. ErrorCodeVolumeName = errcode.Register(errGroup, errcode.ErrorDescriptor{ - Value: "VOLUMENAME", - Message: "%s looks like a relative path, but it's taken as a name. And it is not a valid name.", - Description: "The name of named volume is invalid", - HTTPStatusCode: http.StatusInternalServerError, + Value: "VOLUME_NAME_INVALID", + Message: "%s includes invalid characters for a local volume name, only %s are allowed", + Description: "The name of volume is invalid", + HTTPStatusCode: http.StatusBadRequest, }) // ErrorCodeVolumeFromBlank is generated when path to a volume is blank. diff --git a/utils/names.go b/utils/names.go new file mode 100644 index 0000000000..e09e569b15 --- /dev/null +++ b/utils/names.go @@ -0,0 +1,9 @@ +package utils + +import "regexp" + +// RestrictedNameChars collects the characters allowed to represent a name, normally used to validate container and volume names. +const RestrictedNameChars = `[a-zA-Z0-9][a-zA-Z0-9_.-]` + +// RestrictedNamePattern is a regular expression to validate names against the collection of restricted characters. +var RestrictedNamePattern = regexp.MustCompile(`^/?` + RestrictedNameChars + `+$`) diff --git a/volume/local/local.go b/volume/local/local.go index d3ec38b050..e0c3a64462 100644 --- a/volume/local/local.go +++ b/volume/local/local.go @@ -11,7 +11,9 @@ import ( "path/filepath" "sync" + derr "github.com/docker/docker/errors" "github.com/docker/docker/pkg/idtools" + "github.com/docker/docker/utils" "github.com/docker/docker/volume" ) @@ -23,8 +25,14 @@ const ( volumesPathName = "volumes" ) -// ErrNotFound is the typed error returned when the requested volume name can't be found -var ErrNotFound = errors.New("volume not found") +var ( + // ErrNotFound is the typed error returned when the requested volume name can't be found + ErrNotFound = errors.New("volume not found") + // volumeNameRegex ensures the name asigned for the volume is valid. + // This name is used to create the bind directory, so we need to avoid characters that + // would make the path to escape the root directory. + volumeNameRegex = utils.RestrictedNamePattern +) // New instantiates a new Root instance with the provided scope. Scope // is the base path that the Root instance uses to store its @@ -96,6 +104,10 @@ func (r *Root) Name() string { // the underlying directory tree required for this volume in the // process. func (r *Root) Create(name string, _ map[string]string) (volume.Volume, error) { + if err := r.validateName(name); err != nil { + return nil, err + } + r.m.Lock() defer r.m.Unlock() @@ -174,6 +186,13 @@ func (r *Root) Get(name string) (volume.Volume, error) { return v, nil } +func (r *Root) validateName(name string) error { + if !volumeNameRegex.MatchString(name) { + return derr.ErrorCodeVolumeName.WithArgs(name, utils.RestrictedNameChars) + } + return nil +} + // localVolume implements the Volume interface from the volume package and // represents the volumes created by Root. type localVolume struct { diff --git a/volume/local/local_test.go b/volume/local/local_test.go index 45fdd5ffc8..2c5b800a56 100644 --- a/volume/local/local_test.go +++ b/volume/local/local_test.go @@ -79,3 +79,48 @@ func TestInitializeWithVolumes(t *testing.T) { t.Fatal("expected to re-initialize root with existing volumes") } } + +func TestCreate(t *testing.T) { + rootDir, err := ioutil.TempDir("", "local-volume-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(rootDir) + + r, err := New(rootDir, 0, 0) + if err != nil { + t.Fatal(err) + } + + cases := map[string]bool{ + "name": true, + "name-with-dash": true, + "name_with_underscore": true, + "name/with/slash": false, + "name/with/../../slash": false, + "./name": false, + "../name": false, + "./": false, + "../": false, + "~": false, + ".": false, + "..": false, + "...": false, + } + + for name, success := range cases { + v, err := r.Create(name, nil) + if success { + if err != nil { + t.Fatal(err) + } + if v.Name() != name { + t.Fatalf("Expected volume with name %s, got %s", name, v.Name()) + } + } else { + if err == nil { + t.Fatalf("Expected error creating volume with name %s, got nil", name) + } + } + } +}