From c575daaf0af57bf5cb8d7eac6070ad5b1c8fde92 Mon Sep 17 00:00:00 2001 From: doguhan Date: Thu, 3 Sep 2026 15:01:48 +0200 Subject: [PATCH] fix(android): per-call temp file to close concurrent verify->install race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadVerifiedFromUrl spawned a thread per call and every call shared one fixed temp path (verified-bundle.jsbundle.tmp). Under two overlapping calls, call B's delete+recreate at that path swapped the file out from under call A between A's streaming hash and A's renameTo(target): A's digest still matched its own (orphaned-inode) bytes, but the rename promoted B's partial/unverified file onto the canonical bundle — breaking the 'canonical path only ever holds verified bytes' invariant the temp-then-promote design establishes. Download to a per-call File.createTempFile temp instead, cleaned up in a finally. Each call now verifies and renames its own file, so the canonical path only ever receives a fully verified bundle regardless of concurrency. iOS is unaffected (it hashes the in-memory NSData and atomically writes those same bytes). Adds a regression test asserting distinct temps per call. --- README.md | 2 +- SECURITY.md | 2 +- .../BundleLoaderModule.java | 22 ++++++------ .../VerifyAndInstallTest.java | 34 ++++++++++++++++--- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 99c41a6..7788f88 100644 --- a/README.md +++ b/README.md @@ -69,7 +69,7 @@ Works on iOS and Android. After download and hash verification, the module: -1. Downloads to a temp file, verifies the SHA-256, then **atomically promotes** it to `Context.getCacheDir()/verified-bundle.jsbundle` — the canonical path never holds unverified or partial bytes. On a hash mismatch or download error the temp file is deleted and the current bundle is left untouched. +1. Downloads to a **per-call unique temp file** (`File.createTempFile` in `Context.getCacheDir()`), verifies the SHA-256, then **atomically promotes** it to `Context.getCacheDir()/verified-bundle.jsbundle` — the canonical path never holds unverified or partial bytes. A unique temp per call (rather than a shared fixed path) means two overlapping loads can never swap each other's file between verify and rename. On a hash mismatch or download error the temp file is deleted and the current bundle is left untouched. 2. Sets a one-shot flag in `SharedPreferences` (`"BundleLoader"` / `"pending_remote_bundle"`), using a synchronous `commit()` so the flag survives the imminent process kill. 3. Restarts the process via `startActivity` + `Process.killProcess`. diff --git a/SECURITY.md b/SECURITY.md index 9eb0da6..8188722 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -35,7 +35,7 @@ This library exists to load and execute a remote JavaScript bundle inside the ho ## Latest hardening - **Removed the unverified `load()` path** — the `load(url)` native methods (iOS + Android), the JS `load` export, and the `BundlePrompt` UI. Loading a remote bundle without native SHA-256 verification is an unauthenticated RCE primitive; only `loadVerified` remains. -- **Android verifies before install.** The download streams to a temp file; the verified bytes are atomically promoted (same-directory rename) to the canonical path only after the hash matches, and the temp is deleted on mismatch or download error — the canonical path never holds unverified or partial content. +- **Android verifies before install.** The download streams to a **per-call unique temp file** (`File.createTempFile`); the verified bytes are atomically promoted (same-directory rename) to the canonical path only after the hash matches, and the temp is deleted on mismatch or download error — the canonical path never holds unverified or partial content. Using a unique temp per call rather than a shared fixed path means overlapping `loadVerifiedFromUrl` calls can never swap each other's file between verify and rename (so a call always promotes exactly the bytes it verified). - **iOS bundle-size cap (64 MB)**, matching Android's, rejects oversized responses before they are hashed, written, or loaded. ## Accepted residual risks diff --git a/android/src/main/java/com/reactnativebundleloader/BundleLoaderModule.java b/android/src/main/java/com/reactnativebundleloader/BundleLoaderModule.java index b840990..483efe3 100644 --- a/android/src/main/java/com/reactnativebundleloader/BundleLoaderModule.java +++ b/android/src/main/java/com/reactnativebundleloader/BundleLoaderModule.java @@ -25,9 +25,6 @@ public class BundleLoaderModule extends ReactContextBaseJavaModule { // Host app references these as string literals (library is debugImplementation only). static final String BUNDLE_FILENAME = "verified-bundle.jsbundle"; - // Bytes are downloaded here first and only promoted to BUNDLE_FILENAME after the - // hash matches, so the canonical path never holds unverified/partial content. - static final String BUNDLE_TMP_FILENAME = "verified-bundle.jsbundle.tmp"; static final String PREFS_NAME = "BundleLoader"; static final String PREFS_PENDING_KEY = "pending_remote_bundle"; static final String PREFS_ACTIVE_KEY = "active_remote_bundle"; @@ -68,12 +65,13 @@ public void loadVerifiedFromUrl(final String url, final String expectedSha256, f public void run() { File cacheDir = getReactApplicationContext().getCacheDir(); File targetFile = new File(cacheDir, BUNDLE_FILENAME); - File tmpFile = new File(cacheDir, BUNDLE_TMP_FILENAME); - // Never write to the canonical path before verifying: download to a temp - // file, then promote it atomically only after the hash matches. Clear any - // stale temp left by a previously interrupted download. - tmpFile.delete(); + // Per-call unique temp: concurrent loadVerifiedFromUrl calls must never share a + // temp path, or a second call could swap the file out between this call's verify + // and its rename, promoting bytes this call never verified. Download here, then + // promote atomically onto the canonical path only after the hash matches. + File tmpFile = null; try { + tmpFile = File.createTempFile("verified-bundle-", ".jsbundle.tmp", cacheDir); byte[] actualDigest = downloadAndHashToCache( url, tmpFile, @@ -90,9 +88,13 @@ public void run() { setPendingFlag(); restartApp(); } catch (Exception e) { - // Never leave a partial/unverified temp bundle on disk. - tmpFile.delete(); promise.reject("E_LOAD_FAILED", e.getMessage(), e); + } finally { + // No-op after a successful rename; on mismatch or error it guarantees no + // partial/unverified temp is left behind. + if (tmpFile != null) { + tmpFile.delete(); + } } } }, "BundleLoader-loadVerifiedFromUrl").start(); diff --git a/android/src/test/java/com/reactnativebundleloader/VerifyAndInstallTest.java b/android/src/test/java/com/reactnativebundleloader/VerifyAndInstallTest.java index 82061f4..0273f0e 100644 --- a/android/src/test/java/com/reactnativebundleloader/VerifyAndInstallTest.java +++ b/android/src/test/java/com/reactnativebundleloader/VerifyAndInstallTest.java @@ -31,7 +31,7 @@ private File freshDir() throws IOException { @Test public void promotesTempToTargetOnHashMatch() throws Exception { File dir = freshDir(); - File tmp = new File(dir, BundleLoaderModule.BUNDLE_TMP_FILENAME); + File tmp = File.createTempFile("verified-bundle-", ".jsbundle.tmp", dir); File target = new File(dir, BundleLoaderModule.BUNDLE_FILENAME); byte[] body = "bundle-bytes".getBytes(StandardCharsets.UTF_8); Files.write(tmp.toPath(), body); @@ -48,7 +48,7 @@ public void promotesTempToTargetOnHashMatch() throws Exception { @Test public void deletesTempAndDoesNotCreateTargetOnMismatch() throws Exception { File dir = freshDir(); - File tmp = new File(dir, BundleLoaderModule.BUNDLE_TMP_FILENAME); + File tmp = File.createTempFile("verified-bundle-", ".jsbundle.tmp", dir); File target = new File(dir, BundleLoaderModule.BUNDLE_FILENAME); Files.write(tmp.toPath(), "attacker-bytes".getBytes(StandardCharsets.UTF_8)); @@ -63,7 +63,7 @@ public void deletesTempAndDoesNotCreateTargetOnMismatch() throws Exception { @Test public void doesNotClobberExistingVerifiedBundleOnMismatch() throws Exception { File dir = freshDir(); - File tmp = new File(dir, BundleLoaderModule.BUNDLE_TMP_FILENAME); + File tmp = File.createTempFile("verified-bundle-", ".jsbundle.tmp", dir); File target = new File(dir, BundleLoaderModule.BUNDLE_FILENAME); byte[] good = "previously-verified".getBytes(StandardCharsets.UTF_8); Files.write(target.toPath(), good); @@ -81,7 +81,7 @@ public void doesNotClobberExistingVerifiedBundleOnMismatch() throws Exception { @Test public void replacesExistingBundleOnHashMatch() throws Exception { File dir = freshDir(); - File tmp = new File(dir, BundleLoaderModule.BUNDLE_TMP_FILENAME); + File tmp = File.createTempFile("verified-bundle-", ".jsbundle.tmp", dir); File target = new File(dir, BundleLoaderModule.BUNDLE_FILENAME); Files.write(target.toPath(), "old".getBytes(StandardCharsets.UTF_8)); byte[] body = "new-verified".getBytes(StandardCharsets.UTF_8); @@ -94,4 +94,30 @@ public void replacesExistingBundleOnHashMatch() throws Exception { assertFalse(tmp.exists()); assertArrayEquals(body, Files.readAllBytes(target.toPath())); } + + /** + * Regression guard for the concurrent-load race: each loadVerifiedFromUrl call now + * downloads to its own File.createTempFile temp, so two overlapping installs can never + * share a path and promote each other's (unverified/partial) bytes. Distinct temps mean + * a call's verify→rename always promotes exactly the bytes it verified. + */ + @Test + public void concurrentCallsUseDistinctTempsSoTargetOnlyHoldsVerifiedBytes() throws Exception { + File dir = freshDir(); + File target = new File(dir, BundleLoaderModule.BUNDLE_FILENAME); + + File tmpA = File.createTempFile("verified-bundle-", ".jsbundle.tmp", dir); + File tmpB = File.createTempFile("verified-bundle-", ".jsbundle.tmp", dir); + assertFalse("each call must get a distinct temp path", tmpA.getPath().equals(tmpB.getPath())); + + byte[] a = "bundle-A".getBytes(StandardCharsets.UTF_8); + Files.write(tmpA.toPath(), a); + Files.write(tmpB.toPath(), "bundle-B-partial".getBytes(StandardCharsets.UTF_8)); + + // A promotes its own verified bytes; B mutating/deleting its own temp (as its call + // would) cannot affect A's target — the shared-path swap is impossible. + assertTrue(BundleLoaderModule.verifyAndInstall(tmpA, target, sha256("bundle-A"), sha256("bundle-A"))); + assertTrue("B's temp is fully independent of A's promotion", tmpB.delete()); + assertArrayEquals("target holds exactly A's verified bytes", a, Files.readAllBytes(target.toPath())); + } }