JavaScript SDK: fix config/cache file permissions and secure the caching fallback - #1129
Conversation
…ndling, and secure the caching fallback (KSM-1263, KSM-1265, KSM-1266) fs.openSync's mode argument only applies at file creation, so config and cache files that already existed with looser permissions kept them. Permissions are now re-applied on every write. localConfigStorage's readStorage treated every read failure as "no config yet," including permission errors and malformed JSON. Only a missing file is treated that way now; everything else throws a KeeperError. The Node cachingPostFunction stored its AES transmission key in plaintext beside the ciphertext it protected, in a fixed CWD-relative path, and restored it with no integrity check (CWE-312, CWE-345). 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, authenticated so tampering is rejected instead of trusted, bounded by a configurable freshness window, and located outside the working directory by default. This breaks cachingPostFunction's signature; the opt-in caching example has been updated to the new function.
stas-schaller
changed the title
…ample createCachingFunction defaults to ~/.keeper/ksm-cache.dat now, not a cache.dat file in the example's own working directory.
examples/javascript/custom-caching-function-support was out of scope for this release; reverting hello.js, package.json, and .gitignore back to the release branch tip.
…ore" Restores the custom-caching-function-support example update. Examples/javascript changes are in scope for a JavaScript SDK release; the earlier revert was based on an overly strict reading of the scope directive.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
Review summary
The permission re-assertion and config-error-handling fixes (KSM-1263, KSM-1266) look solid and are correctly tested. The caching rewrite (KSM-1265) is a real improvement over the previous plaintext-key-beside-ciphertext design, but I found one exploitable gap in the new crypto design, a CI trigger issue that means this PR's own new tests never actually ran, and a semver concern. Details below, ranked by severity.
Security findings
1. Freshness timestamp is unauthenticated, staleness check can be bypassed (Medium-High)
In writeCacheFile/readCacheFile (localConfigStorage.ts:74-111), the cache file layout is [9-byte header][AES-256-GCM ciphertext], where the header (1 version byte + 8-byte timestamp) is written in cleartext, outside the AEAD boundary. Only the ciphertext is authenticated; the timestamp used for the staleness check is not.
Verified with a proof of concept: flipping only the 8 timestamp bytes to a date far in the future, leaving the ciphertext untouched, causes a cache entry that is genuinely stale (past maxCacheAgeMs) to be served as fresh with statusCode 200 and the original plaintext intact, no error. Flipping a byte inside the ciphertext still correctly throws (failed integrity check), confirming the ciphertext itself is properly authenticated; only the freshness metadata is not.
This requires local write access to the cache file, the same trust boundary the fix already defends against via the 0600 permission. Given that prerequisite, an attacker can pin an old/rotated set of secrets as "fresh" indefinitely by rewriting 8 unauthenticated bytes, with no need for the cache key. Suggested fix: bind the header into the AEAD (pass it as GCM associated data, or prepend it to the plaintext before encryption) rather than storing it out of band.
2. Cache directory permissions are not re-asserted (Low, confirmed inconsistency)
writeCacheFile creates ~/.keeper via fs.mkdirSync(dir, {recursive: true, mode: 0o700}). Like openSync's mode argument (which this PR explicitly re-asserts for files via chmodSecure()), mkdirSync's mode is only honored at creation time; verified this empirically. If ~/.keeper pre-exists with looser bits, it stays that way. Impact is limited since the cache file itself is independently chmod'd to 0600, so contents stay protected, but a loose directory can leak the file's existence, size, and mtime to other local users.
3. No symlink protection, compounds #2 into a potential arbitrary-file-overwrite (Low-Medium, same threat actor as #2)
None of writeCacheFile, readCacheFile, or the config writeStorage use O_NOFOLLOW or an lstat pre-check. If an attacker with write access to ~/.keeper (same access level as #2) plants a symlink at ksm-cache.dat pointing elsewhere, the SDK will open, truncate, write, and chmod 0600 whatever that symlink points to.
4. A failed cache write aborts an otherwise-successful call (Low-Medium, design tradeoff)
The cache write on a successful response is not wrapped in try/catch, so if ~/.keeper is unwritable (disk full, permission race, read-only filesystem), a request that got a valid 200 from the server still throws. The new test "a write failure after a successful response propagates instead of being treated as a fallback trigger" documents this as intentional, but a best-effort cache write probably should not be able to fail an already-successful primary operation.
5. chmodSecure() runs after the write, not before (Low, theoretical)
If a config or cache file pre-exists with loose permissions, the new write briefly happens under the old mode before being chmod'd to 0600 afterward. All calls are synchronous with no event-loop yield, so the window is narrow, but chmod'ing before the write (when the file already exists) would close it entirely.
CI gap: the new tests never actually ran
.github/workflows/test.js.yml triggers only on pull_request: branches: [master]. This PR targets release/sdk/javascript/core/v17.6.0. Confirmed via the PR's checks: only Socket Security dependency scans ran, no JS test job at all. Running npm test manually shows all 71 tests across 7 suites pass, including the 6 new ones, so there is no evidence of an actual test failure, but the green PR status currently includes zero automated proof of that. The repo's own test.javascript.storage.*-kms.yml and test.ruby.yml workflows already trigger on their respective release/** branches; test.js.yml is missing the equivalent release/sdk/javascript/core/** pattern.
Breaking change shipped in a minor version bump
cachingPostFunction is removed entirely (not just re-signatured) between 17.5.0 and 17.6.0, a minor bump. The prior breaking change in this changelog, KSM-574 ("Replace Node.js Buffer with Browser-Compatible Alternative"), shipped as 16.6.3 to 17.0.0, a major bump. A consumer on ^17.5.0 will silently pull this breaking change on their next install.
Browser platform left with the same vulnerability
This PR does not touch src/browser/localConfigStorage.ts's createCachingFunction, which still has the pre-fix pattern: raw transmissionKey.key concatenated with response bytes, no dedicated encryption, no integrity check, no staleness bound. The comment claiming the new Node function "match[es] the factory shape already used on the browser platform" is only true about the closure shape, not the security properties, worth a follow-up ticket so it does not read as "browser is covered too." Separately, package.json's types field always points at dist/node/index.d.ts regardless of which bundle a consumer's browser field resolves to; a browser consumer's TypeScript would type-check createCachingFunction(storage, cachePath, maxCacheAgeMs) fine, but at runtime get the 1-arg browser implementation that silently ignores the extra arguments.
Not blocking this PR, but worth a heads-up: the same caching pattern (plaintext key beside ciphertext, CWD/env-relative path, no integrity check) exists unfixed in the Java/Kotlin, Python, .NET, and Ruby SDKs in this monorepo.
Test coverage gaps
- KSM-1263 claims both config and cache file permissions are re-asserted, but only the config-file path has a test; nothing asserts
writeCacheFilere-chmods a pre-existing, loosely-permissioned cache file. - Every test passes an explicit
cachePathinside an already-created temp directory. The real default path (~/.keeper/ksm-cache.dat) and thefs.mkdirSyncfirst-run/directory-creation behavior (including the permission gap in #2) are never exercised. - No test for either "no
appKeyin storage" branch: silently skipping the cache write on success, or throwingCached value does not existon the fallback path. - No test pins the default
maxCacheAgeMs(24h) value itself.
Minor
KEY_APP_KEY = 'appKey'is a hand-duplicated copy of a private constant inkeeper.ts(currently correct), with nothing but a comment guarding against drift.catch (e: Error | any)is unusual;Error | anycollapses toanyin TypeScript, so it is cosmetic, not a bug.- No migration note that old
cache.datfiles at the pre-fix CWD-relative path are orphaned after upgrading (harmless since the new code safely rejects old-format files, but the stale plaintext key is not cleaned up).
2 participants
Summary
Fixes three security and correctness issues in the JavaScript SDK core's Node config storage and its opt-in caching helper, and updates the caching example to demonstrate the fix.
Changes
Fixed
fs.openSync's mode argument only applies when a file is created, so a config or cache file that already existed with looser permissions kept them. Permissions are now re-applied to 0600 after every write.cachingPostFunctionstored its AES transmission key in plaintext beside the ciphertext it protected, in a path relative to the process's working directory, and restored it with no integrity check. Replaced it withcreateCachingFunction(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, authenticated so a tampered file is rejected instead of trusted, bounded by a configurable freshness window (default 24h), and located at~/.keeper/ksm-cache.datby default instead of the working directory.readStoragetreated every read failure as "no config yet," including permission errors and malformed JSON. Only a missing file (ENOENT) is treated that way now; everything else throwsKeeperError.Maintenance
examples/javascript/custom-caching-function-supportto usecreateCachingFunctioninstead of hand-rolling the same insecure pattern this PR fixes in the SDK.Security Impact
KSM-1265 closes CWE-312 (cleartext storage of the transmission key) and CWE-345 (accepting a cache with no integrity check) in the opt-in
cachingPostFunctionreference implementation. The default request path, with no customqueryFunctionconfigured, was never affected. The cache-encryption key is derived from the app key already present in the config, so reading the cache requires holding the config, not just the cache file, and the cache is authenticated (AES-256-GCM) so a modified file is rejected rather than served.Breaking Changes
cachingPostFunctionhas been removed and replaced withcreateCachingFunction(storage, cachePath?, maxCacheAgeMs?). No other callers in this repo referenced it.Related Issues