diff --git a/internal/osbuildexecutor/export_test.go b/internal/osbuildexecutor/export_test.go new file mode 100644 index 000000000..a82b02a6b --- /dev/null +++ b/internal/osbuildexecutor/export_test.go @@ -0,0 +1,3 @@ +package osbuildexecutor + +var ValidateOutputArchive = validateOutputArchive diff --git a/internal/osbuildexecutor/runner-impl-aws-ec2.go b/internal/osbuildexecutor/runner-impl-aws-ec2.go index a09e476c9..f6fc98bef 100644 --- a/internal/osbuildexecutor/runner-impl-aws-ec2.go +++ b/internal/osbuildexecutor/runner-impl-aws-ec2.go @@ -1,6 +1,7 @@ package osbuildexecutor import ( + "archive/tar" "context" "encoding/json" "fmt" @@ -9,8 +10,11 @@ import ( "os" "os/exec" "path/filepath" + "strings" "time" + "golang.org/x/exp/slices" + "github.com/osbuild/images/pkg/osbuild" "github.com/sirupsen/logrus" @@ -176,7 +180,50 @@ func fetchOutputArchive(cacheDir, host string) (string, error) { return file.Name(), nil } +func validateOutputArchive(outputTarPath string) error { + f, err := os.Open(outputTarPath) + if err != nil { + return err + } + defer f.Close() + + tr := tar.NewReader(f) + for { + hdr, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + return err + } + // check for directory traversal attacks + if filepath.Clean(hdr.Name) != strings.TrimSuffix(hdr.Name, "/") { + return fmt.Errorf("name %q not clean, got %q after cleaning", hdr.Name, filepath.Clean(hdr.Name)) + } + if strings.HasPrefix(filepath.Clean(hdr.Name), "/") { + return fmt.Errorf("name %q must not start with an absolute path", hdr.Name) + } + // protect against someone smuggling in eg. device files + // XXX: should we support symlinks here? + if !slices.Contains([]byte{tar.TypeReg, tar.TypeDir}, hdr.Typeflag) { + return fmt.Errorf("name %q must be a file/dir, is header type %q", hdr.Name, hdr.Typeflag) + } + // protect against executables, this implicitly protects + // against suid/sgid (XXX: or should we also check that?) + if hdr.Typeflag == tar.TypeReg && hdr.Mode&0111 != 0 { + return fmt.Errorf("name %q must not be executable (is mode 0%o)", hdr.Name, hdr.Mode) + } + } + + return nil +} + func extractOutputArchive(outputDirectory, outputTar string) error { + // validate against directory traversal attacks + if err := validateOutputArchive(outputTar); err != nil { + return fmt.Errorf("unable to validate output tar: %w", err) + } + cmd := exec.Command("tar", "--strip-components=1", "-C", diff --git a/internal/osbuildexecutor/runner-impl-aws-ec2_test.go b/internal/osbuildexecutor/runner-impl-aws-ec2_test.go new file mode 100644 index 000000000..3eb8a2f27 --- /dev/null +++ b/internal/osbuildexecutor/runner-impl-aws-ec2_test.go @@ -0,0 +1,76 @@ +package osbuildexecutor_test + +import ( + "archive/tar" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/osbuild/osbuild-composer/internal/osbuildexecutor" +) + +func makeTestTarfile(t *testing.T, content map[*tar.Header]string) string { + tmpdir := t.TempDir() + testTarPath := filepath.Join(tmpdir, "test.tar") + f, err := os.Create(testTarPath) + assert.NoError(t, err) + defer f.Close() + + atar := tar.NewWriter(f) + for hdr, fcnt := range content { + if hdr.Mode == 0 { + hdr.Mode = 0644 + } + hdr.Size = int64(len(fcnt)) + + err := atar.WriteHeader(hdr) + assert.NoError(t, err) + _, err = atar.Write([]byte(fcnt)) + assert.NoError(t, err) + } + + return testTarPath +} + +func TestValidateOutputArchiveHappy(t *testing.T) { + testTarPath := makeTestTarfile(t, map[*tar.Header]string{ + &tar.Header{Name: "file1"}: "some content", + &tar.Header{Name: "path/to/file"}: "other content", + }) + err := osbuildexecutor.ValidateOutputArchive(testTarPath) + assert.NoError(t, err) +} + +func TestValidateOutputArchiveSadDotDot(t *testing.T) { + testTarPath := makeTestTarfile(t, map[*tar.Header]string{ + &tar.Header{Name: "file1/.."}: "some content", + }) + err := osbuildexecutor.ValidateOutputArchive(testTarPath) + assert.EqualError(t, err, `name "file1/.." not clean, got "." after cleaning`) +} + +func TestValidateOutputArchiveSadAbsolutePath(t *testing.T) { + testTarPath := makeTestTarfile(t, map[*tar.Header]string{ + &tar.Header{Name: "/file1"}: "some content", + }) + err := osbuildexecutor.ValidateOutputArchive(testTarPath) + assert.EqualError(t, err, `name "/file1" must not start with an absolute path`) +} + +func TestValidateOutputArchiveSadBadType(t *testing.T) { + testTarPath := makeTestTarfile(t, map[*tar.Header]string{ + &tar.Header{Name: "dev/sda", Typeflag: tar.TypeBlock}: "", + }) + err := osbuildexecutor.ValidateOutputArchive(testTarPath) + assert.EqualError(t, err, `name "dev/sda" must be a file/dir, is header type '4'`) +} + +func TestValidateOutputArchiveSadExecutable(t *testing.T) { + testTarPath := makeTestTarfile(t, map[*tar.Header]string{ + &tar.Header{Name: "exe", Mode: 0755}: "#!/bin/sh p0wned", + }) + err := osbuildexecutor.ValidateOutputArchive(testTarPath) + assert.EqualError(t, err, `name "exe" must not be executable (is mode 0755)`) +}