From 2250a00c2902fc1d32f75da317c9486a053ef00c Mon Sep 17 00:00:00 2001 From: Russell Rollins Date: Mon, 7 Dec 2020 10:34:50 -0500 Subject: [PATCH 1/3] Classify errors related to illegal input as such. --- .circleci/config.yml | 16 +++++++++ go.mod | 2 +- slug.go | 82 +++++++++++++++++++++++++++++--------------- slug_test.go | 23 +++++++------ 4 files changed, 85 insertions(+), 38 deletions(-) create mode 100644 .circleci/config.yml diff --git a/.circleci/config.yml b/.circleci/config.yml new file mode 100644 index 0000000..2070d1f --- /dev/null +++ b/.circleci/config.yml @@ -0,0 +1,16 @@ +version: 2 + +workflows: + version: 2 + build: + jobs: + - test + +jobs: + test: + docker: + - image: docker.mirror.hashicorp.services/cimg/go:1.15 + + steps: + - checkout + - run: go test ./... diff --git a/go.mod b/go.mod index 49ed430..c27382a 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,3 @@ module github.com/hashicorp/go-slug -go 1.12 +go 1.15 diff --git a/slug.go b/slug.go index 9491ca5..606b5f6 100644 --- a/slug.go +++ b/slug.go @@ -19,6 +19,23 @@ type Meta struct { Size int64 } +// IllegalSlugError indicates the provided slug (io.Writer for Pack, io.Reader +// for Unpack) validates a rule about its contents. For example, an absolute or +// external symlink. +type IllegalSlugError struct { + Err error +} + +// Error implements the error interface by returning an error string about the +// issue with the underlying error. +func (e *IllegalSlugError) Error() string { + return fmt.Sprintf("illegal slug error: %v", e.Err) +} + +// Unwrap returns the underlying issue with the provided Slug into the error +// chain. +func (e *IllegalSlugError) Unwrap() error { return e.Err } + // Pack creates a slug from a src directory, and writes the new slug // to w. Returns metadata about the slug and any errors. // @@ -48,12 +65,12 @@ func Pack(src string, w io.Writer, dereference bool) (*Meta, error) { // Flush the tar writer. if err := tarW.Close(); err != nil { - return nil, fmt.Errorf("Failed to close the tar archive: %v", err) + return nil, fmt.Errorf("failed to close the tar archive: %w", err) } // Flush the gzip writer. if err := gzipW.Close(); err != nil { - return nil, fmt.Errorf("Failed to close the gzip writer: %v", err) + return nil, fmt.Errorf("failed to close the gzip writer: %w", err) } return meta, nil @@ -68,7 +85,7 @@ func packWalkFn(root, src, dst string, tarW *tar.Writer, meta *Meta, dereference // Get the relative path from the current src directory. subpath, err := filepath.Rel(src, path) if err != nil { - return fmt.Errorf("Failed to get relative path for file %q: %v", path, err) + return fmt.Errorf("failed to get relative path for file %q: %w", path, err) } if subpath == "." { return nil @@ -89,7 +106,7 @@ func packWalkFn(root, src, dst string, tarW *tar.Writer, meta *Meta, dereference // Get the relative path from the initial root directory. subpath, err = filepath.Rel(root, strings.Replace(path, src, dst, 1)) if err != nil { - return fmt.Errorf("Failed to get relative path for file %q: %v", path, err) + return fmt.Errorf("failed to get relative path for file %q: %w", path, err) } if subpath == "." { return nil @@ -120,7 +137,7 @@ func packWalkFn(root, src, dst string, tarW *tar.Writer, meta *Meta, dereference case fm&os.ModeSymlink != 0: target, err := filepath.EvalSymlinks(path) if err != nil { - return fmt.Errorf("Failed to get symbolic link destination for %q: %v", path, err) + return fmt.Errorf("failed to get symbolic link destination for %q: %w", path, err) } // If the target is within the current source, we @@ -128,7 +145,7 @@ func packWalkFn(root, src, dst string, tarW *tar.Writer, meta *Meta, dereference if strings.HasPrefix(target, src) { link, err := filepath.Rel(filepath.Dir(path), target) if err != nil { - return fmt.Errorf("Failed to get relative path for symlink destination %q: %v", target, err) + return fmt.Errorf("failed to get relative path for symlink destination %q: %w", target, err) } header.Typeflag = tar.TypeSymlink @@ -148,7 +165,7 @@ func packWalkFn(root, src, dst string, tarW *tar.Writer, meta *Meta, dereference // Get the file info for the target. info, err = os.Lstat(target) if err != nil { - return fmt.Errorf("Failed to get file info from file %q: %v", target, err) + return fmt.Errorf("failed to get file info from file %q: %w", target, err) } // If the target is a directory we can recurse into the target @@ -166,12 +183,12 @@ func packWalkFn(root, src, dst string, tarW *tar.Writer, meta *Meta, dereference writeBody = true default: - return fmt.Errorf("Unexpected file mode %v", fm) + return fmt.Errorf("unexpected file mode %v", fm) } // Write the header first to the archive. if err := tarW.WriteHeader(header); err != nil { - return fmt.Errorf("Failed writing archive header for file %q: %v", path, err) + return fmt.Errorf("failed writing archive header for file %q: %w", path, err) } // Account for the file in the list. @@ -184,13 +201,13 @@ func packWalkFn(root, src, dst string, tarW *tar.Writer, meta *Meta, dereference f, err := os.Open(path) if err != nil { - return fmt.Errorf("Failed opening file %q for archiving: %v", path, err) + return fmt.Errorf("failed opening file %q for archiving: %w", path, err) } defer f.Close() size, err := io.Copy(tarW, f) if err != nil { - return fmt.Errorf("Failed copying file %q to archive: %v", path, err) + return fmt.Errorf("failed copying file %q to archive: %w", path, err) } // Add the size we copied to the body. @@ -207,7 +224,7 @@ func Unpack(r io.Reader, dst string) error { // Decompress as we read. uncompressed, err := gzip.NewReader(r) if err != nil { - return fmt.Errorf("Failed to uncompress slug: %v", err) + return fmt.Errorf("failed to uncompress slug: %w", err) } // Untar as we read. @@ -220,7 +237,7 @@ func Unpack(r io.Reader, dst string) error { break } if err != nil { - return fmt.Errorf("Failed to untar slug: %v", err) + return fmt.Errorf("failed to untar slug: %w", err) } // Get rid of absolute paths. @@ -233,7 +250,9 @@ func Unpack(r io.Reader, dst string) error { // Check for paths outside our directory, they are forbidden target := filepath.Clean(path) if !strings.HasPrefix(target, dst) { - return fmt.Errorf("Invalid filename, traversal with \"..\" outside of current directory") + return &IllegalSlugError{ + Err: fmt.Errorf("invalid filename, traversal with \"..\" outside of current directory"), + } } // Ensure the destination is not through any symlinks. This prevents @@ -260,39 +279,48 @@ func Unpack(r io.Reader, dst string) error { break } if err != nil { - return fmt.Errorf("Failed to evaluate path %q: %v", header.Name, err) + return fmt.Errorf("failed to evaluate path %q: %w", header.Name, err) } if fi.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("Cannot extract %q through symlink", - header.Name) + return &IllegalSlugError{ + Err: fmt.Errorf("cannot extract %q through symlink", header.Name), + } } } // Make the directories to the path. dir := filepath.Dir(path) if err := os.MkdirAll(dir, 0755); err != nil { - return fmt.Errorf("Failed to create directory %q: %v", dir, err) + return fmt.Errorf("failed to create directory %q: %w", dir, err) } // Handle symlinks. if header.Typeflag == tar.TypeSymlink { // Disallow absolute targets. if filepath.IsAbs(header.Linkname) { - return fmt.Errorf("Invalid symlink (%q -> %q) has absolute target", - header.Name, header.Linkname) + return &IllegalSlugError{ + Err: fmt.Errorf( + "invalid symlink (%q -> %q) has absolute target", + header.Name, header.Linkname, + ), + } } // Ensure the link target is within the destination directory. This // disallows providing symlinks to external files and directories. target := filepath.Join(dir, header.Linkname) if !strings.HasPrefix(target, dst) { - return fmt.Errorf("Invalid symlink (%q -> %q) has external target", - header.Name, header.Linkname) + return &IllegalSlugError{ + Err: fmt.Errorf( + "invalid symlink (%q -> %q) has external target", + header.Name, header.Linkname, + ), + } } // Create the symlink. if err := os.Symlink(header.Linkname, path); err != nil { - return fmt.Errorf("Failed creating symlink (%q -> %q): %v", + return fmt.Errorf("failed creating symlink (%q -> %q): %w", header.Name, header.Linkname, err) } @@ -303,7 +331,7 @@ func Unpack(r io.Reader, dst string) error { if header.Typeflag == tar.TypeDir { continue } else if header.Typeflag != tar.TypeReg && header.Typeflag != tar.TypeRegA { - return fmt.Errorf("Failed creating %q: unsupported type %c", path, + return fmt.Errorf("failed creating %q: unsupported type %c", path, header.Typeflag) } @@ -319,7 +347,7 @@ func Unpack(r io.Reader, dst string) error { } if err != nil { - return fmt.Errorf("Failed creating file %q: %v", path, err) + return fmt.Errorf("failed creating file %q: %w", path, err) } } @@ -327,14 +355,14 @@ func Unpack(r io.Reader, dst string) error { _, err = io.Copy(fh, untar) fh.Close() if err != nil { - return fmt.Errorf("Failed to copy slug file %q: %v", path, err) + return fmt.Errorf("failed to copy slug file %q: %w", path, err) } // Restore the file mode. We have to do this after writing the file, // since it is possible we have a read-only mode. mode := header.FileInfo().Mode() if err := os.Chmod(path, mode); err != nil { - return fmt.Errorf("Failed setting permissions on %q: %v", path, err) + return fmt.Errorf("failed setting permissions on %q: %w", path, err) } } return nil diff --git a/slug_test.go b/slug_test.go index de6f761..119ce63 100644 --- a/slug_test.go +++ b/slug_test.go @@ -4,6 +4,7 @@ import ( "archive/tar" "bytes" "compress/gzip" + "errors" "io" "io/ioutil" "os" @@ -443,7 +444,7 @@ func TestUnpackMaliciousSymlinks(t *testing.T) { Typeflag: tar.TypeSymlink, }, }, - err: `Cannot extract "subdir/parent/escapes" through symlink`, + err: `cannot extract "subdir/parent/escapes" through symlink`, }, { desc: "nested symlinks within symlinked dir", @@ -459,7 +460,7 @@ func TestUnpackMaliciousSymlinks(t *testing.T) { Typeflag: tar.TypeSymlink, }, }, - err: `Cannot extract "subdir/parent/otherdir/escapes" through symlink`, + err: `cannot extract "subdir/parent/otherdir/escapes" through symlink`, }, { desc: "regular file through symlink", @@ -474,7 +475,7 @@ func TestUnpackMaliciousSymlinks(t *testing.T) { Typeflag: tar.TypeReg, }, }, - err: `Cannot extract "subdir/parent/file" through symlink`, + err: `cannot extract "subdir/parent/file" through symlink`, }, { desc: "directory through symlink", @@ -489,7 +490,7 @@ func TestUnpackMaliciousSymlinks(t *testing.T) { Typeflag: tar.TypeDir, }, }, - err: `Cannot extract "subdir/parent/dir" through symlink`, + err: `cannot extract "subdir/parent/dir" through symlink`, }, } @@ -537,9 +538,10 @@ func TestUnpackMaliciousSymlinks(t *testing.T) { defer os.RemoveAll(dst) // Now try unpacking it, which should fail + var e *IllegalSlugError err = Unpack(fh, dst) - if err == nil || !strings.Contains(err.Error(), tc.err) { - t.Fatalf("expected %v, got %v", tc.err, err) + if err == nil || !errors.As(err, &e) || !strings.Contains(err.Error(), tc.err) { + t.Fatalf("expected *IllegalSlugError %v, got %T %v", tc.err, err, err) } }) } @@ -554,12 +556,12 @@ func TestUnpackMaliciousFiles(t *testing.T) { { desc: "filename containing path traversal", name: "../../../../../../../../tmp/test", - err: "Invalid filename, traversal with \"..\" outside of current directory", + err: "invalid filename, traversal with \"..\" outside of current directory", }, { desc: "should fail before attempting to create directories", name: "../../../../../../../../Users/root", - err: "Invalid filename, traversal with \"..\" outside of current directory", + err: "invalid filename, traversal with \"..\" outside of current directory", }, } @@ -614,9 +616,10 @@ func TestUnpackMaliciousFiles(t *testing.T) { defer os.RemoveAll(dst) // Now try unpacking it, which should fail + var e *IllegalSlugError err = Unpack(fh, dst) - if err == nil || !strings.Contains(err.Error(), tc.err) { - t.Fatalf("expected %v, got %v", tc.err, err) + if err == nil || !errors.As(err, &e) || !strings.Contains(err.Error(), tc.err) { + t.Fatalf("expected *IllegalSlugError %v, got %T %v", tc.err, err, err) } }) } From a51cdcaf634eb5a4a72c0da6603edad86b8670d5 Mon Sep 17 00:00:00 2001 From: Russell Rollins Date: Mon, 7 Dec 2020 10:49:32 -0500 Subject: [PATCH 2/3] The race detector is generally worth it in CI. --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 2070d1f..33b0318 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -13,4 +13,4 @@ jobs: steps: - checkout - - run: go test ./... + - run: go test -race ./... From b773cfe09b4a83b0a020d73e0eefca4311000369 Mon Sep 17 00:00:00 2001 From: Russell Rollins Date: Mon, 7 Dec 2020 13:46:20 -0500 Subject: [PATCH 3/3] Some comment fixup. --- slug.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/slug.go b/slug.go index 606b5f6..b911cf2 100644 --- a/slug.go +++ b/slug.go @@ -20,14 +20,12 @@ type Meta struct { } // IllegalSlugError indicates the provided slug (io.Writer for Pack, io.Reader -// for Unpack) validates a rule about its contents. For example, an absolute or -// external symlink. +// for Unpack) violates a rule about its contents. For example, an absolute or +// external symlink. It implements the error interface. type IllegalSlugError struct { Err error } -// Error implements the error interface by returning an error string about the -// issue with the underlying error. func (e *IllegalSlugError) Error() string { return fmt.Sprintf("illegal slug error: %v", e.Err) }