osbuildexecutor: add validateOutputArchive() and run before extract

The tar file from the `osbuild-worker-executor` is potentially
tainted. Ensure we validate and only extract if it harmless.

internal/osbuildexecutor/export_test.go

@@ -0,0 +1,3 @@
+package osbuildexecutor
+
+var ValidateOutputArchive = validateOutputArchive

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",

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)`)
+}