fix(snapshot azure): reject zip entries that would extract outside the temp dir (#1175)

* fix(snapshot azure): reject zip entries that would extract outside the temp dir

snapshot azure downloads each zip-deployed Web App's package and extracts
it by joining every entry name onto a temp directory, with no check that
the result stayed inside. An entry named "../../x" wrote outside the temp
directory, as the account running the snapshot, on every platform.

The S3 snapshot already guards this shape in localPathForS3Key. That rule
now lives in utils.LocalRelativePath so both paths, and any future place
an external name becomes a local path, apply the same containment check:
a segment resolving to ".." (including ".. " and "..." as Windows reads
them) is rejected, only a leading "/" is trimmed, and the remainder must
satisfy filepath.IsLocal. The zip path fails the snapshot naming the
offending entry rather than skipping it, so a tampered package is not
silently fingerprinted without part of its content.

* fix(snapshot azure): tolerate a "./" entry, pin symlink handling, export the rejection sentinels

utils.LocalRelativePath now returns ErrPathTraversal, ErrNamesNoFile and
ErrNotLocalPath so callers tell an attack apart from a harmless entry with
errors.Is rather than by matching message text. The messages are unchanged.

A "./" directory entry names the extraction directory itself and was a
no-op before the containment rule; it is skipped again instead of failing
the snapshot. A file entry that names no file still fails.

A symlink entry has always landed as a regular file because os.OpenFile
only honours permission bits; that is now explicit with Mode().Perm() and
pinned by a test, since name containment cannot survive a real symlink
that a later entry or the fingerprinter would follow out of the directory.

The unzip wrapper now wraps with %w. The app name was already prefixed by
the caller in GetAzureAppsData.

* fix(snapshot s3): keep the rejection sentinel through the object-key error

unusableS3KeyError now takes the reason as an error and wraps it with %w,
so errors.Is(err, utils.ErrPathTraversal) holds on the S3 path as it
already did through utils.ContainedPath. Message text is unchanged.

Also corrects the comment on the zip extraction open call: os.OpenFile
never creates a symlink whatever the type bits, so the choice of OpenFile
over os.Symlink is what keeps a symlink entry a regular file. What Perm()
adds is dropping setuid, setgid and sticky, which OpenFile does honour.

* fix(snapshot azure): name the entry on every extraction error and write entries with a constant mode

The rejection message now says the app cannot be reported until it is
redeployed without the entry: snapshot azure has no exclude flag and
.kosli_ignore is only read after extraction, so a changed package is the
only remedy. "..." and ".. " are legal names off Windows but the shared
rule still rejects them, so the message matters there too.

Every other extraction failure is wrapped with the entry name, and a file
"a" alongside an entry "a/b" gets the same actionable sentence the S3 path
gives an object and a prefix, instead of a bare ENOTDIR on a temp path.

Entries are written 0o600 rather than with the archive's permission bits.
The tree is deleted once fingerprinted and the fingerprint never reads a
mode, so this is fingerprint-identical; it keeps a zero-mode entry
readable, which previously failed later inside the fingerprinter, and
still drops setuid, setgid and sticky.

The two key-caused S3 rejections are package-level sentinels, matching
the utils style.

* fix(snapshot azure): say a rejected entry fails the whole snapshot, and name either side of a name collision

One app's error cancels the run in GetAzureAppsData, so a rejected entry
means no app in the environment is reported, not only the offending one.
The help text and the runtime error now say so.

A directory entry over an earlier file was reported as a parent problem,
and a file entry over an earlier directory fell through to a bare EISDIR
carrying the temp path. Both are checked up front with Lstat so the error
names the colliding entry, says which kind was there first, and fails the
same way on every platform. The parent-is-a-file case keeps its message.

* fix(snapshot azure): fail on two entries landing on one path, and on a write error surfacing at Close

O_EXCL replaces O_TRUNC so the second of two entries resolving to the same
path fails the snapshot, naming the entry, instead of silently replacing
the first. Duplicates are legal in a zip, and containment collapses "x",
"/x" and "./x" onto one path, so this was more reachable than before. It
mirrors errPathCollision on the S3 path: a package is never fingerprinted
without part of its content.

A destination Close error is now returned when Copy succeeded, since a
write error can surface only there and the entry would otherwise be
fingerprinted truncated.

Directories are created 0o700, consistent with the constant file mode.

* fix(snapshot azure): name the case-insensitive filesystem remedy on a path collision

Two entries differing only in case collide on macOS and Windows but not on
Linux, so the same package can snapshot from one machine and fail from
another. The operator has no flag to exclude an entry, so the error now
says to run the snapshot on a case-sensitive filesystem.

The escape-test table no longer indexes the last byte of an entry name,
which panicked on an empty name; that row is now in the table.

* fix(snapshot azure): name both entries of a path collision, and give the case-insensitivity advice only when it applies

unzip now records each extracted path with the entry that produced it, so
two entries resolving to one path fail with both names: "zip entry
[dir/./x]: resolves to the same local path as entry [dir/x]". That case
fails identically on every platform and the operator can see what to fix.

Only when no recorded entry resolves to the path, yet the file exists, is
the filesystem equating two names the package keeps distinct, as macOS
and Windows do for case. That branch alone advises running the snapshot
on a case-sensitive filesystem.

The escape test asserts the bracketed entry name, so the empty-name row
checks that the message still names the entry rather than passing
vacuously.

Author: mbevc1

cmd/kosli/snapshotAzureApps.go

@@ -24,6 +24,9 @@ will not match. See
 https://learn.microsoft.com/en-us/azure/azure-functions/functions-app-settings#website_run_from_package
 
 For zip-deployed apps, the fingerprint respects a ^.kosli_ignore^ file at the root of the deployed package.
+The package is extracted into a temporary directory. An entry whose name would resolve outside that directory
+(for example one containing a ^..^ segment) is never written; instead the whole snapshot fails and no app in
+the environment is reported until the offending app is redeployed without that entry.
 
 With ^--digests-source acr^, the registry is taken from each app's own container configuration. Azure
 credentials are only ever sent to an Azure Container Registry login server. An app whose image comes

internal/aws/aws.go

@@ -28,6 +28,7 @@ import (
 	"github.com/kosli-dev/cli/internal/digest"
 	"github.com/kosli-dev/cli/internal/filters"
 	"github.com/kosli-dev/cli/internal/logger"
+	"github.com/kosli-dev/cli/internal/utils"
 )
 
 // EcsEnvRequest represents the PUT request body to be sent to kosli from ECS
@@ -556,36 +557,28 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex
 }
 
 // localPathForS3Key turns an S3 object key into a path under the download
-// directory, or rejects it. A key holding a ".." segment resolves onto a path
-// it does not name, taking another key's place or leaving the directory.
+// directory, or rejects it. The containment rule is shared with every other
+// place an external name becomes a local path.
 func localPathForS3Key(key string) (string, error) {
-	// Windows separates on '\\' and drops trailing dots and spaces from a
-	// name, so ".. " and "..." resolve as ".." there.
-	segments := strings.FieldsFunc(key, func(r rune) bool { return r == '/' || r == '\\' })
-	for _, segment := range segments {
-		if strings.HasPrefix(segment, "..") && strings.TrimRight(segment, ". ") == "" {
-			return "", unusableS3KeyError(key, `contains a segment that resolves to ".."`)
-		}
-	}
-
-	// A leading '\\' is left for filepath.IsLocal: rooted on Windows, an
-	// ordinary filename elsewhere.
-	rel := strings.TrimLeft(key, "/")
-	if filepath.Clean(rel) == "." {
-		return "", unusableS3KeyError(key, "names no file")
-	}
-	if !filepath.IsLocal(rel) {
-		return "", unusableS3KeyError(key, "is not a local path")
+	rel, err := utils.LocalRelativePath(key)
+	if err != nil {
+		return "", unusableS3KeyError(key, err)
 	}
-
 	return rel, nil
 }
 
+// The key-caused rejections downloadFileFromBucket adds to those of
+// utils.LocalRelativePath.
+var (
+	errParentPrefixIsObject = errors.New("one of its parent prefixes has already been downloaded as an object")
+	errPathCollision        = errors.New("another object already downloaded to the same local path")
+)
+
 // unusableS3KeyError is only for failures the key itself causes. Advising
 // exclusion on a machine fault such as a full disk would drop a legitimate
 // object from the snapshot.
-func unusableS3KeyError(key, reason string) error {
-	return fmt.Errorf("object key [%s] cannot be stored as a local file: %s; exclude it with --exclude-regex, or narrow the include filter if one is set", key, reason)
+func unusableS3KeyError(key string, reason error) error {
+	return fmt.Errorf("object key [%s] cannot be stored as a local file: %w; exclude it with --exclude-regex, or narrow the include filter if one is set", key, reason)
 }
 
 func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket string, logger *logger.Logger) error {
@@ -597,7 +590,7 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	err = os.MkdirAll(filepath.Dir(dest), 0770)
 	if errors.Is(err, syscall.ENOTDIR) {
 		// Legal in S3, impossible on disk: an object "a" and a key under "a/".
-		return unusableS3KeyError(key, "one of its parent prefixes has already been downloaded as an object")
+		return unusableS3KeyError(key, errParentPrefixIsObject)
 	}
 	if err != nil {
 		return fmt.Errorf("object key [%s]: %w", key, err)
@@ -607,7 +600,7 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin
 	// "A/x" and "a/y" share one on a case-insensitive filesystem.
 	file, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
 	if errors.Is(err, fs.ErrExist) {
-		return unusableS3KeyError(key, "another object already downloaded to the same local path")
+		return unusableS3KeyError(key, errPathCollision)
 	}
 	if err != nil {
 		return fmt.Errorf("object key [%s]: %w", key, err)

internal/aws/aws_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/kosli-dev/cli/internal/filters"
 	"github.com/kosli-dev/cli/internal/logger"
 	"github.com/kosli-dev/cli/internal/testHelpers"
+	"github.com/kosli-dev/cli/internal/utils"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 )
@@ -1320,6 +1321,12 @@ func (suite *AWSTestSuite) TestLocalPathForS3Key() {
 	}
 }
 
+func (suite *AWSTestSuite) TestLocalPathForS3KeyKeepsTheRejectionSentinel() {
+	_, err := localPathForS3Key("uploads/../protected/release.bin")
+	require.ErrorIs(suite.T(), err, utils.ErrPathTraversal)
+	require.Contains(suite.T(), err.Error(), "object key [uploads/../protected/release.bin]")
+}
+
 func (suite *AWSTestSuite) TestDownloadFileFromBucketRefusesToOverwrite() {
 	tempDir := suite.T().TempDir()
 	preexisting := filepath.Join(tempDir, "README.md")

internal/azure/azure_apps.go

@@ -16,6 +16,7 @@ import (
 	"path/filepath"
 	"strings"
 	"sync"
+	"syscall"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
 	"github.com/Azure/azure-sdk-for-go/sdk/azidentity"
@@ -26,6 +27,7 @@ import (
 	"github.com/kosli-dev/cli/internal/digest"
 	"github.com/kosli-dev/cli/internal/logger"
 	"github.com/kosli-dev/cli/internal/server"
+	"github.com/kosli-dev/cli/internal/utils"
 )
 
 type AzureStaticCredentials struct {
@@ -274,7 +276,7 @@ func (azureClient *AzureClient) fingerprintZipService(app *armappservice.Site, l
 	destDir := filepath.Join(tmpDir, "extracted")
 	err = unzip(packagePath, destDir, logger)
 	if err != nil {
-		return AppData{}, fmt.Errorf("failed to unzip the downloaded package: %v", err)
+		return AppData{}, fmt.Errorf("failed to unzip the downloaded package: %w", err)
 	}
 
 	//  fingerprint the downloaded and unzipped package
@@ -329,53 +331,112 @@ func unzip(zipFile, destDir string, logger *logger.Logger) error {
 		}
 	}()
 
-	for _, f := range r.File {
-		filePath := filepath.Join(destDir, f.Name)
+	// Resolved path to the entry name that produced it, so a collision can name
+	// both sides.
+	extracted := make(map[string]string, len(r.File))
 
-		if f.FileInfo().IsDir() {
-			// Create directories
-			err := os.MkdirAll(filePath, os.ModePerm)
-			if err != nil {
-				return err
-			}
+	for _, f := range r.File {
+		// The entry name comes from the deployed package, which anyone able to
+		// deploy the app controls, so it must not be able to leave destDir.
+		filePath, err := utils.ContainedPath(destDir, f.Name)
+		if errors.Is(err, utils.ErrNamesNoFile) && f.FileInfo().IsDir() {
+			// A "./" entry names destDir itself; there is nothing to create.
 			continue
 		}
-
-		// Ensure the directory for the file exists
-		if err := os.MkdirAll(filepath.Dir(filePath), os.ModePerm); err != nil {
-			return err
+		if err != nil {
+			// One app's error cancels the whole run, snapshot azure has no exclude
+			// flag, and .kosli_ignore is only read after extraction, so the only
+			// remedy is a changed package.
+			return fmt.Errorf("zip entry %w; the package cannot be extracted safely, so no app in the environment is reported until this app is redeployed without that entry", err)
 		}
 
-		// Open the destination file
-		destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, f.Mode())
-		if err != nil {
-			return err
+		if err := extractZipEntry(f, filePath, extracted, logger); err != nil {
+			return fmt.Errorf("zip entry [%s]: %w", f.Name, err)
 		}
+	}
+	return nil
+}
 
-		// Open the source file within the ZIP archive
-		zipFile, err := f.Open()
-		if err != nil {
-			return err
+// extractZipEntry writes one entry to filePath, which the caller has already
+// checked stays inside the destination directory, and records it in extracted.
+func extractZipEntry(f *zip.File, filePath string, extracted map[string]string, logger *logger.Logger) error {
+	isDir := f.FileInfo().IsDir()
+
+	// Legal in a zip, impossible on disk: one name as both a file and a
+	// directory. Checked up front so the message names this entry rather than
+	// whichever filesystem call happens to fail, and fails the same way on
+	// every platform.
+	if existing, statErr := os.Lstat(filePath); statErr == nil && existing.IsDir() != isDir {
+		if existing.IsDir() {
+			return errors.New("was already extracted as a directory")
 		}
+		return errors.New("was already extracted as a file")
+	}
 
-		// Copy the file contents
-		_, err = io.Copy(destFile, zipFile)
+	dir := filePath
+	if !isDir {
+		dir = filepath.Dir(filePath)
+	}
+	err := os.MkdirAll(dir, 0o700)
+	if errors.Is(err, syscall.ENOTDIR) {
+		// A file "a" and an entry under "a/".
+		return errors.New("one of its parent directories was already extracted as a file")
+	}
+	if err != nil {
+		return err
+	}
+	if isDir {
+		return nil
+	}
+
+	// The tree is deleted once fingerprinted and the fingerprint never reads a
+	// mode, so a constant is safe: it keeps a zero-mode entry readable and
+	// drops setuid, setgid and sticky, which os.OpenFile would honour.
+	// Writing every entry through OpenFile, never os.Symlink, is what keeps a
+	// symlink entry from becoming a real symlink. Name containment does not
+	// survive one, since a later entry or the fingerprinter would follow it
+	// out of the destination.
+	// Legal in a zip, and containment collapses "x", "/x" and "./x" onto one
+	// path, but overwriting would fingerprint the package without the first
+	// entry. This fails the same way on every platform.
+	if first, dup := extracted[filePath]; dup {
+		return fmt.Errorf("resolves to the same local path as entry [%s]", first)
+	}
+	destFile, err := os.OpenFile(filePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600)
+	if errors.Is(err, os.ErrExist) {
+		// No recorded entry resolves here, so the filesystem itself equates two
+		// names: case, or Unicode form, on macOS and Windows. Moving the
+		// snapshot is the operator's only remedy.
+		return errors.New("collides with an earlier entry whose name this filesystem treats as the same, such as one differing only in case; run the snapshot on a case-sensitive filesystem")
+	}
+	if err != nil {
+		return err
+	}
+	extracted[filePath] = f.Name
 
-		// Close the open files
+	zipFile, err := f.Open()
+	if err != nil {
 		if closeErr := destFile.Close(); closeErr != nil {
-			// Log warning for cleanup error
 			logger.Warn("failed to close destination file %s: %v", filePath, closeErr)
 		}
-		if closeErr := zipFile.Close(); closeErr != nil {
-			// Log warning for cleanup error
-			logger.Warn("failed to close zip file: %v", closeErr)
-		}
+		return err
+	}
 
-		if err != nil {
-			return err
+	_, err = io.Copy(destFile, zipFile)
+
+	// A write error can surface only at Close, and an entry truncated that
+	// way would be fingerprinted as if it were complete.
+	if closeErr := destFile.Close(); closeErr != nil {
+		logger.Warn("failed to close destination file %s: %v", filePath, closeErr)
+		if err == nil {
+			err = closeErr
 		}
 	}
-	return nil
+	if closeErr := zipFile.Close(); closeErr != nil {
+		logger.Warn("failed to close zip file: %v", closeErr)
+	}
+
+	return err
 }
 
 func (azureClient *AzureClient) fingerprintDockerService(app *armappservice.Site, logger *logger.Logger, imageName string) (AppData, error) {