Snowflake JDBC uses insecure temporary credential cache file permissions

@@ -1,6 +1,7 @@
 package net.snowflake.client.config;
 
 import static net.snowflake.client.jdbc.SnowflakeUtil.convertSystemGetEnvToBooleanValue;
+import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows;
 import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetEnv;
 
 import com.fasterxml.jackson.dataformat.toml.TomlMapper;
@@ -18,7 +19,6 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Properties;
-import net.snowflake.client.core.Constants;
 import net.snowflake.client.core.SnowflakeJdbcInternalApi;
 import net.snowflake.client.jdbc.SnowflakeSQLException;
 import net.snowflake.client.log.SFLogger;
@@ -117,7 +117,7 @@ private static Map<String, Map> readParametersMap(Path configFilePath)
 
   private static void verifyFilePermissionSecure(Path configFilePath)
       throws IOException, SnowflakeSQLException {
-    if (Constants.getOS() != Constants.OS.WINDOWS) {
+    if (!isWindows()) {
       PosixFileAttributeView posixFileAttributeView =
           Files.getFileAttributeView(configFilePath, PosixFileAttributeView.class);
       if (!posixFileAttributeView.readAttributes().permissions().stream()

@@ -4,6 +4,7 @@
 
 package net.snowflake.client.core;
 
+import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows;
 import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetEnv;
 import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty;
 
@@ -23,7 +24,11 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.attribute.BasicFileAttributes;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
 import java.util.Date;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 import net.snowflake.client.log.SFLogger;
 import net.snowflake.client.log.SFLoggerFactory;
 
@@ -46,6 +51,8 @@ class FileCacheManager {
 
   private File cacheDir;
 
+  private boolean onlyOwnerPermissions = true;
+
   private FileCacheManager() {}
 
   static FileCacheManager builder() {
@@ -78,16 +85,29 @@ FileCacheManager setCacheFileLockExpirationInSeconds(long cacheFileLockExpiratio
     return this;
   }
 
+  FileCacheManager setOnlyOwnerPermissions(boolean onlyOwnerPermissions) {
+    this.onlyOwnerPermissions = onlyOwnerPermissions;
+    return this;
+  }
+
   /**
    * Override the cache file.
    *
    * @param newCacheFile a file object to override the default one.
    */
   void overrideCacheFile(File newCacheFile) {
+    if (!newCacheFile.exists()) {
+      logger.debug("Cache file doesn't exists. File: {}", newCacheFile);
+    }
+    if (onlyOwnerPermissions) {
+      FileUtil.throwWhenPermiossionDifferentThanReadWriteForOwner(
+          newCacheFile, "Override cache file");
+    } else {
+      FileUtil.logFileUsage(cacheFile, "Override cache file", false);
+    }
     this.cacheFile = newCacheFile;
     this.cacheDir = newCacheFile.getParentFile();
     this.baseCacheFileName = newCacheFile.getName();
-    FileUtil.logFileUsage(cacheFile, "Override cache file", true);
   }
 
   FileCacheManager build() {
@@ -116,15 +136,12 @@ FileCacheManager build() {
     } else {
       // use user home directory to store the cache file
       String homeDir = systemGetProperty("user.home");
-      if (homeDir == null) {
-        // use tmp dir if not exists.
-        homeDir = systemGetProperty("java.io.tmpdir");
-      } else {
+      if (homeDir != null) {
         // Checking if home directory is writable.
         File homeFile = new File(homeDir);
         if (!homeFile.canWrite()) {
-          logger.debug("Home directory not writeable, using tmpdir", false);
-          homeDir = systemGetProperty("java.io.tmpdir");
+          logger.debug("Home directory not writeable, skip using cache", false);
+          homeDir = null;
         }
       }
       if (homeDir == null) {
@@ -155,7 +172,16 @@ FileCacheManager build() {
       // If exists. the method returns false.
       // In this particular case, it doesn't matter as long as the file is
       // writable.
-      if (cacheFileTmp.createNewFile()) {
+      if (!cacheFileTmp.exists()) {
+        if (!isWindows() && onlyOwnerPermissions) {
+          Files.createFile(
+              cacheFileTmp.toPath(),
+              PosixFilePermissions.asFileAttribute(
+                  Stream.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)
+                      .collect(Collectors.toSet())));
+        } else {
+          Files.createFile(cacheFileTmp.toPath());
+        }
         logger.debug("Successfully created a cache file {}", cacheFileTmp);
       } else {
         logger.debug("Cache file already exists {}", cacheFileTmp);
@@ -165,7 +191,10 @@ FileCacheManager build() {
       this.cacheLockFile =
           new File(this.cacheFile.getParentFile(), this.baseCacheFileName + ".lck");
     } catch (IOException | SecurityException ex) {
-      logger.debug("Failed to touch the cache file. Ignored. {}", cacheFileTmp.getAbsoluteFile());
+      logger.info(
+          "Failed to touch the cache file: {}. Ignored. {}",
+          ex.getMessage(),
+          cacheFileTmp.getAbsoluteFile());
     }
     return this;
   }
@@ -184,7 +213,13 @@ JsonNode readCacheFile() {
 
       try (Reader reader =
           new InputStreamReader(new FileInputStream(cacheFile), DEFAULT_FILE_ENCODING)) {
-        FileUtil.logFileUsage(cacheFile, "Read cache", false);
+
+        if (onlyOwnerPermissions) {
+          FileUtil.throwWhenPermiossionDifferentThanReadWriteForOwner(cacheFile, "Read cache");
+          FileUtil.throwWhenOwnerDifferentThanCurrentUser(cacheFile, "Read cache");
+        } else {
+          FileUtil.logFileUsage(cacheFile, "Read cache", false);
+        }
         return OBJECT_MAPPER.readTree(reader);
       }
     } catch (IOException ex) {
@@ -208,7 +243,11 @@ void writeCacheFile(JsonNode input) {
       }
       try (Writer writer =
           new OutputStreamWriter(new FileOutputStream(cacheFile), DEFAULT_FILE_ENCODING)) {
-        FileUtil.logFileUsage(cacheFile, "Write to cache", false);
+        if (onlyOwnerPermissions) {
+          FileUtil.throwWhenPermiossionDifferentThanReadWriteForOwner(cacheFile, "Write to cache");
+        } else {
+          FileUtil.logFileUsage(cacheFile, "Write to cache", false);
+        }
         writer.write(input.toString());
       }
     } catch (IOException ex) {

@@ -1,11 +1,14 @@
 package net.snowflake.client.core;
 
+import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows;
+
 import com.google.common.base.Strings;
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.nio.file.attribute.FileOwnerAttributeView;
 import java.nio.file.attribute.PosixFilePermission;
 import java.util.Arrays;
 import java.util.Collection;
@@ -19,9 +22,13 @@ public class FileUtil {
       Arrays.asList(PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE);
   private static final Collection<PosixFilePermission> READ_BY_OTHERS =
       Arrays.asList(PosixFilePermission.GROUP_READ, PosixFilePermission.OTHERS_READ);
+  private static final Collection<PosixFilePermission> EXECUTABLE =
+      Arrays.asList(
+          PosixFilePermission.OWNER_EXECUTE,
+          PosixFilePermission.GROUP_EXECUTE,
+          PosixFilePermission.OTHERS_EXECUTE);
 
   public static void logFileUsage(Path filePath, String context, boolean logReadAccess) {
-    logger.info("{}Accessing file: {}", getContextStr(context), filePath);
     logWarnWhenAccessibleByOthers(filePath, context, logReadAccess);
   }
 
@@ -34,10 +41,40 @@ public static void logFileUsage(String stringPath, String context, boolean logRe
     logFileUsage(path, context, logReadAccess);
   }
 
+  public static void throwWhenPermiossionDifferentThanReadWriteForOwner(File file, String context) {
+    throwWhenPermiossionDifferentThanReadWriteForOwner(file.toPath(), context);
+  }
+
+  public static void throwWhenPermiossionDifferentThanReadWriteForOwner(
+      Path filePath, String context) {
+    // we do not check the permissions for Windows
+    if (isWindows()) {
+      return;
+    }
+
+    try {
+      Collection<PosixFilePermission> filePermissions = Files.getPosixFilePermissions(filePath);
+      boolean isWritableByOthers = isPermPresent(filePermissions, WRITE_BY_OTHERS);
+      boolean isReadableByOthers = isPermPresent(filePermissions, READ_BY_OTHERS);
+      boolean isExecutable = isPermPresent(filePermissions, EXECUTABLE);
+
+      if (isWritableByOthers || isReadableByOthers || isExecutable) {
+        logger.debug(
+            "{}File {} access rights: {}", getContextStr(context), filePath, filePermissions);
+        throw new SecurityException(
+            String.format("Access to file %s is wider than allowed only to the owner", filePath));
+      }
+    } catch (IOException e) {
+      throw new SecurityException(
+          String.format(
+              "%s Unable to access the file to check the permissions. Error: %s", filePath, e));
+    }
+  }
+
   private static void logWarnWhenAccessibleByOthers(
       Path filePath, String context, boolean logReadAccess) {
     // we do not check the permissions for Windows
-    if (Constants.getOS() == Constants.OS.WINDOWS) {
+    if (isWindows()) {
       return;
     }
 
@@ -48,14 +85,16 @@ private static void logWarnWhenAccessibleByOthers(
 
       boolean isWritableByOthers = isPermPresent(filePermissions, WRITE_BY_OTHERS);
       boolean isReadableByOthers = isPermPresent(filePermissions, READ_BY_OTHERS);
+      boolean isExecutable = isPermPresent(filePermissions, EXECUTABLE);
 
-      if (isWritableByOthers || (isReadableByOthers && logReadAccess)) {
+      if (isWritableByOthers || (isReadableByOthers || isExecutable)) {
         logger.warn(
             "{}File {} is accessible by others to:{}{}",
             getContextStr(context),
             filePath,
             isReadableByOthers && logReadAccess ? " read" : "",
-            isWritableByOthers ? " write" : "");
+            isWritableByOthers ? " write" : "",
+            isExecutable ? " executable" : "");
       }
     } catch (IOException e) {
       logger.warn(
@@ -66,12 +105,43 @@ private static void logWarnWhenAccessibleByOthers(
     }
   }
 
+  public static void throwWhenOwnerDifferentThanCurrentUser(File file, String context) {
+    // we do not check the permissions for Windows
+    if (isWindows()) {
+      return;
+    }
+
+    Path filePath = Paths.get(file.getPath());
+
+    try {
+      String fileOwnerName = getFileOwnerName(filePath);
+      String currentUser = System.getProperty("user.name");
+      if (!currentUser.equalsIgnoreCase(fileOwnerName)) {
+        logger.debug(
+            "The file owner: {} is different than current user: {}", fileOwnerName, currentUser);
+        throw new SecurityException("The file owner is different than current user");
+      }
+    } catch (IOException e) {
+      logger.warn(
+          "{}Unable to access the file to check the owner: {}. Error: {}",
+          getContextStr(context),
+          filePath,
+          e);
+    }
+  }
+
   private static boolean isPermPresent(
       Collection<PosixFilePermission> filePerms, Collection<PosixFilePermission> permsToCheck)
       throws IOException {
     return filePerms.stream().anyMatch(permsToCheck::contains);
   }
 
+  static String getFileOwnerName(Path filePath) throws IOException {
+    FileOwnerAttributeView ownerAttributeView =
+        Files.getFileAttributeView(filePath, FileOwnerAttributeView.class);
+    return ownerAttributeView.getOwner().getName();
+  }
+
   private static String getContextStr(String context) {
     return Strings.isNullOrEmpty(context) ? "" : context + ": ";
   }

@@ -227,6 +227,7 @@ public class SFTrustManager extends X509ExtendedTrustManager {
             .setBaseCacheFileName(CACHE_FILE_NAME)
             .setCacheExpirationInSeconds(CACHE_EXPIRATION_IN_SECONDS)
             .setCacheFileLockExpirationInSeconds(CACHE_FILE_LOCK_EXPIRATION_IN_SECONDS)
+            .setOnlyOwnerPermissions(false)
             .build();
   }
 

@@ -2,6 +2,7 @@
 
 import static net.snowflake.client.core.SessionUtil.CLIENT_SFSQL;
 import static net.snowflake.client.core.SessionUtil.JVM_PARAMS_TO_PARAMS;
+import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows;
 import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty;
 
 import java.io.IOException;
@@ -278,7 +279,7 @@ private void createLogFolder(Path path) throws SnowflakeSQLLoggedException {
   }
 
   private void checkLogFolderPermissions(Path path) throws SnowflakeSQLLoggedException {
-    if (Constants.getOS() != Constants.OS.WINDOWS) {
+    if (!isWindows()) {
       try {
         Set<PosixFilePermission> folderPermissions = Files.getPosixFilePermissions(path);
         if (folderPermissions.contains(PosixFilePermission.GROUP_WRITE)

@@ -901,4 +901,14 @@ public static Map<String, String> createCaseInsensitiveMap(Header[] headers) {
       return new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
     }
   }
+
+  /**
+   * Check whether the OS is Windows
+   *
+   * @return boolean
+   */
+  @SnowflakeJdbcInternalApi
+  public static boolean isWindows() {
+    return Constants.getOS() == Constants.OS.WINDOWS;
+  }
 }

@@ -4,6 +4,7 @@
 import static net.snowflake.client.config.SFConnectionConfigParser.SKIP_TOKEN_FILE_PERMISSIONS_VERIFICATION;
 import static net.snowflake.client.config.SFConnectionConfigParser.SNOWFLAKE_DEFAULT_CONNECTION_NAME_KEY;
 import static net.snowflake.client.config.SFConnectionConfigParser.SNOWFLAKE_HOME_KEY;
+import static net.snowflake.client.jdbc.SnowflakeUtil.isWindows;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -25,7 +26,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import net.snowflake.client.core.Constants;
 import net.snowflake.client.jdbc.SnowflakeSQLException;
 import net.snowflake.client.jdbc.SnowflakeUtil;
 import org.junit.jupiter.api.AfterEach;
@@ -237,7 +237,7 @@ private void prepareConnectionConfigurationTomlFile(
 
   private Path createFilePathWithPermission(Path path, boolean onlyUserPermission)
       throws IOException {
-    if (Constants.getOS() != Constants.OS.WINDOWS) {
+    if (!isWindows()) {
       FileAttribute<Set<PosixFilePermission>> fileAttribute =
           onlyUserPermission
               ? PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------"))

@@ -0,0 +1,163 @@
+/*
+ * Copyright (c) 2012-2022 Snowflake Computing Inc. All rights reserved.
+ */
+
+package net.snowflake.client.core;
+
+import static net.snowflake.client.core.StmtUtil.mapper;
+import static net.snowflake.client.jdbc.SnowflakeUtil.systemGetProperty;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.isA;
+
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import net.snowflake.client.annotations.RunOnLinuxOrMac;
+import net.snowflake.client.category.TestTags;
+import net.snowflake.client.jdbc.BaseJDBCTest;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+@Nested
+@Tag(TestTags.CORE)
+class FileCacheManagerTest extends BaseJDBCTest {
+
+  private static final String CACHE_FILE_NAME = "temporary_credential.json";
+  private static final String CACHE_DIR_PROP = "net.snowflake.jdbc.temporaryCredentialCacheDir";
+  private static final String CACHE_DIR_ENV = "SF_TEMPORARY_CREDENTIAL_CACHE_DIR";
+  private static final long CACHE_EXPIRATION_IN_SECONDS = 86400L;
+  private static final long CACHE_FILE_LOCK_EXPIRATION_IN_SECONDS = 60L;
+
+  private FileCacheManager fileCacheManager;
+  private File cacheFile;
+
+  @BeforeEach
+  public void setup() throws IOException {
+    fileCacheManager =
+        FileCacheManager.builder()
+            .setCacheDirectorySystemProperty(CACHE_DIR_PROP)
+            .setCacheDirectoryEnvironmentVariable(CACHE_DIR_ENV)
+            .setBaseCacheFileName(CACHE_FILE_NAME)
+            .setCacheExpirationInSeconds(CACHE_EXPIRATION_IN_SECONDS)
+            .setCacheFileLockExpirationInSeconds(CACHE_FILE_LOCK_EXPIRATION_IN_SECONDS)
+            .build();
+    cacheFile = createCacheFile();
+  }
+
+  @AfterEach
+  public void clean() throws IOException {
+    if (Files.exists(cacheFile.toPath())) {
+      Files.delete(cacheFile.toPath());
+    }
+  }
+
+  @ParameterizedTest
+  @CsvSource({
+    "rwx------,false",
+    "rw-------,true",
+    "r-x------,false",
+    "r--------,true",
+    "rwxrwx---,false",
+    "rwxrw----,false",
+    "rwxr-x---,false",
+    "rwxr-----,false",
+    "rwx-wx---,false",
+    "rwx-w----,false",
+    "rwx--x---,false",
+    "rwx---rwx,false",
+    "rwx---rw-,false",
+    "rwx---r-x,false",
+    "rwx---r--,false",
+    "rwx----wx,false",
+    "rwx----w-,false",
+    "rwx-----x,false"
+  })
+  @RunOnLinuxOrMac
+  public void throwWhenReadCacheFileWithPermissionDifferentThanReadWriteForUserTest(
+      String permission, boolean isSucceed) throws IOException {
+    fileCacheManager.overrideCacheFile(cacheFile);
+    Files.setPosixFilePermissions(cacheFile.toPath(), PosixFilePermissions.fromString(permission));
+    if (isSucceed) {
+      assertDoesNotThrow(() -> fileCacheManager.readCacheFile());
+    } else {
+      SecurityException ex =
+          assertThrows(SecurityException.class, () -> fileCacheManager.readCacheFile());
+      assertTrue(ex.getMessage().contains("is wider than allowed only to the owner"));
+    }
+  }
+
+  @Test
+  @RunOnLinuxOrMac
+  public void throwWhenOverrideCacheFileHasDifferentOwnerThanCurrentUserTest() {
+    try (MockedStatic<FileUtil> fileUtilMock =
+        Mockito.mockStatic(FileUtil.class, Mockito.CALLS_REAL_METHODS)) {
+      fileUtilMock.when(() -> FileUtil.getFileOwnerName(isA(Path.class))).thenReturn("anotherUser");
+      SecurityException ex =
+          assertThrows(SecurityException.class, () -> fileCacheManager.readCacheFile());
+      assertTrue(ex.getMessage().contains("The file owner is different than current user"));
+    }
+  }
+
+  @Test
+  @RunOnLinuxOrMac
+  public void notThrowForToWidePermissionsWhenOnlyOwnerPermissionsSetFalseTest()
+      throws IOException {
+    fileCacheManager.setOnlyOwnerPermissions(false);
+    Files.setPosixFilePermissions(cacheFile.toPath(), PosixFilePermissions.fromString("rwxrwx---"));
+    assertDoesNotThrow(() -> fileCacheManager.readCacheFile());
+  }
+
+  @Test
+  @RunOnLinuxOrMac
+  public void throwWhenOverrideCacheFileNotFound() {
+    Path wrongPath =
+        Paths.get(systemGetProperty("user.home"), ".cache", "snowflake2", "wrongFileName");
+    SecurityException ex =
+        assertThrows(
+            SecurityException.class, () -> fileCacheManager.overrideCacheFile(wrongPath.toFile()));
+    assertTrue(
+        ex.getMessage()
+            .contains(
+                "Unable to access the file to check the permissions. Error: java.nio.file.NoSuchFileException:"));
+  }
+
+  private File createCacheFile() {
+    Path cacheFile =
+        Paths.get(systemGetProperty("user.home"), ".cache", "snowflake2", CACHE_FILE_NAME);
+    try {
+      if (Files.exists(cacheFile)) {
+        Files.delete(cacheFile);
+      }
+      Files.createDirectories(cacheFile.getParent());
+      Files.createFile(
+          cacheFile,
+          PosixFilePermissions.asFileAttribute(
+              Stream.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)
+                  .collect(Collectors.toSet())));
+      ObjectNode cacheContent = mapper.createObjectNode();
+      cacheContent.put("token", "tokenValue");
+      fileCacheManager.overrideCacheFile(cacheFile.toFile());
+      fileCacheManager.writeCacheFile(cacheContent);
+      return cacheFile.toFile();
+
+    } catch (IOException e) {
+      throw new RuntimeException(e);
+    }
+  }
+}