refactor: attempt to make CopyFile more secure

Redo the order-of-operations so that the destination file is nominally non-readable, and does not attempt to create the file until all info about the source file is in-hand. Also change the default permissions on the `giltDir` and `cacheDir` directories to owner-only. Fixes: Issue #149
retr0h · Feb 21, 2024 · a30f9b3 · a30f9b3
1 parent e0939ee
commit a30f9b3
Show file tree

Hide file tree

Showing 5 changed files with 16 additions and 7 deletions.
diff --git a/internal/repositories/repositories.go b/internal/repositories/repositories.go
@@ -64,7 +64,7 @@ func (r *Repositories) getCacheDir() (string, error) {
 
 	cacheDir := filepath.Join(giltDir, "cache")
 	if _, err := r.appFs.Stat(cacheDir); os.IsNotExist(err) {
-		if err := r.appFs.Mkdir(cacheDir, 0o755); err != nil {
+		if err := r.appFs.Mkdir(cacheDir, 0o700); err != nil {
 			return "", err
 		}
 	}

diff --git a/internal/repositories/repositories_public_test.go b/internal/repositories/repositories_public_test.go
@@ -148,7 +148,7 @@ func (suite *RepositoriesPublicTestSuite) TestOverlayDstDirExists() {
 
 func (suite *RepositoriesPublicTestSuite) TestOverlayErrorRemovingDstDir() {
 	suite.mockRepo.EXPECT().Clone(gomock.Any(), gomock.Any()).Return("", nil)
-	_ = suite.appFs.MkdirAll(filepath.Join(suite.giltDir, "cache"), 0o755)
+	_ = suite.appFs.MkdirAll(filepath.Join(suite.giltDir, "cache"), 0o700)
 	_ = suite.appFs.MkdirAll(suite.dstDir, 0o755)
 	// Replace the test FS with a read-only copy
 	suite.appFs = afero.NewReadOnlyFs(suite.appFs)

diff --git a/internal/repository/copy.go b/internal/repository/copy.go
@@ -58,33 +58,42 @@ func (r *Copy) CopyFile(
 		slog.String("dstFile", dst),
 	)
 
+	// Open the source file for reading, and record its metadata
 	in, err := r.appFs.Open(src)
 	if err != nil {
 		return err
 	}
 	defer func() { _ = in.Close() }()
 
+	si, err := r.appFs.Stat(src)
+	if err != nil {
+		return err
+	}
+
+	// Open dest file for writing; make it owner-only perms before putting
+	// anything in it
 	out, err := r.appFs.Create(dst)
 	if err != nil {
 		return err
 	}
 	defer func() { _ = out.Close() }()
 
-	_, err = io.Copy(out, in)
+	err = r.appFs.Chmod(dst, 0o600)
 	if err != nil {
 		return err
 	}
 
-	err = out.Sync()
+	_, err = io.Copy(out, in)
 	if err != nil {
 		return err
 	}
 
-	si, err := r.appFs.Stat(src)
+	err = out.Sync()
 	if err != nil {
 		return err
 	}
 
+	// All done; make the permissions match
 	err = r.appFs.Chmod(dst, si.Mode())
 	if err != nil {
 		return err

diff --git a/pkg/repositories/repositories.go b/pkg/repositories/repositories.go
@@ -90,7 +90,7 @@ func (r *Repositories) getGiltDir() (string, error) {
 		return "", err
 	}
 
-	if err := r.appFs.MkdirAll(dir, 0o755); err != nil {
+	if err := r.appFs.MkdirAll(dir, 0o700); err != nil {
 		return "", err
 	}
 

diff --git a/test/integration/test_cli.bats b/test/integration/test_cli.bats
@@ -34,7 +34,7 @@ setup() {
 	GILT_CLONED_REPO_1_DST_DIR=${GILT_TEST_BASE_TMP_DIR}/retr0h.ansible-etcd
 	GILT_CLONED_REPO_2_DST_DIR=${GILT_TEST_BASE_TMP_DIR}/retr0h.ansible-etcd-tag
 
-    mkdir -p ${GILT_DIR}
+	mkdir -p -m 700 ${GILT_DIR}
 	mkdir -p ${GILT_LIBRARY_DIR}
 	mkdir -p ${GILT_ROLES_DIR}
 	cp test/Giltfile.yaml ${GILT_TEST_BASE_TMP_DIR}/Giltfile.yaml