Fix possibility of path traversal for zip and tar archival

policies/apt.go

@@ -39,6 +39,8 @@ var debArchiveTypeMap = map[agentendpointpb.AptRepository_ArchiveType]string{
 	agentendpointpb.AptRepository_DEB_SRC: "deb-src",
 }
 
+var osInfoProvider osinfo.Provider = osinfo.NewProvider()
+
 const aptGPGFile = "/etc/apt/trusted.gpg.d/osconfig_agent_managed.gpg"
 
 func isArmoredGPGKey(keyData []byte) bool {
@@ -85,8 +87,8 @@ func containsEntity(es []*openpgp.Entity, e *openpgp.Entity) bool {
 	return false
 }
 
-func readInstanceOsInfo() (string, float64, error) {
-	oi, err := osinfo.Get()
+func readInstanceOsInfo(ctx context.Context) (string, float64, error) {
+	oi, err := osInfoProvider.GetOSInfo(ctx)
 	if err != nil {
 		return "", 0, fmt.Errorf("error getting osinfo: %v", err)
 	}
@@ -99,8 +101,8 @@ func readInstanceOsInfo() (string, float64, error) {
 	return oi.ShortName, osVersion, nil
 }
 
-func shouldUseSignedBy() bool {
-	osShortName, osVersion, err := readInstanceOsInfo()
+func shouldUseSignedBy(ctx context.Context) bool {
+	osShortName, osVersion, err := readInstanceOsInfo(ctx)
 	if err != nil {
 		return false // Default to not using signed-by approach
 	}
@@ -181,7 +183,7 @@ func aptRepositories(ctx context.Context, repos []*agentendpointpb.AptRepository
 	var buf bytes.Buffer
 	buf.WriteString("# Repo file managed by Google OSConfig agent\n")
 
-	shouldUseSignedByBool := shouldUseSignedBy()
+	shouldUseSignedByBool := shouldUseSignedBy(ctx)
 	for _, repo := range repos {
 		line := getAptRepoLine(repo, shouldUseSignedByBool)
 		buf.WriteString(line + "\n")

policies/apt_test.go

@@ -24,6 +24,7 @@ import (
 	"testing"
 
 	"cloud.google.com/go/osconfig/agentendpoint/apiv1beta/agentendpointpb"
+	"github.com/GoogleCloudPlatform/osconfig/osinfo"
 )
 
 func runAptRepositories(ctx context.Context, repos []*agentendpointpb.AptRepository) (string, error) {
@@ -47,37 +48,75 @@ func runAptRepositories(ctx context.Context, repos []*agentendpointpb.AptReposit
 }
 
 func TestAptRepositories(t *testing.T) {
+	debian10 := func() (string, string, string) {
+		return "debian", "Debian", "10"
+	}
+
+	debian12 := func() (string, string, string) {
+		return "debian", "Debian", "12"
+	}
+
 	tests := []struct {
-		desc  string
-		repos []*agentendpointpb.AptRepository
-		want  string
+		name                   string
+		repos                  []*agentendpointpb.AptRepository
+		nameAndVersionProvider func() (string, string, string)
+		want                   string
 	}{
-		{"no repos", []*agentendpointpb.AptRepository{}, "# Repo file managed by Google OSConfig agent\n"},
 		{
-			"1 repo",
-			[]*agentendpointpb.AptRepository{
+			name:                   "No repositories",
+			nameAndVersionProvider: debian10,
+			repos:                  []*agentendpointpb.AptRepository{},
+			want:                   "# Repo file managed by Google OSConfig agent\n"},
+		{
+			name:                   "1 repositoy, Debian 10",
+			nameAndVersionProvider: debian10,
+			repos: []*agentendpointpb.AptRepository{
 				{Uri: "http://repo1-url/", Distribution: "distribution", Components: []string{"component1"}},
 			},
-			"# Repo file managed by Google OSConfig agent\n\ndeb http://repo1-url/ distribution component1\n",
+			want: "# Repo file managed by Google OSConfig agent\n\ndeb http://repo1-url/ distribution component1\n",
 		},
 		{
-			"2 repos",
-			[]*agentendpointpb.AptRepository{
+			name:                   "1 repositoy, Debian 12",
+			nameAndVersionProvider: debian12,
+			repos: []*agentendpointpb.AptRepository{
+				{Uri: "http://repo1-url/", Distribution: "distribution", Components: []string{"component1"}},
+			},
+			want: "# Repo file managed by Google OSConfig agent\n\ndeb [signed-by=/etc/apt/trusted.gpg.d/osconfig_agent_managed.gpg] http://repo1-url/ distribution component1\n",
+		},
+		{
+			name:                   "2 repos, Debian 10",
+			nameAndVersionProvider: debian10,
+			repos: []*agentendpointpb.AptRepository{
 				{Uri: "http://repo1-url/", Distribution: "distribution", Components: []string{"component1"}, ArchiveType: agentendpointpb.AptRepository_DEB_SRC},
 				{Uri: "http://repo2-url/", Distribution: "distribution", Components: []string{"component1", "component2"}, ArchiveType: agentendpointpb.AptRepository_DEB},
 			},
-			"# Repo file managed by Google OSConfig agent\n\ndeb-src http://repo1-url/ distribution component1\n\ndeb http://repo2-url/ distribution component1 component2\n",
+			want: "# Repo file managed by Google OSConfig agent\n\ndeb-src http://repo1-url/ distribution component1\n\ndeb http://repo2-url/ distribution component1 component2\n",
+		},
+		{
+			name:                   "2 repos, Debian 12",
+			nameAndVersionProvider: debian12,
+			repos: []*agentendpointpb.AptRepository{
+				{Uri: "http://repo1-url/", Distribution: "distribution", Components: []string{"component1"}, ArchiveType: agentendpointpb.AptRepository_DEB_SRC},
+				{Uri: "http://repo2-url/", Distribution: "distribution", Components: []string{"component1", "component2"}, ArchiveType: agentendpointpb.AptRepository_DEB},
+			},
+			want: "# Repo file managed by Google OSConfig agent\n\ndeb-src [signed-by=/etc/apt/trusted.gpg.d/osconfig_agent_managed.gpg] http://repo1-url/ distribution component1\n\ndeb [signed-by=/etc/apt/trusted.gpg.d/osconfig_agent_managed.gpg] http://repo2-url/ distribution component1 component2\n",
 		},
 	}
 
 	for _, tt := range tests {
+		osInfoProviderActual := osInfoProvider
+		defer func() { osInfoProvider = osInfoProviderActual }()
+
+		osInfoStub := stubOsInfoProvider{nameVersionProvider: tt.nameAndVersionProvider}
+		osInfoProvider = osInfoStub
+
 		got, err := runAptRepositories(context.Background(), tt.repos)
 		if err != nil {
 			t.Fatal(err)
 		}
 
 		if got != tt.want {
-			t.Errorf("%s: got:\n%q\nwant:\n%q", tt.desc, got, tt.want)
+			t.Errorf("%s: got:\n%q\nwant:\n%q", tt.name, got, tt.want)
 		}
 	}
 }
@@ -107,7 +146,7 @@ func TestGetAptGPGKey(t *testing.T) {
 
 func TestUseSignedBy(t *testing.T) {
 	tests := []struct {
-		desc string
+		name string
 		repo *agentendpointpb.AptRepository
 		want string
 	}{
@@ -128,7 +167,25 @@ func TestUseSignedBy(t *testing.T) {
 		aptRepoLine := getAptRepoLine(tt.repo, useSignedBy)
 
 		if aptRepoLine != tt.want {
-			t.Errorf("%s: got:\n%q\nwant:\n%q", tt.desc, aptRepoLine, tt.want)
+			t.Errorf("%s: got:\n%q\nwant:\n%q", tt.name, aptRepoLine, tt.want)
 		}
 	}
 }
+
+type stubOsInfoProvider struct {
+	nameVersionProvider func() (string, string, string)
+}
+
+func (s stubOsInfoProvider) GetOSInfo(ctx context.Context) (osinfo.OSInfo, error) {
+	short, long, version := s.nameVersionProvider()
+
+	return osinfo.OSInfo{
+		Hostname:      "test",
+		LongName:      long,
+		ShortName:     short,
+		Version:       version,
+		KernelVersion: "test",
+		KernelRelease: "test",
+		Architecture:  "x86_64",
+	}, nil
+}

policies/recipes/steps.go

@@ -46,6 +46,8 @@
 	agentendpointpb.SoftwareRecipe_Step_RunScript_POWERSHELL:              ".ps1",
 }
 
+var chown = chownFunc
+
 func stepCopyFile(step *agentendpointpb.SoftwareRecipe_Step_CopyFile, artifacts map[string]string, runEnvs []string, stepDir string) error {
 	dest, err := util.NormPath(step.Destination)
 	if err != nil {
@@ -127,19 +129,73 @@
 	return strings.HasSuffix(name, "/")
 }
 
+func ensureSymlinkBelongsToDir(dirPath string, symlink string) error {
+	dirAbs, err := filepath.Abs(dirPath)
+	if err != nil {
+		return err
+	}
+	symlinkAbs, err := filepath.Abs(symlink)
+	if err != nil {
+		return err
+	}
+
+	evaluatedSymlinkAbs, err := filepath.EvalSymlinks(symlinkAbs)
+	if err != nil {
+		return err
+	}
+
+	rel, err := filepath.Rel(dirAbs, evaluatedSymlinkAbs)
+	if err != nil {
+		return err
+	}
+
+	if strings.HasPrefix(rel, "..") {
+		return fmt.Errorf("symlink %s, does not belongs to dir %s, rel %s", symlink, dirPath, rel)
+	}
+
+	return nil
+}
+
+func ensureFilePathBelongsToDir(dirPath string, filePath string) error {
+	dirAbs, err := filepath.Abs(dirPath)
+	if err != nil {
+		return err
+	}
+	fileAbs, err := filepath.Abs(filePath)
+	if err != nil {
+		return err
+	}
+
+	rel, err := filepath.Rel(dirAbs, fileAbs)
+	if err != nil {
+		return err
+	}
+
+	if strings.HasPrefix(rel, "..") {
+		return fmt.Errorf("path %s, does not belongs to dir %s, rel %s", filePath, dirPath, rel)
+	}
+
+	return nil
+}
+
 func extractZip(zipPath string, dst string) error {
 	zr, err := zip.OpenReader(zipPath)
 	if err != nil {
 		return err
 	}
 	defer zr.Close()
 
-	// Check for conflicts
+	// Check that we can extract zip
 	for _, f := range zr.File {
-		filen, err := util.NormPath(util.SanitizePath(filepath.Join(dst, f.Name)))
+		filen, err := util.NormPath(filepath.Join(dst, f.Name))
 		if err != nil {
 			return err
 		}
+
+		if err := ensureFilePathBelongsToDir(dst, filen); err != nil {
+			return fmt.Errorf("unable to extract zip archive %s: %w", zipPath, err)
+		}
+
 		stat, err := os.Stat(filen)
 		if os.IsNotExist(err) {
 			continue
@@ -151,12 +207,12 @@
 			// it's ok if directories already exist
 			continue
 		}
-		return fmt.Errorf("file exists: %s", filen)
+		return fmt.Errorf("unable to extract zip archive %s: file %s is already exists", zipPath, filen)
 	}
 
 	// Create files.
 	for _, f := range zr.File {
-		filen, err := util.NormPath(util.SanitizePath(filepath.Join(dst, f.Name)))
+		filen, err := util.NormPath(filepath.Join(dst, f.Name))
 		if err != nil {
 			return err
 		}
@@ -240,6 +296,11 @@
 		if err != nil {
 			return err
 		}
+
+		if err := ensureFilePathBelongsToDir(dst, filen); err != nil {
+			return err
+		}
+
 		stat, err := os.Stat(filen)
 		if os.IsNotExist(err) {
 			continue
@@ -262,7 +323,7 @@
 		return err
 	}
 	defer file.Close()
 
 	decompressed, err := decompress(file, archiveType)
 	if err != nil {
 		return err
@@ -270,7 +331,7 @@
 	tr := tar.NewReader(decompressed)
 
 	if err := checkForConflicts(tr, dst); err != nil {
-		return err
+		return fmt.Errorf("unable to extract tar archive %s: %s", tarName, err)
 	}
 
 	file.Seek(0, 0)
@@ -289,10 +350,15 @@
 		if err != nil {
 			return err
 		}
-		filen, err := util.NormPath(filepath.Join(dst, util.SanitizePath(header.Name)))
+		filen, err := util.NormPath(filepath.Join(dst, header.Name))
 		if err != nil {
 			return err
 		}
+
+		if err := ensureFilePathBelongsToDir(dst, filen); err != nil {
+			return err
+		}
+
 		filedir := filepath.Dir(filen)
 
 		if err := os.MkdirAll(filedir, 0700); err != nil {
@@ -324,11 +390,23 @@
 				return err
 			}
 		case tar.TypeLink:
+			if err := ensureSymlinkBelongsToDir(dst, header.Linkname); err != nil {
+				clog.Infof(ctx,
+					"link %s resolved outside of destination %s, for the security reason it is not allowed", header.Linkname, dst)
+				continue
+			}
+
 			if err := os.Link(header.Linkname, filen); err != nil {
 				return err
 			}
 			continue
 		case tar.TypeSymlink:
+			if err := ensureSymlinkBelongsToDir(dst, header.Linkname); err != nil {
+				clog.Infof(ctx,
+					"symlink %s resolved outside of destination %s, for the security reason it is not allowed", header.Linkname, dst)
+				continue
+			}
+
 			if err := os.Symlink(header.Linkname, filen); err != nil {
 				return err
 			}
@@ -506,7 +584,7 @@
 	return err
 }
 
-func chown(file string, uid, gid int) error {
+func chownFunc(file string, uid, gid int) error {
 	// os.Chown unsupported on windows
 	if runtime.GOOS == "windows" {
 		return nil