From b2f2c5a02a598bae05926a953109865ca1fbbed6 Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Wed, 21 Feb 2024 12:25:00 -0600 Subject: [PATCH 1/4] :fix: toctou for files - Fixes https://github.com/defenseunicorns/uds-cli/issues/440 Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- go.mod | 2 +- src/pkg/bundle/tarball.go | 18 ++++++++- src/pkg/utils/utils.go | 28 ++++++++++++++ src/pkg/utils/utils_test.go | 77 +++++++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 src/pkg/utils/utils_test.go diff --git a/go.mod b/go.mod index 8a55d622..7aea38ef 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/AlecAivazis/survey/v2 v2.3.7 github.com/alecthomas/jsonschema v0.0.0-20220216202328-9eeeec9d044b github.com/defenseunicorns/zarf v0.32.3 + github.com/fsnotify/fsnotify v1.7.0 github.com/goccy/go-yaml v1.11.3 github.com/mholt/archiver/v3 v3.5.1 github.com/mholt/archiver/v4 v4.0.0-alpha.8 @@ -197,7 +198,6 @@ require ( github.com/fluxcd/pkg/apis/kustomize v1.3.0 // indirect github.com/fluxcd/pkg/apis/meta v1.3.0 // indirect github.com/fluxcd/source-controller/api v1.2.4 // indirect - github.com/fsnotify/fsnotify v1.7.0 // indirect github.com/fvbommel/sortorder v1.1.0 // indirect github.com/gabriel-vasile/mimetype v1.4.3 // indirect github.com/gdamore/encoding v1.0.0 // indirect diff --git a/src/pkg/bundle/tarball.go b/src/pkg/bundle/tarball.go index f7b53dd3..1b063519 100644 --- a/src/pkg/bundle/tarball.go +++ b/src/pkg/bundle/tarball.go @@ -121,8 +121,11 @@ func (tp *tarballBundleProvider) getBundleManifest() error { if err := av3.Extract(tp.src, "index.json", tp.dst); err != nil { return fmt.Errorf("failed to extract index.json from %s: %w", tp.src, err) } - indexPath := filepath.Join(tp.dst, "index.json") + indexPathChecksum, err := utils.CalculateFileChecksum(indexPath) + if err != nil { + return fmt.Errorf("failed to calculate checksum for %s: %w", indexPath, err) + } defer os.Remove(indexPath) @@ -136,7 +139,13 @@ func (tp *tarballBundleProvider) getBundleManifest() error { if err := json.Unmarshal(b, &index); err != nil { return err } - + // The check is done after json.Unmarshal to ensure that the file wasn't tampered after written to the temp dir + // while reading the file + // Verify the calculated checksum against the expected checksum + isValid, err := utils.VerifyFileChecksum(indexPath, indexPathChecksum) + if err != nil || !isValid { + return fmt.Errorf("checksum verification failed for %s", indexPath) + } // local bundles only have one manifest entry in their index.json bundleManifestDesc := index.Manifests[0] tp.bundleRootDesc = bundleManifestDesc @@ -169,6 +178,11 @@ func (tp *tarballBundleProvider) getBundleManifest() error { if err := json.Unmarshal(b, &manifest); err != nil { return err } + // This is to ensure that the file wasn't tampered after written to the temp dir + isValid, err = utils.VerifyFileChecksum(manifestPath, bundleManifestDesc.Digest.Encoded()) + if err != nil || !isValid { + return fmt.Errorf("checksum verification failed for %s", manifestPath) + } tp.bundleRootManifest = manifest return nil diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 344a65c3..6e1b8088 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -6,6 +6,8 @@ package utils import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "io" @@ -120,3 +122,29 @@ func ToLocalFile(t any, filePath string) error { func IsRemotePkg(pkg types.Package) bool { return pkg.Repository != "" } + +// CalculateFileChecksum calculates the SHA-256 checksum of a file. +func CalculateFileChecksum(filePath string) (string, error) { + file, err := os.Open(filePath) + if err != nil { + return "", fmt.Errorf("error opening file: %w", err) + } + defer file.Close() + + hasher := sha256.New() + if _, err := io.Copy(hasher, file); err != nil { + return "", fmt.Errorf("error calculating checksum: %w", err) + } + + return hex.EncodeToString(hasher.Sum(nil)), nil +} + +// VerifyFileChecksum compares the calculated checksum of a file against a provided checksum. +func VerifyFileChecksum(filePath, expectedChecksum string) (bool, error) { + calculatedChecksum, err := CalculateFileChecksum(filePath) + if err != nil { + return false, fmt.Errorf("error calculating checksum: %w", err) + } + + return calculatedChecksum == expectedChecksum, nil +} diff --git a/src/pkg/utils/utils_test.go b/src/pkg/utils/utils_test.go new file mode 100644 index 00000000..2eba7791 --- /dev/null +++ b/src/pkg/utils/utils_test.go @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2023-Present The UDS Authors + +// Package utils provides utility fns for UDS-CLI +package utils + +import ( + "os" + "testing" +) + +func TestCalculateFileChecksum(t *testing.T) { + // Create a temporary file + tmpFile, err := os.CreateTemp("", "testfile-") + if err != nil { + t.Fatalf("Unable to create temporary file: %v", err) + } + defer os.Remove(tmpFile.Name()) // clean up + + // Write some content to the file + content := []byte("hello world") + if _, err := tmpFile.Write(content); err != nil { + t.Fatalf("Unable to write to temporary file: %v", err) + } + tmpFile.Close() + + // Expected checksum of "hello world" + expectedChecksum := "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" + + checksum, err := CalculateFileChecksum(tmpFile.Name()) + if err != nil { + t.Errorf("CalculateFileChecksum returned an error: %v", err) + } + + if checksum != expectedChecksum { + t.Errorf("Expected checksum %s, got %s", expectedChecksum, checksum) + } +} + +func TestVerifyFileChecksum(t *testing.T) { + // Create a temporary file + tmpFile, err := os.CreateTemp("", "testfile-") + if err != nil { + t.Fatalf("Unable to create temporary file: %v", err) + } + defer os.Remove(tmpFile.Name()) // clean up + + // Write some content to the file + content := []byte("hello world") + if _, err := tmpFile.Write(content); err != nil { + t.Fatalf("Unable to write to temporary file: %v", err) + } + tmpFile.Close() + + // Expected checksum of "hello world" + expectedChecksum := "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" + + valid, err := VerifyFileChecksum(tmpFile.Name(), expectedChecksum) + if err != nil { + t.Errorf("VerifyFileChecksum returned an error: %v", err) + } + + if !valid { + t.Errorf("Expected checksum verification to be true, got false") + } + + // Test with incorrect checksum + invalidChecksum := "invalidchecksum" + valid, err = VerifyFileChecksum(tmpFile.Name(), invalidChecksum) + if err != nil { + t.Errorf("Didn't expect an error for incorrect checksum, got %v", err) + } + + if valid { + t.Errorf("Expected checksum verification to be false, got true") + } +} From d6221f6942a0dc318dabb8277d8b6fc6304c9bab Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Thu, 22 Feb 2024 11:10:03 -0600 Subject: [PATCH 2/4] Included a secure temp dir to reduce the attack vector Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- src/pkg/bundle/tarball.go | 12 +++++++++--- src/pkg/utils/utils.go | 13 +++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/pkg/bundle/tarball.go b/src/pkg/bundle/tarball.go index 1b063519..32444b9c 100644 --- a/src/pkg/bundle/tarball.go +++ b/src/pkg/bundle/tarball.go @@ -40,7 +40,7 @@ func (tp *tarballBundleProvider) CreateBundleSBOM(extractSBOM bool) error { return err } // make tmp dir for pkg SBOM extraction - err = os.Mkdir(filepath.Join(tp.dst, config.BundleSBOM), 0700) + err = os.Mkdir(filepath.Join(tp.dst, config.BundleSBOM), 0o700) if err != nil { return err } @@ -117,11 +117,17 @@ func (tp *tarballBundleProvider) getBundleManifest() error { if tp.bundleRootManifest != nil { return nil } + // Create a secure temporary directory for handling files + secureTempDir, err := utils.CreateSecureTempDir() + if err != nil { + return fmt.Errorf("failed to create a secure temporary directory: %w", err) + } + defer os.RemoveAll(secureTempDir) // Ensure cleanup of the temp directory if err := av3.Extract(tp.src, "index.json", tp.dst); err != nil { return fmt.Errorf("failed to extract index.json from %s: %w", tp.src, err) } - indexPath := filepath.Join(tp.dst, "index.json") + indexPath := filepath.Join(secureTempDir, "index.json") indexPathChecksum, err := utils.CalculateFileChecksum(indexPath) if err != nil { return fmt.Errorf("failed to calculate checksum for %s: %w", indexPath, err) @@ -160,7 +166,7 @@ func (tp *tarballBundleProvider) getBundleManifest() error { return fmt.Errorf("failed to extract %s from %s: %w", bundleManifestDesc.Digest.Encoded(), tp.src, err) } - manifestPath := filepath.Join(tp.dst, manifestRelativePath) + manifestPath := filepath.Join(secureTempDir, manifestRelativePath) defer os.Remove(manifestPath) diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 6e1b8088..15697a16 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -148,3 +148,16 @@ func VerifyFileChecksum(filePath, expectedChecksum string) (bool, error) { return calculatedChecksum == expectedChecksum, nil } + +// CreateSecureTempDir creates a secure, randomly named subdirectory with restricted permissions. +func CreateSecureTempDir() (string, error) { + tempDir := os.TempDir() + + // Restrict permissions to the directory + // the permissions are being restricted to 0700 for security reasons + if err := os.Chmod(tempDir, 0o700); err != nil { + os.Remove(tempDir) // Cleanup on error + return "", fmt.Errorf("failed to set permissions for the temp directory: %w", err) + } + return tempDir, nil +} From 2585f0c56abf3001ee6b1766d775bd1abd4452e7 Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Thu, 22 Feb 2024 12:04:20 -0600 Subject: [PATCH 3/4] Fixed the tempdir issue Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- src/pkg/utils/utils.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 15697a16..3ca378f8 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -151,13 +151,16 @@ func VerifyFileChecksum(filePath, expectedChecksum string) (bool, error) { // CreateSecureTempDir creates a secure, randomly named subdirectory with restricted permissions. func CreateSecureTempDir() (string, error) { - tempDir := os.TempDir() - // Restrict permissions to the directory // the permissions are being restricted to 0700 for security reasons - if err := os.Chmod(tempDir, 0o700); err != nil { - os.Remove(tempDir) // Cleanup on error + // create a subdirectory with a random name + secureTempDir, err := os.MkdirTemp(os.TempDir(), "uds-*") + if err != nil { + return "", fmt.Errorf("failed to create a secure temp directory: %w", err) + } + if err := os.Chmod(secureTempDir, 0o700); err != nil { + os.Remove(secureTempDir) // Cleanup on error return "", fmt.Errorf("failed to set permissions for the temp directory: %w", err) } - return tempDir, nil + return secureTempDir, nil } From 8c295f8d23c5ead9843bdc1bf7a04143c4c8707e Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Thu, 22 Feb 2024 15:15:06 -0600 Subject: [PATCH 4/4] fixes based on code review comments. Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- src/pkg/bundle/tarball.go | 26 +++---------- src/pkg/utils/utils.go | 44 --------------------- src/pkg/utils/utils_test.go | 77 ------------------------------------- 3 files changed, 5 insertions(+), 142 deletions(-) delete mode 100644 src/pkg/utils/utils_test.go diff --git a/src/pkg/bundle/tarball.go b/src/pkg/bundle/tarball.go index 32444b9c..360b4f41 100644 --- a/src/pkg/bundle/tarball.go +++ b/src/pkg/bundle/tarball.go @@ -118,39 +118,28 @@ func (tp *tarballBundleProvider) getBundleManifest() error { return nil } // Create a secure temporary directory for handling files - secureTempDir, err := utils.CreateSecureTempDir() + secureTempDir, err := zarfUtils.MakeTempDir(config.CommonOptions.TempDirectory) if err != nil { return fmt.Errorf("failed to create a secure temporary directory: %w", err) } defer os.RemoveAll(secureTempDir) // Ensure cleanup of the temp directory - if err := av3.Extract(tp.src, "index.json", tp.dst); err != nil { + if err := av3.Extract(tp.src, "index.json", secureTempDir); err != nil { return fmt.Errorf("failed to extract index.json from %s: %w", tp.src, err) } indexPath := filepath.Join(secureTempDir, "index.json") - indexPathChecksum, err := utils.CalculateFileChecksum(indexPath) - if err != nil { - return fmt.Errorf("failed to calculate checksum for %s: %w", indexPath, err) - } defer os.Remove(indexPath) b, err := os.ReadFile(indexPath) if err != nil { - return err + return fmt.Errorf("failed to read index.json: %w", err) } var index ocispec.Index if err := json.Unmarshal(b, &index); err != nil { - return err - } - // The check is done after json.Unmarshal to ensure that the file wasn't tampered after written to the temp dir - // while reading the file - // Verify the calculated checksum against the expected checksum - isValid, err := utils.VerifyFileChecksum(indexPath, indexPathChecksum) - if err != nil || !isValid { - return fmt.Errorf("checksum verification failed for %s", indexPath) + return fmt.Errorf("failed to unmarshal index.json: %w", err) } // local bundles only have one manifest entry in their index.json bundleManifestDesc := index.Manifests[0] @@ -162,7 +151,7 @@ func (tp *tarballBundleProvider) getBundleManifest() error { manifestRelativePath := filepath.Join(config.BlobsDir, bundleManifestDesc.Digest.Encoded()) - if err := av3.Extract(tp.src, manifestRelativePath, tp.dst); err != nil { + if err := av3.Extract(tp.src, manifestRelativePath, secureTempDir); err != nil { return fmt.Errorf("failed to extract %s from %s: %w", bundleManifestDesc.Digest.Encoded(), tp.src, err) } @@ -184,11 +173,6 @@ func (tp *tarballBundleProvider) getBundleManifest() error { if err := json.Unmarshal(b, &manifest); err != nil { return err } - // This is to ensure that the file wasn't tampered after written to the temp dir - isValid, err = utils.VerifyFileChecksum(manifestPath, bundleManifestDesc.Digest.Encoded()) - if err != nil || !isValid { - return fmt.Errorf("checksum verification failed for %s", manifestPath) - } tp.bundleRootManifest = manifest return nil diff --git a/src/pkg/utils/utils.go b/src/pkg/utils/utils.go index 3ca378f8..344a65c3 100644 --- a/src/pkg/utils/utils.go +++ b/src/pkg/utils/utils.go @@ -6,8 +6,6 @@ package utils import ( "context" - "crypto/sha256" - "encoding/hex" "encoding/json" "fmt" "io" @@ -122,45 +120,3 @@ func ToLocalFile(t any, filePath string) error { func IsRemotePkg(pkg types.Package) bool { return pkg.Repository != "" } - -// CalculateFileChecksum calculates the SHA-256 checksum of a file. -func CalculateFileChecksum(filePath string) (string, error) { - file, err := os.Open(filePath) - if err != nil { - return "", fmt.Errorf("error opening file: %w", err) - } - defer file.Close() - - hasher := sha256.New() - if _, err := io.Copy(hasher, file); err != nil { - return "", fmt.Errorf("error calculating checksum: %w", err) - } - - return hex.EncodeToString(hasher.Sum(nil)), nil -} - -// VerifyFileChecksum compares the calculated checksum of a file against a provided checksum. -func VerifyFileChecksum(filePath, expectedChecksum string) (bool, error) { - calculatedChecksum, err := CalculateFileChecksum(filePath) - if err != nil { - return false, fmt.Errorf("error calculating checksum: %w", err) - } - - return calculatedChecksum == expectedChecksum, nil -} - -// CreateSecureTempDir creates a secure, randomly named subdirectory with restricted permissions. -func CreateSecureTempDir() (string, error) { - // Restrict permissions to the directory - // the permissions are being restricted to 0700 for security reasons - // create a subdirectory with a random name - secureTempDir, err := os.MkdirTemp(os.TempDir(), "uds-*") - if err != nil { - return "", fmt.Errorf("failed to create a secure temp directory: %w", err) - } - if err := os.Chmod(secureTempDir, 0o700); err != nil { - os.Remove(secureTempDir) // Cleanup on error - return "", fmt.Errorf("failed to set permissions for the temp directory: %w", err) - } - return secureTempDir, nil -} diff --git a/src/pkg/utils/utils_test.go b/src/pkg/utils/utils_test.go deleted file mode 100644 index 2eba7791..00000000 --- a/src/pkg/utils/utils_test.go +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// SPDX-FileCopyrightText: 2023-Present The UDS Authors - -// Package utils provides utility fns for UDS-CLI -package utils - -import ( - "os" - "testing" -) - -func TestCalculateFileChecksum(t *testing.T) { - // Create a temporary file - tmpFile, err := os.CreateTemp("", "testfile-") - if err != nil { - t.Fatalf("Unable to create temporary file: %v", err) - } - defer os.Remove(tmpFile.Name()) // clean up - - // Write some content to the file - content := []byte("hello world") - if _, err := tmpFile.Write(content); err != nil { - t.Fatalf("Unable to write to temporary file: %v", err) - } - tmpFile.Close() - - // Expected checksum of "hello world" - expectedChecksum := "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" - - checksum, err := CalculateFileChecksum(tmpFile.Name()) - if err != nil { - t.Errorf("CalculateFileChecksum returned an error: %v", err) - } - - if checksum != expectedChecksum { - t.Errorf("Expected checksum %s, got %s", expectedChecksum, checksum) - } -} - -func TestVerifyFileChecksum(t *testing.T) { - // Create a temporary file - tmpFile, err := os.CreateTemp("", "testfile-") - if err != nil { - t.Fatalf("Unable to create temporary file: %v", err) - } - defer os.Remove(tmpFile.Name()) // clean up - - // Write some content to the file - content := []byte("hello world") - if _, err := tmpFile.Write(content); err != nil { - t.Fatalf("Unable to write to temporary file: %v", err) - } - tmpFile.Close() - - // Expected checksum of "hello world" - expectedChecksum := "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9" - - valid, err := VerifyFileChecksum(tmpFile.Name(), expectedChecksum) - if err != nil { - t.Errorf("VerifyFileChecksum returned an error: %v", err) - } - - if !valid { - t.Errorf("Expected checksum verification to be true, got false") - } - - // Test with incorrect checksum - invalidChecksum := "invalidchecksum" - valid, err = VerifyFileChecksum(tmpFile.Name(), invalidChecksum) - if err != nil { - t.Errorf("Didn't expect an error for incorrect checksum, got %v", err) - } - - if valid { - t.Errorf("Expected checksum verification to be false, got true") - } -}