Low firefox Logic Error 🔧 Commit mapped

Overview

Low
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactlow
DescriptionOther issue in the Profile Backup component
ComponentCore
Bug ClassLogic Error
Tracker2044882
Fix commitd42b5220fa3d (firefox) +131/-13
CISA KEVNot listed
Credited5up3rh3i
Disclosed2026-09-01

Changed Functions

FunctionChangeNotes
if
browser/components/backup/BackupService.sys.mjs
modified
add_task
browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
modified

Files Changed

  • browser/components/backup/BackupService.sys.mjs
  • browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
diff --git a/browser/components/backup/BackupService.sys.mjs b/browser/components/backup/BackupService.sys.mjs
index 79193eecf63..3bcb6761d9e 100644
--- a/browser/components/backup/BackupService.sys.mjs
+++ b/browser/components/backup/BackupService.sys.mjs
@@ -2956,7 +2956,8 @@ export class BackupService extends EventTarget {
    *   The path to write the extracted file to.
    * @param {string} [recoveryCode=null]
    *   The recovery code to decrypt an encrypted backup with.
-   * @returns {Promise<undefined, Error>}
+   * @returns {Promise<{isEncrypted: boolean}, Error>}
+   *   Resolves with whether the archive was encrypted.
    */
   async extractCompressedSnapshotFromArchive(
     archivePath,
@@ -3015,6 +3016,8 @@ export class BackupService extends EventTarget {
         decryptor.OSKeyStoreSecret
       );
     }
+
+    return { isEncrypted };
   }
 
   /**
@@ -3250,11 +3253,12 @@ export class BackupService extends EventTarget {
         BackupService.RECOVERY_ZIP_FILE_NAME
       );
       currentStep = RESTORE_STEPS.RESTORE_EXTRACT_SNAPSHOT;
-      await this.extractCompressedSnapshotFromArchive(
-        archivePath,
-        RECOVERY_FILE_DEST_PATH,
-        recoveryCode
-      );
+      let { isEncrypted } =
+        (await this.extractCompressedSnapshotFromArchive(
+          archivePath,
+          RECOVERY_FILE_DEST_PATH,
+          recoveryCode
+        )) ?? {};
 
       const RECOVERY_FOLDER_DEST_PATH = PathUtils.join(
         profilePath,
@@ -3330,14 +3334,16 @@ export class BackupService extends EventTarget {
               null,
               profileRootPath,
               manifest,
-              replaceCurrentProfile
+              replaceCurrentProfile,
+              isEncrypted
             );
         } else {
           newProfile = await this.recoverFromSnapshotFolder(
             RECOVERY_FOLDER_DEST_PATH,
             shouldLaunchOrQuit,
             profileRootPath,
-            manifest
+            manifest,
+            isEncrypted
           );
         }
 
@@ -3585,10 +3591,12 @@ export class BackupService extends EventTarget {
    * @param {string} recoveryPath The path to the decompressed backup archive
    *   on the file system.
    * @param {string} profilePath The path of the newly recovered profile
+   * @param {boolean} wasEncrypted Whether the source archive was encrypted.
+   *   Resources that require encryption are skipped when this is false.
    * @returns {object}
    *   An object containing post recovery data for each resource.
    */
-  async #recoverResources(manifest, recoveryPath, profilePath) {
+  async #recoverResources(manifest, recoveryPath, profilePath, wasEncrypted) {
     let postRecovery = {};
 
     // Iterate over each resource in the manifest and call recover() on each
@@ -3601,6 +3609,16 @@ export class BackupService extends EventTarget {
         continue;
       }
 
+      // A resource that requires encryption should only have been written into
+      // an encrypted archive. If the archive isn't encrypted, refuse to recover it.
+      if (resourceClass.requiresEncryption && !wasEncrypted) {
+        lazy.logConsole.warn(
+          `Skipping resource ${resourceKey}: requires encryption but the ` +
+            `archive is not encrypted.`
+        );
+        continue;
+      }
+
       try {
         lazy.logConsole.debug(
           `Restoring resource with key ${resourceKey}. ` +
@@ -3679,6 +3697,9 @@ export class BackupService extends EventTarget {
    * @param {object} [manifest=null]
    *   If we've already read and validated the manifest, we can avoid redoing that work
    *   by passing this in as a parameter.
+   * @param {boolean} [wasEncrypted=false]
+   *   Whether the source archive was encrypted. Resources that require
+   *   encryption are skipped during recovery when this is false.
    * @returns {Promise<nsIToolkitProfile>}
    *   The nsIToolkitProfile that was created for the recovered profile.
    * @throws {Exception}
@@ -3688,7 +3709,8 @@ export class BackupService extends EventTarget {
     recoveryPath,
     shouldLaunch = false,
     profileRootPath = null,
-    manifest = null
+    manifest = null,
+    wasEncrypted = false
   ) {
     lazy.logConsole.debug("Recovering from backup at ", recoveryPath);
 
@@ -3724,7 +3746,8 @@ export class BackupService extends EventTarget {
       let postRecovery = await this.#recoverResources(
         manifest,
         recoveryPath,
-        profile.rootDir.path
+        profile.rootDir.path,
+        wasEncrypted
       );
 
       restoreStep = RESTORE_STEPS.RESTORE_WRITE_POST_RECOVERY;
@@ -3841,6 +3864,9 @@ export class BackupService extends EventTarget {
    *   by passing this in as a parameter.
    * @param {boolean} [replaceCurrentProfile=false]
    *   Indicates if we are replacing the current running profile or adding to the group
+   * @param {boolean} [wasEncrypted=false]
+   *   Whether the source archive was encrypted. Resources that require
+   *   encryption are skipped during recovery when this is false.
    * @returns {Promise<SelectableProfile>}
    *   The SelectableProfile that was created for the recovered profile.
    * @throws {Exception}
@@ -3852,7 +3878,8 @@ export class BackupService extends EventTarget {
     copiedProfile = null,
     profileRootPath = null,
     manifest = null,
-    replaceCurrentProfile = false
+    replaceCurrentProfile = false,
+    wasEncrypted = false
   ) {
     lazy.logConsole.debug(
       "Recovering SelectableProfile from backup at ",
@@ -3895,7 +3922,8 @@ export class BackupService extends EventTarget {
       let postRecovery = await this.#recoverResources(
         manifest,
         recoveryPath,
-        profile.path
+        profile.path,
+        wasEncrypted
       );
 
       if (copiedProfile) {
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js b/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
index 2b9d2e475ae..cd92c8d7096 100644
--- a/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
+++ b/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
@@ -152,3 +152,93 @@ add_task(async function test_profile_naming() {
 
   await IOUtils.remove(testRecoveryPath, { recursive: true });
 });
+
+/**
+ * Tests that resources whose requiresEncryption is true are skipped when
+ * recovering from an unencrypted archive, and recovered when the archive was
+ * encrypted.
+ */
+add_task(async function test_skip_encrypted_resource_when_unencrypted() {
+  let sandbox = sinon.createSandbox();
+
+  sandbox.stub(FakeBackupResource1, "requiresEncryption").get(() => true);
+  sandbox.stub(FakeBackupResource2, "requiresEncryption").get(() => false);
+  sandbox.stub(FakeBackupResource3, "requiresEncryption").get(() => false);
+
+  let recover1 = sandbox
+    .stub(FakeBackupResource1.prototype, "recover")
+    .resolves();
+  let recover2 = sandbox
+    .stub(FakeBackupResource2.prototype, "recover")
+    .resolves();
+  let recover3 = sandbox
+    .stub(FakeBackupResource3.prototype, "recover")
+    .resolves();
+
+  let bs = new BackupService({
+    FakeBackupResource1,
+    FakeBackupResource2,
+    FakeBackupResource3,
+  });
+
+  let testRecoveryPath = await IOUtils.createUniqueDirectory(
+    PathUtils.tempDir,
+    "testSkipEncrypted"
+  );
+  let profileRootPath = await IOUtils.createUniqueDirectory(
+    PathUtils.tempDir,
+    "testSkipEncryptedProfiles"
+  );
+
+  let manifest = {
+    version: ArchiveUtils.SCHEMA_VERSION,
+    meta: Object.assign({}, FAKE_METADATA),
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js b/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
index 2b9d2e475ae..cd92c8d7096 100644
--- a/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
+++ b/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js
@@ -152,3 +152,93 @@ add_task(async function test_profile_naming() {
 
   await IOUtils.remove(testRecoveryPath, { recursive: true });
 });
+
+/**
+ * Tests that resources whose requiresEncryption is true are skipped when
+ * recovering from an unencrypted archive, and recovered when the archive was
+ * encrypted.
+ */
+add_task(async function test_skip_encrypted_resource_when_unencrypted() {
+  let sandbox = sinon.createSandbox();
+
+  sandbox.stub(FakeBackupResource1, "requiresEncryption").get(() => true);
+  sandbox.stub(FakeBackupResource2, "requiresEncryption").get(() => false);
+  sandbox.stub(FakeBackupResource3, "requiresEncryption").get(() => false);
+
+  let recover1 = sandbox
+    .stub(FakeBackupResource1.prototype, "recover")
+    .resolves();
+  let recover2 = sandbox
+    .stub(FakeBackupResource2.prototype, "recover")
+    .resolves();
+  let recover3 = sandbox
+    .stub(FakeBackupResource3.prototype, "recover")
+    .resolves();
+
+  let bs = new BackupService({
+    FakeBackupResource1,
+    FakeBackupResource2,
+    FakeBackupResource3,
+  });
+
+  let testRecoveryPath = await IOUtils.createUniqueDirectory(
+    PathUtils.tempDir,
+    "testSkipEncrypted"
+  );
+  let profileRootPath = await IOUtils.createUniqueDirectory(
+    PathUtils.tempDir,
+    "testSkipEncryptedProfiles"
+  );
+
+  let manifest = {
+    version: ArchiveUtils.SCHEMA_VERSION,
+    meta: Object.assign({}, FAKE_METADATA),
+    resources: { fake1: {}, fake2: {}, fake3: {} },
+  };
+
+  // Unencrypted archive: the encryption-requiring resource must be skipped.
+  await bs.recoverFromSnapshotFolder(
+    testRecoveryPath,
+    false,
+    profileRootPath,
+    manifest,
+    false
+  );
+
+  Assert.ok(
+    recover1.notCalled,
+    "Resource requiring encryption is skipped for an unencrypted archive."
+  );
+  Assert.ok(
+    recover2.calledOnce,
+    "Resource not requiring encryption is recovered."
+  );
+  Assert.ok(
+    recover3.calledOnce,
+    "Resource not requiring encryption is recovered."
+  );
+
+  recover1.resetHistory();
+  recover2.resetHistory();
+  recover3.resetHistory();
+
+  // Encrypted archive: every resource is recovered.
+  await bs.recoverFromSnapshotFolder(
+    testRecoveryPath,
+    false,
+    profileRootPath,
+    manifest,
+    true
+  );
+
+  Assert.ok(
+    recover1.calledOnce,
+    "Resource requiring encryption is recovered for an encrypted archive."
+  );
+  Assert.ok(recover2.calledOnce, "Non-encryption resource recovered again.");
+  Assert.ok(recover3.calledOnce, "Non-encryption resource recovered again.");
+
+  sandbox.restore();
+  await IOUtils.remove(testRecoveryPath, { recursive: true });
+  await IOUtils.remove(profileRootPath, { recursive: true });
+});
Loading diff…