JavaScript SDK: fix config/cache file permissions and secure the caching fallback

examples/javascript/custom-caching-function-support/.gitignore

@@ -1,3 +1,2 @@
 node_modules
-config.json
-cache.dat
+config.json

examples/javascript/custom-caching-function-support/hello.js

@@ -1,58 +1,22 @@
-const fs = require('fs');
-
 const {
     getSecrets,
     initializeStorage,
     localConfigStorage,
-    postFunction
+    createCachingFunction
 } = require('@keeper-security/secrets-manager-core')
 
-const CACHE_FILENAME = 'cache.dat';
-
-// This is basic example of creating custom caching function
-// ⓘ This will store only last request, however you can use any tool to extend this functionality
-// ⓘ Stale cache entries can cause version mismatches if records are updated from other keepersecurity utils. Prefer fresh reads
-
-const cachingPostFunction = async (url, transmissionKey, payload, allowUnverifiedCertificate) => {
-    try {
-        const response = await postFunction(
-            url,
-            transmissionKey,
-            payload,
-            allowUnverifiedCertificate
-        )
-
-        if (response.statusCode == 200) {
-            fs.writeFileSync(CACHE_FILENAME, Buffer.concat([transmissionKey.key, response.data]))
-        }
-
-        return response
-    } catch (e) {
-        console.error(e)
-        let cachedData
-        try {
-            cachedData = fs.readFileSync(CACHE_FILENAME)
-        } catch {
-        }
-        if (!cachedData) {
-            throw new Error('Cached value does not exist')
-        }
-        console.log('Using cached data')
-        transmissionKey.key = cachedData.slice(0, 32)
-        return {
-            statusCode: 200,
-            data: cachedData.slice(32),
-            headers: []
-        }
-    }
-}
+// This is a basic example of using the SDK's built-in caching function.
+// ⓘ createCachingFunction stores only the last successful request, but you can supply your own
+//   queryFunction to extend this behavior.
+// ⓘ Stale cache entries can cause version mismatches if records are updated from other keepersecurity
+//   utils. createCachingFunction rejects cache entries older than its maxCacheAgeMs (default 24h).
 
 const getKeeperRecords = async () => {
     const storage = localConfigStorage("config.json")
 
     const options = {
         storage,
-        queryFunction: cachingPostFunction
+        queryFunction: createCachingFunction(storage)
     }
 
     // if your Keeper Account is in other region than US, update the hostname accordingly

examples/javascript/custom-caching-function-support/package.json

@@ -8,6 +8,6 @@
     "run": "node hello.js"
   },
   "dependencies": {
-    "@keeper-security/secrets-manager-core": "17.3.0"
+    "@keeper-security/secrets-manager-core": "17.6.0"
   }
 }

sdk/javascript/packages/core/CHANGELOG.md

@@ -7,6 +7,9 @@
 - KSM-748 - Fixed `getSecrets()` silently dropping records created by Commander or the Vault UI inside shared folders. The SDK now uses the folder key to decrypt the record key for any flat record that has `innerFolderUid` set. This matches the behavior for records in `folders[].records[]`.
 - KSM-1035 - Fixed throttle retry jitter being two-sided, which could reduce a retry delay below the computed floor. Jitter is now one-sided (0 to +25%). The SDK also caps a server-supplied `retry_after` at 176s to prevent an arbitrarily long wait.
 - KSM-1128 - Bounded the server key-rotation retry in `postQuery`. When the server sends `{"error":"key"}`, the code retries at most 3 times before throwing a typed `KeeperError`, instead of retrying forever. Before storing a suggested `key_id`, the code validates its shape (positive integer) and its membership in the bundled key table (keys 7-18). An unsupported key id can no longer corrupt the configuration. The pinned custom-key path does not change.
+- KSM-1263 - Fixed config and cache file permissions not being re-applied on every write. `fs.openSync`'s mode argument only takes effect when a file is created, so a config or cache file that already existed with looser permissions kept them; permissions are now explicitly reset to 0600 after every write.
+- KSM-1265 - Security fix (CWE-312, CWE-345): the Node `cachingPostFunction` stored its AES transmission key in plaintext next to the ciphertext it protected, in a fixed path relative to the process's working directory, and restored it with no integrity check. Replaced it with `createCachingFunction(storage, cachePath?, maxCacheAgeMs?)`, matching the factory shape already used on the browser platform. The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. This is a breaking change to `cachingPostFunction`'s call signature; usage was limited to the opt-in caching example, which has been updated to use the new function.
+- KSM-1266 - Fixed `localConfigStorage` treating every config-read failure as "no config yet." Only a missing file (`ENOENT`) is treated that way now; permission errors and malformed JSON throw a `KeeperError` instead of silently starting fresh.
 - Maintenance: Updated `minimatch`, `@babel/core`, and `handlebars` dev dependencies.
 
 ## 17.5.0

sdk/javascript/packages/core/internal/quicktest.ts

@@ -10,7 +10,7 @@ import {
 import {nodePlatform} from '../src/node/nodePlatform';
 import {connectPlatform} from '../src/platform';
 import {inspect} from 'util';
-import {cachingPostFunction, localConfigStorage} from "../src/node";
+import {createCachingFunction, localConfigStorage} from "../src/node";
 
 process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'
 
@@ -26,7 +26,7 @@ async function test() {
     await initializeStorage(kvs, oneTimeToken)
     const options: SecretManagerOptions = {
         storage: kvs,
-        // queryFunction: cachingPostFunction
+        // queryFunction: createCachingFunction(kvs)
         allowUnverifiedCertificate: true
     }
     const { records } = await getSecrets(options)

sdk/javascript/packages/core/src/node/localConfigStorage.ts

@@ -1,5 +1,19 @@
 import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, platform, TransmissionKey, inMemoryStorage} from "../platform";
+import {KeeperError} from "../errors";
 import * as fs from 'fs';
+import * as os from 'os';
+import * as path from 'path';
+
+// Duplicated from keeper.ts's private KEY_APP_KEY - not exported there, so the literal is repeated here.
+const KEY_APP_KEY = 'appKey'
+const CACHE_KEY_LABEL = Buffer.from('KSM-cache-v1')
+const CACHE_FORMAT_VERSION = 0x02
+const DEFAULT_CACHE_PATH = path.join(os.homedir(), '.keeper', 'ksm-cache.dat')
+const DEFAULT_MAX_CACHE_AGE_MS = 24 * 60 * 60 * 1000
+
+// fs.openSync's mode argument is only honored when the file is created; it is a no-op on an
+// existing file, so permissions must be re-asserted after every write, not just the first one.
+const chmodSecure = (filePath: string) => fs.chmodSync(filePath, 0o600)
 
 export const localConfigStorage = (configName?: string): KeyValueStorage => {
 
@@ -9,8 +23,11 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => {
         }
         try {
             return JSON.parse(fs.readFileSync(configName).toString())
-        } catch (e) {
-            return {}
+        } catch (e: Error | any) {
+            if (e.code === 'ENOENT') {
+                return {}
+            }
+            throw new KeeperError(`Unable to read local config ${configName}: ${e.message}`)
         }
     }
 
@@ -21,13 +38,13 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => {
         if (!configName) {
             return
         }
-        // Create file with secure permissions (0600)
         const fd = fs.openSync(configName, 'w', 0o600)
         try {
             fs.writeSync(fd, JSON.stringify(storageData, null, 2))
         } finally {
             fs.closeSync(fd)
         }
+        chmodSecure(configName)
     }
 
     return {
@@ -51,37 +68,85 @@ export const localConfigStorage = (configName?: string): KeyValueStorage => {
     }
 }
 
-export const cachingPostFunction = async (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload): Promise<KeeperHttpResponse> => {
+const deriveCacheKey = (appKey: Uint8Array): Promise<Uint8Array> =>
+    platform.getHmacDigest('SHA256', appKey, CACHE_KEY_LABEL)
+
+const writeCacheFile = async (cachePath: string, cacheKey: Uint8Array, plaintext: Uint8Array): Promise<void> => {
+    fs.mkdirSync(path.dirname(cachePath), {recursive: true, mode: 0o700})
+    const encrypted = await platform.encryptWithKey(plaintext, cacheKey)
+    const header = Buffer.alloc(9)
+    header.writeUInt8(CACHE_FORMAT_VERSION, 0)
+    header.writeBigUInt64BE(BigInt(Date.now()), 1)
+    const fd = fs.openSync(cachePath, 'w', 0o600)
     try {
-        const response = await platform.post(url, payload.payload, {
-            PublicKeyId: transmissionKey.publicKeyId.toString(),
-            TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey),
-            Authorization: `Signature ${platform.bytesToBase64(payload.signature)}`
-        })
-        if (response.statusCode == 200) {
-            // Create cache file with secure permissions (0600)
-            const cacheFd = fs.openSync('cache.dat', 'w', 0o600)
-            try {
-                fs.writeSync(cacheFd, Buffer.concat([transmissionKey.key, response.data]))
-            } finally {
-                fs.closeSync(cacheFd)
-            }
+        fs.writeSync(fd, Buffer.concat([header, encrypted]))
+    } finally {
+        fs.closeSync(fd)
+    }
+    chmodSecure(cachePath)
+}
+
+const readCacheFile = async (cachePath: string, cacheKey: Uint8Array, maxCacheAgeMs: number): Promise<Uint8Array> => {
+    let raw: Buffer
+    try {
+        raw = fs.readFileSync(cachePath)
+    } catch (e: Error | any) {
+        if (e.code === 'ENOENT') {
+            throw new KeeperError('Cached value does not exist')
         }
-        return response
-    } catch (e) {
-        let cachedData
+        throw new KeeperError(`Unable to read cache file ${cachePath}: ${e.message}`)
+    }
+    if (raw.length < 9 || raw[0] !== CACHE_FORMAT_VERSION) {
+        throw new KeeperError(`Cache file ${cachePath} is not in a recognized format`)
+    }
+    const timestamp = Number(raw.readBigUInt64BE(1))
+    if (Date.now() - timestamp > maxCacheAgeMs) {
+        throw new KeeperError(`Cached value at ${cachePath} is stale (age exceeds ${maxCacheAgeMs}ms)`)
+    }
+    try {
+        return await platform.decryptWithKey(raw.subarray(9), cacheKey)
+    } catch (e: Error | any) {
+        throw new KeeperError(`Cache file ${cachePath} failed integrity check: ${e.message}`)
+    }
+}
+
+// Node counterpart to browser/localConfigStorage.ts's createCachingFunction - same factory shape
+// on both platforms. Replaces the old standalone cachingPostFunction, which kept the AES key in
+// plaintext beside the ciphertext it protected, in a fixed CWD-relative file, with no integrity
+// check on restore. Fixing all three requires access to the config (to derive a cache key that
+// isn't the transmission key itself) and a chosen cache location, so the factory shape - not the
+// old zero-argument function - is what the fix needs.
+export const createCachingFunction = (
+    storage: KeyValueStorage,
+    cachePath: string = DEFAULT_CACHE_PATH,
+    maxCacheAgeMs: number = DEFAULT_MAX_CACHE_AGE_MS
+): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload, allowUnverifiedCertificate?: boolean) => Promise<KeeperHttpResponse> =>
+    async (url, transmissionKey, payload, allowUnverifiedCertificate) => {
+        let response: KeeperHttpResponse
         try {
-            cachedData = fs.readFileSync('cache.dat')
-        } catch {
-        }
-        if (!cachedData) {
-            throw new Error('Cached value does not exist')
+            response = await platform.post(url, payload.payload, {
+                PublicKeyId: transmissionKey.publicKeyId.toString(),
+                TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey),
+                Authorization: `Signature ${platform.bytesToBase64(payload.signature)}`
+            }, allowUnverifiedCertificate)
+        } catch (e) {
+            const appKey = await storage.getBytes(KEY_APP_KEY)
+            if (!appKey) {
+                throw new KeeperError('Cached value does not exist')
+            }
+            const cachedData = await readCacheFile(cachePath, await deriveCacheKey(appKey), maxCacheAgeMs)
+            transmissionKey.key = cachedData.slice(0, 32)
+            return {
+                statusCode: 200,
+                data: cachedData.slice(32),
+                headers: []
+            }
         }
-        transmissionKey.key = cachedData.slice(0, 32)
-        return {
-            statusCode: 200,
-            data: cachedData.slice(32),
-            headers: []
+        if (response.statusCode == 200) {
+            const appKey = await storage.getBytes(KEY_APP_KEY)
+            if (appKey) {
+                await writeCacheFile(cachePath, await deriveCacheKey(appKey), Buffer.concat([transmissionKey.key, response.data]))
+            }
         }
+        return response
     }
-}