SNOW-1155452 Fix race condition on perm checking for easy logging (#1382)

Author: sfc-gh-pfus

client_configuration.go

@@ -7,7 +7,6 @@ import (
 	"os"
 	"path"
 	"path/filepath"
-	"runtime"
 	"strings"
 )
 
@@ -115,11 +114,9 @@ func parseClientConfiguration(filePath string) (*ClientConfig, error) {
 	if filePath == "" {
 		return nil, nil
 	}
-	fileContents, err := os.ReadFile(filePath)
-	if err != nil {
-		return nil, parsingClientConfigError(err)
-	}
-	err = validateCfgPerm(filePath)
+	// Check if group (5th LSB) or others (2nd LSB) have a write permission to the file
+	expectedPerm := os.FileMode(1<<4 | 1<<1)
+	fileContents, err := getFileContents(filePath, expectedPerm)
 	if err != nil {
 		return nil, parsingClientConfigError(err)
 	}
@@ -185,22 +182,6 @@ func validateLogLevel(clientConfig ClientConfig) error {
 	return nil
 }
 
-func validateCfgPerm(filePath string) error {
-	if runtime.GOOS == "windows" {
-		return nil
-	}
-	stat, err := os.Stat(filePath)
-	if err != nil {
-		return err
-	}
-	perm := stat.Mode()
-	// Check if group (5th LSB) or others (2nd LSB) have a write permission to the file
-	if perm&(1<<4) != 0 || perm&(1<<1) != 0 {
-		return fmt.Errorf("configuration file: %s can be modified by group or others", filePath)
-	}
-	return nil
-}
-
 func toLogLevel(logLevelString string) (string, error) {
 	var logLevel = strings.ToUpper(logLevelString)
 	switch logLevel {

client_configuration_test.go

@@ -298,6 +298,21 @@ func TestUnknownValues(t *testing.T) {
 	}
 }
 
+func TestConfigFileOpenSymlinkFail(t *testing.T) {
+	skipOnWindows(t, "file permission is different")
+	dir := t.TempDir()
+	configFilePath := createFile(t, defaultConfigName, "random content", dir)
+	symlinkFile := path.Join(dir, "test_symlink")
+	expectedErrMsg := "too many levels of symbolic links"
+
+	err := os.Symlink(configFilePath, symlinkFile)
+	assertNilF(t, err, "failed to create symlink")
+
+	_, err = getFileContents(symlinkFile, os.FileMode(1<<4|1<<1))
+	assertNotNilF(t, err, "should have blocked opening symlink")
+	assertTrueF(t, strings.Contains(err.Error(), expectedErrMsg))
+}
+
 func createFile(t *testing.T, fileName string, fileContents string, directory string) string {
 	fullFileName := path.Join(directory, fileName)
 	err := os.WriteFile(fullFileName, []byte(fileContents), 0644)

os_specific_posix.go

@@ -4,6 +4,7 @@ package gosnowflake
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"syscall"
 )
@@ -23,3 +24,40 @@ func provideOwnerFromStat(info os.FileInfo, filepath string) (uint32, error) {
 	}
 	return nativeStat.Uid, nil
 }
+
+func getFileContents(filePath string, expectedPerm os.FileMode) ([]byte, error) {
+	// open the file with read only and no symlink flags
+	file, err := os.OpenFile(filePath, syscall.O_RDONLY|syscall.O_NOFOLLOW, 0)
+	if err != nil {
+		return nil, err
+	}
+	defer file.Close()
+
+	// validate file permissions and owner
+	if err = validateFilePermissionBits(file, expectedPerm); err != nil {
+		return nil, err
+	}
+	if err = ensureFileOwner(file); err != nil {
+		return nil, err
+	}
+
+	// read the file
+	fileContents, err := io.ReadAll(file)
+	if err != nil {
+		return nil, err
+	}
+
+	return fileContents, nil
+}
+
+func validateFilePermissionBits(f *os.File, expectedPerm os.FileMode) error {
+	fileInfo, err := f.Stat()
+	if err != nil {
+		return err
+	}
+	filePerm := fileInfo.Mode()
+	if filePerm&expectedPerm != 0 {
+		return fmt.Errorf("incorrect permissions of %s", f.Name())
+	}
+	return nil
+}

os_specific_windows.go

@@ -10,3 +10,11 @@ import (
 func provideFileOwner(file *os.File) (uint32, error) {
 	return 0, errors.New("provideFileOwner is unsupported on windows")
 }
+
+func getFileContents(filePath string, expectedPerm os.FileMode) ([]byte, error) {
+	fileContents, err := os.ReadFile(filePath)
+	if err != nil {
+		return nil, err
+	}
+	return fileContents, nil
+}

permissions_test.go

@@ -4,10 +4,11 @@ package gosnowflake
 
 import (
 	"fmt"
-	"golang.org/x/sys/unix"
 	"os"
 	"path"
 	"testing"
+
+	"golang.org/x/sys/unix"
 )
 
 func TestConfigPermissions(t *testing.T) {
@@ -19,9 +20,6 @@ func TestConfigPermissions(t *testing.T) {
 		{filePerm: 0600, isValid: true},
 		{filePerm: 0500, isValid: true},
 		{filePerm: 0400, isValid: true},
-		{filePerm: 0300, isValid: true},
-		{filePerm: 0200, isValid: true},
-		{filePerm: 0100, isValid: true},
 		{filePerm: 0707, isValid: false},
 		{filePerm: 0706, isValid: false},
 		{filePerm: 0705, isValid: true},
@@ -47,7 +45,11 @@ func TestConfigPermissions(t *testing.T) {
 			err := os.WriteFile(tempFile, nil, os.FileMode(tc.filePerm))
 			assertNilE(t, err)
 			defer os.Remove(tempFile)
-			err = validateCfgPerm(tempFile)
+			f, err := os.Open(tempFile)
+			assertNilE(t, err)
+			defer f.Close()
+			expectedPerm := os.FileMode(1<<4 | 1<<1)
+			err = validateFilePermissionBits(f, expectedPerm)
 			if err != nil && tc.isValid {
 				t.Error(err)
 			}

secure_storage_manager.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/99designs/keyring"
 	"io"
 	"os"
 	"os/user"
@@ -16,6 +15,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"github.com/99designs/keyring"
 )
 
 type tokenType string
@@ -215,11 +216,19 @@ func (ssm *fileBasedSecureStorageManager) withCacheFile(action func(*os.File)) {
 		}
 	}(cacheDir)
 
-	if err := ssm.ensurePermissionsAndOwner(cacheFile, 0600); err != nil {
+	if err := ensureFileOwner(cacheFile); err != nil {
+		logger.Warnf("failed to ensure owner for temporary cache file. %v", err)
+		return
+	}
+	if err := ensureFilePermissions(cacheFile, 0600); err != nil {
 		logger.Warnf("failed to ensure permission for temporary cache file. %v", err)
 		return
 	}
-	if err := ssm.ensurePermissionsAndOwner(cacheDir, 0700|os.ModeDir); err != nil {
+	if err := ensureFileOwner(cacheDir); err != nil {
+		logger.Warnf("failed to ensure owner for temporary cache dir. %v", err)
+		return
+	}
+	if err := ensureFilePermissions(cacheDir, 0700|os.ModeDir); err != nil {
 		logger.Warnf("failed to ensure permission for temporary cache dir. %v", err)
 		return
 	}
@@ -334,16 +343,7 @@ func (ssm *fileBasedSecureStorageManager) credFilePath() string {
 	return filepath.Join(ssm.credDirPath, credCacheFileName)
 }
 
-func (ssm *fileBasedSecureStorageManager) ensurePermissionsAndOwner(f *os.File, expectedMode os.FileMode) error {
-	fileInfo, err := f.Stat()
-	if err != nil {
-		return err
-	}
-
-	if fileInfo.Mode().Perm() != expectedMode&os.ModePerm {
-		return fmt.Errorf("incorrect permissions(%v, expected %v) for credential file", fileInfo.Mode(), expectedMode)
-	}
-
+func ensureFileOwner(f *os.File) error {
 	ownerUID, err := provideFileOwner(f)
 	if err != nil && !errors.Is(err, os.ErrNotExist) {
 		return err
@@ -361,6 +361,17 @@ func (ssm *fileBasedSecureStorageManager) ensurePermissionsAndOwner(f *os.File,
 	return nil
 }
 
+func ensureFilePermissions(f *os.File, expectedMode os.FileMode) error {
+	fileInfo, err := f.Stat()
+	if err != nil {
+		return err
+	}
+	if fileInfo.Mode().Perm() != expectedMode&os.ModePerm {
+		return fmt.Errorf("incorrect permissions(%v, expected %v) for credential file", fileInfo.Mode(), expectedMode)
+	}
+	return nil
+}
+
 func (ssm *fileBasedSecureStorageManager) readTemporaryCacheFile(cacheFile *os.File) (map[string]any, error) {
 
 	jsonData, err := io.ReadAll(cacheFile)