Pier Angelo Vendrame pushed to branch tor-browser-128.4.0esr-14.5-1 at The Tor Project / Applications / Tor Browser

Commits:

1 changed file:

Changes:

  • toolkit/components/lox/Lox.sys.mjs
    ... ... @@ -144,9 +144,9 @@ class LoxImpl {
    144 144
       /**
    
    145 145
        * The latest credentials for a given lox id.
    
    146 146
        *
    
    147
    -   * @type {Object<string, string>}
    
    147
    +   * @type {Map<string, string>}
    
    148 148
        */
    
    149
    -  #credentials = {};
    
    149
    +  #credentials = new Map();
    
    150 150
       /**
    
    151 151
        * The list of accumulated blockage or upgrade events.
    
    152 152
        *
    
    ... ... @@ -257,25 +257,73 @@ class LoxImpl {
    257 257
       }
    
    258 258
     
    
    259 259
       /**
    
    260
    -   * Change some existing credentials for an ID to a new value.
    
    260
    +   * Stores a promise for the last task that was performed to change
    
    261
    +   * credentials for a given lox ID. This promise completes when the task
    
    262
    +   * completes and it is safe to perform a new action on the credentials.
    
    263
    +   *
    
    264
    +   * This essentially acts as a lock on the credential, so that only one task
    
    265
    +   * acts on the credentials at any given time. See tor-browser#42492.
    
    266
    +   *
    
    267
    +   * @type {Map<string, Promise>}
    
    268
    +   */
    
    269
    +  #credentialsTasks = new Map();
    
    270
    +
    
    271
    +  /**
    
    272
    +   * Attempt to change some existing credentials for an ID to a new value.
    
    273
    +   *
    
    274
    +   * Each call for the same lox ID must await the previous call. As such, this
    
    275
    +   * should *never* be called recursively.
    
    261 276
        *
    
    262 277
        * @param {string} loxId - The ID to change the credentials for.
    
    263
    -   * @param {string} newCredentials - The new credentials to set.
    
    278
    +   * @param {Function} task - The task that performs the change in credentials.
    
    279
    +   *   The method is given the current credentials. It should either return the
    
    280
    +   *   new credentials as a string, or null if the credentials should not
    
    281
    +   *   change, or throw an error which will fall through to the caller.
    
    282
    +   *
    
    283
    +   * @returns {?string} - The credentials returned by the task, if any.
    
    264 284
        */
    
    265
    -  #changeCredentials(loxId, newCredentials) {
    
    266
    -    // FIXME: Several async methods want to update the credentials, but they
    
    267
    -    // might race and conflict with each. tor-browser#42492
    
    268
    -    if (!newCredentials) {
    
    269
    -      // Avoid overwriting and losing our current credentials.
    
    270
    -      throw new LoxError(`Empty credentials being set for ${loxId}`);
    
    285
    +  async #changeCredentials(loxId, task) {
    
    286
    +    // Read and replace #credentialsTasks before we do any async operations.
    
    287
    +    // I.e. this is effectively atomic read and replace.
    
    288
    +    const prevTask = this.#credentialsTasks.get(loxId);
    
    289
    +    let taskComplete;
    
    290
    +    this.#credentialsTasks.set(
    
    291
    +      loxId,
    
    292
    +      new Promise(res => {
    
    293
    +        taskComplete = res;
    
    294
    +      })
    
    295
    +    );
    
    296
    +
    
    297
    +    // Wait for any previous task to complete first, to avoid making conflicting
    
    298
    +    // changes to the credentials. See tor-browser#42492.
    
    299
    +    // prevTask is either undefined or a promise that should not throw.
    
    300
    +    await prevTask;
    
    301
    +
    
    302
    +    // Future calls now await us.
    
    303
    +
    
    304
    +    const cred = this.#getCredentials(loxId);
    
    305
    +    let newCred = null;
    
    306
    +    try {
    
    307
    +      // This task may throw, in which case we do not set new credentials.
    
    308
    +      newCred = await task(cred);
    
    309
    +      if (newCred) {
    
    310
    +        this.#credentials.set(loxId, newCred);
    
    311
    +        // Store the new credentials.
    
    312
    +        this.#store();
    
    313
    +        lazy.logger.debug("Changed credentials");
    
    314
    +      }
    
    315
    +    } finally {
    
    316
    +      // Stop awaiting us.
    
    317
    +      taskComplete();
    
    271 318
         }
    
    272
    -    if (!this.#credentials[loxId]) {
    
    273
    -      // Unexpected, but we still want to save the value to storage.
    
    274
    -      lazy.logger.warn(`Lox ID ${loxId} is missing existing credentials`);
    
    319
    +
    
    320
    +    if (!newCred) {
    
    321
    +      return null;
    
    275 322
         }
    
    276 323
     
    
    277
    -    this.#credentials[loxId] = newCredentials;
    
    278
    -    this.#store();
    
    324
    +    // Let listeners know we have new credentials. We do this *after* calling
    
    325
    +    // taskComplete to avoid a recursive call to await this.#changeCredentials,
    
    326
    +    // which would cause us to hang.
    
    279 327
     
    
    280 328
         // NOTE: In principle we could determine within this module whether the
    
    281 329
         // bridges, remaining invites, or next unlock changes in value when
    
    ... ... @@ -289,6 +337,8 @@ class LoxImpl {
    289 337
         // Let UI know about changes.
    
    290 338
         Services.obs.notifyObservers(null, LoxTopics.UpdateRemainingInvites);
    
    291 339
         Services.obs.notifyObservers(null, LoxTopics.UpdateNextUnlock);
    
    340
    +
    
    341
    +    return newCred;
    
    292 342
       }
    
    293 343
     
    
    294 344
       /**
    
    ... ... @@ -299,7 +349,7 @@ class LoxImpl {
    299 349
        * @returns {string} - The credentials.
    
    300 350
        */
    
    301 351
       #getCredentials(loxId) {
    
    302
    -    const cred = loxId ? this.#credentials[loxId] : undefined;
    
    352
    +    const cred = loxId ? this.#credentials.get(loxId) : undefined;
    
    303 353
         if (!cred) {
    
    304 354
           throw new LoxError(`No credentials for ${loxId}`);
    
    305 355
         }
    
    ... ... @@ -376,7 +426,7 @@ class LoxImpl {
    376 426
         Services.prefs.setStringPref(LoxSettingsPrefs.constants, this.#constants);
    
    377 427
         Services.prefs.setStringPref(
    
    378 428
           LoxSettingsPrefs.credentials,
    
    379
    -      JSON.stringify(this.#credentials)
    
    429
    +      JSON.stringify(Object.fromEntries(this.#credentials))
    
    380 430
         );
    
    381 431
         Services.prefs.setStringPref(
    
    382 432
           LoxSettingsPrefs.invites,
    
    ... ... @@ -390,7 +440,7 @@ class LoxImpl {
    390 440
     
    
    391 441
       #load() {
    
    392 442
         const cred = Services.prefs.getStringPref(LoxSettingsPrefs.credentials, "");
    
    393
    -    this.#credentials = cred ? JSON.parse(cred) : {};
    
    443
    +    this.#credentials = new Map(cred ? Object.entries(JSON.parse(cred)) : []);
    
    394 444
         const invites = Services.prefs.getStringPref(LoxSettingsPrefs.invites, "");
    
    395 445
         this.#invites = invites ? JSON.parse(invites) : [];
    
    396 446
         const events = Services.prefs.getStringPref(LoxSettingsPrefs.events, "");
    
    ... ... @@ -421,17 +471,15 @@ class LoxImpl {
    421 471
         if (prevKeys !== null) {
    
    422 472
           // check if the lox pubkeys have changed and update the lox
    
    423 473
           // credentials if so.
    
    424
    -      //
    
    425
    -      // The UpdateCredOption rust struct serializes to "req" rather than
    
    426
    -      // "request".
    
    427
    -      const { updated, req: request } = JSON.parse(
    
    428
    -        lazy.check_lox_pubkeys_update(
    
    429
    -          pubKeys,
    
    430
    -          prevKeys,
    
    431
    -          this.#getCredentials(this.#activeLoxId)
    
    432
    -        )
    
    433
    -      );
    
    434
    -      if (updated) {
    
    474
    +      await this.#changeCredentials(this.#activeLoxId, async cred => {
    
    475
    +        // The UpdateCredOption rust struct serializes to "req" rather than
    
    476
    +        // "request".
    
    477
    +        const { updated, req: request } = JSON.parse(
    
    478
    +          lazy.check_lox_pubkeys_update(pubKeys, prevKeys, cred)
    
    479
    +        );
    
    480
    +        if (!updated) {
    
    481
    +          return null;
    
    482
    +        }
    
    435 483
             // Try update credentials.
    
    436 484
             // NOTE: This should be re-callable if any step fails.
    
    437 485
             // TODO: Verify this.
    
    ... ... @@ -444,9 +492,8 @@ class LoxImpl {
    444 492
             // is refactored to send repeat responses:
    
    445 493
             // https://gitlab.torproject.org/tpo/anti-censorship/lox/-/issues/74)
    
    446 494
             let response = await this.#makeRequest("updatecred", request);
    
    447
    -        let cred = lazy.handle_update_cred(request, response, pubKeys);
    
    448
    -        this.#changeCredentials(this.#activeLoxId, cred);
    
    449
    -      }
    
    495
    +        return lazy.handle_update_cred(request, response, pubKeys);
    
    496
    +      });
    
    450 497
         }
    
    451 498
         // If we arrive here we haven't had other errors before, we can actually
    
    452 499
         // store the new public key.
    
    ... ... @@ -648,7 +695,7 @@ class LoxImpl {
    648 695
         this.#pubKeyPromise = null;
    
    649 696
         this.#encTablePromise = null;
    
    650 697
         this.#constantsPromise = null;
    
    651
    -    this.#credentials = {};
    
    698
    +    this.#credentials = new Map();
    
    652 699
         this.#events = [];
    
    653 700
         if (this.#backgroundInterval) {
    
    654 701
           clearInterval(this.#backgroundInterval);
    
    ... ... @@ -712,9 +759,9 @@ class LoxImpl {
    712 759
         let loxId;
    
    713 760
         do {
    
    714 761
           loxId = this.#genLoxId();
    
    715
    -    } while (Object.hasOwn(this.#credentials, loxId));
    
    762
    +    } while (this.#credentials.has(loxId));
    
    716 763
         // Set new credentials.
    
    717
    -    this.#credentials[loxId] = cred;
    
    764
    +    this.#credentials.set(loxId, cred);
    
    718 765
         this.#store();
    
    719 766
         return loxId;
    
    720 767
       }
    
    ... ... @@ -760,21 +807,17 @@ class LoxImpl {
    760 807
         if (level < 1) {
    
    761 808
           throw new LoxError(`Cannot generate invites at level ${level}`);
    
    762 809
         }
    
    763
    -    let request = lazy.issue_invite(
    
    764
    -      this.#getCredentials(loxId),
    
    765
    -      this.#encTable,
    
    766
    -      this.#pubKeys
    
    767
    -    );
    
    768
    -    let response = await this.#makeRequest("issueinvite", request);
    
    769
    -    // TODO: Do we ever expect handle_issue_invite to fail (beyond
    
    770
    -    // implementation bugs)?
    
    771
    -    // TODO: What happens if #pubkeys for `issue_invite` differs from the value
    
    772
    -    // when calling `handle_issue_invite`? Should we cache the value at the
    
    773
    -    // start of this method?
    
    774
    -    let cred = lazy.handle_issue_invite(request, response, this.#pubKeys);
    
    775 810
     
    
    776
    -    // Store the new credentials as a priority.
    
    777
    -    this.#changeCredentials(loxId, cred);
    
    811
    +    const cred = await this.#changeCredentials(loxId, async cred => {
    
    812
    +      let request = lazy.issue_invite(cred, this.#encTable, this.#pubKeys);
    
    813
    +      let response = await this.#makeRequest("issueinvite", request);
    
    814
    +      // TODO: Do we ever expect handle_issue_invite to fail (beyond
    
    815
    +      // implementation bugs)?
    
    816
    +      // TODO: What happens if #pubkeys for `issue_invite` differs from the value
    
    817
    +      // when calling `handle_issue_invite`? Should we cache the value at the
    
    818
    +      // start of this method?
    
    819
    +      return lazy.handle_issue_invite(request, response, this.#pubKeys);
    
    820
    +    });
    
    778 821
     
    
    779 822
         const invite = lazy.prepare_invite(cred);
    
    780 823
         this.#invites.push(invite);
    
    ... ... @@ -804,35 +847,26 @@ class LoxImpl {
    804 847
       }
    
    805 848
     
    
    806 849
       async #blockageMigration(loxId) {
    
    807
    -    let request;
    
    808
    -    try {
    
    809
    -      request = lazy.check_blockage(this.#getCredentials(loxId), this.#pubKeys);
    
    810
    -    } catch {
    
    811
    -      lazy.logger.log("Not ready for blockage migration");
    
    812
    -      return false;
    
    813
    -    }
    
    814
    -    let response = await this.#makeRequest("checkblockage", request);
    
    815
    -    // NOTE: If a later method fails, we should be ok to re-call "checkblockage"
    
    816
    -    // from the Lox authority. So there shouldn't be any adverse side effects to
    
    817
    -    // loosing migrationCred.
    
    818
    -    // TODO: Confirm this is safe to lose.
    
    819
    -    const migrationCred = lazy.handle_check_blockage(
    
    820
    -      this.#getCredentials(loxId),
    
    821
    -      response
    
    822
    -    );
    
    823
    -    request = lazy.blockage_migration(
    
    824
    -      this.#getCredentials(loxId),
    
    825
    -      migrationCred,
    
    826
    -      this.#pubKeys
    
    827
    -    );
    
    828
    -    response = await this.#makeRequest("blockagemigration", request);
    
    829
    -    const cred = lazy.handle_blockage_migration(
    
    830
    -      this.#getCredentials(loxId),
    
    831
    -      response,
    
    832
    -      this.#pubKeys
    
    850
    +    return Boolean(
    
    851
    +      await this.#changeCredentials(loxId, async cred => {
    
    852
    +        let request;
    
    853
    +        try {
    
    854
    +          request = lazy.check_blockage(cred, this.#pubKeys);
    
    855
    +        } catch {
    
    856
    +          lazy.logger.log("Not ready for blockage migration");
    
    857
    +          return null;
    
    858
    +        }
    
    859
    +        let response = await this.#makeRequest("checkblockage", request);
    
    860
    +        // NOTE: If a later method fails, we should be ok to re-call "checkblockage"
    
    861
    +        // from the Lox authority. So there shouldn't be any adverse side effects to
    
    862
    +        // loosing migrationCred.
    
    863
    +        // TODO: Confirm this is safe to lose.
    
    864
    +        const migrationCred = lazy.handle_check_blockage(cred, response);
    
    865
    +        request = lazy.blockage_migration(cred, migrationCred, this.#pubKeys);
    
    866
    +        response = await this.#makeRequest("blockagemigration", request);
    
    867
    +        return lazy.handle_blockage_migration(cred, response, this.#pubKeys);
    
    868
    +      })
    
    833 869
         );
    
    834
    -    this.#changeCredentials(loxId, cred);
    
    835
    -    return true;
    
    836 870
       }
    
    837 871
     
    
    838 872
       /**
    
    ... ... @@ -850,25 +884,26 @@ class LoxImpl {
    850 884
           // attempt trust promotion instead
    
    851 885
           return this.#trustMigration(loxId);
    
    852 886
         }
    
    853
    -    let request = lazy.level_up(
    
    854
    -      this.#getCredentials(loxId),
    
    855
    -      this.#encTable,
    
    856
    -      this.#pubKeys
    
    887
    +    return Boolean(
    
    888
    +      await this.#changeCredentials(loxId, async cred => {
    
    889
    +        let request = lazy.level_up(cred, this.#encTable, this.#pubKeys);
    
    890
    +        let response;
    
    891
    +        try {
    
    892
    +          response = await this.#makeRequest("levelup", request);
    
    893
    +        } catch (error) {
    
    894
    +          if (
    
    895
    +            error instanceof LoxError &&
    
    896
    +            error.code === LoxError.ErrorResponse
    
    897
    +          ) {
    
    898
    +            // Not an error.
    
    899
    +            lazy.logger.debug("Not ready for level up", error);
    
    900
    +            return null;
    
    901
    +          }
    
    902
    +          throw error;
    
    903
    +        }
    
    904
    +        return lazy.handle_level_up(request, response, this.#pubKeys);
    
    905
    +      })
    
    857 906
         );
    
    858
    -    let response;
    
    859
    -    try {
    
    860
    -      response = await this.#makeRequest("levelup", request);
    
    861
    -    } catch (error) {
    
    862
    -      if (error instanceof LoxError && error.code === LoxError.ErrorResponse) {
    
    863
    -        // Not an error.
    
    864
    -        lazy.logger.debug("Not ready for level up", error);
    
    865
    -        return false;
    
    866
    -      }
    
    867
    -      throw error;
    
    868
    -    }
    
    869
    -    const cred = lazy.handle_level_up(request, response, this.#pubKeys);
    
    870
    -    this.#changeCredentials(loxId, cred);
    
    871
    -    return true;
    
    872 907
       }
    
    873 908
     
    
    874 909
       /**
    
    ... ... @@ -884,42 +919,37 @@ class LoxImpl {
    884 919
           this.#getPubKeys();
    
    885 920
           return false;
    
    886 921
         }
    
    887
    -    let request;
    
    888
    -    try {
    
    889
    -      request = lazy.trust_promotion(
    
    890
    -        this.#getCredentials(loxId),
    
    891
    -        this.#pubKeys
    
    892
    -      );
    
    893
    -    } catch (err) {
    
    894
    -      // This function is called routinely during the background tasks without
    
    895
    -      // previous checks on whether an upgrade is possible, so it is expected to
    
    896
    -      // fail with a certain frequency. Therefore, do not relay the error to the
    
    897
    -      // caller and just log the message for debugging.
    
    898
    -      lazy.logger.debug("Not ready to upgrade", err);
    
    899
    -      return false;
    
    900
    -    }
    
    922
    +    return Boolean(
    
    923
    +      await this.#changeCredentials(loxId, async cred => {
    
    924
    +        let request;
    
    925
    +        try {
    
    926
    +          request = lazy.trust_promotion(cred, this.#pubKeys);
    
    927
    +        } catch (err) {
    
    928
    +          // This function is called routinely during the background tasks without
    
    929
    +          // previous checks on whether an upgrade is possible, so it is expected to
    
    930
    +          // fail with a certain frequency. Therefore, do not relay the error to the
    
    931
    +          // caller and just log the message for debugging.
    
    932
    +          lazy.logger.debug("Not ready to upgrade", err);
    
    933
    +          return null;
    
    934
    +        }
    
    901 935
     
    
    902
    -    let response = await this.#makeRequest("trustpromo", request);
    
    903
    -    // FIXME: Store response to "trustpromo" in case handle_trust_promotion
    
    904
    -    // or "trustmig" fails. The Lox authority will not accept a re-request
    
    905
    -    // to "trustpromo" with the same credentials.
    
    906
    -    let promoCred = lazy.handle_trust_promotion(request, response);
    
    907
    -    lazy.logger.debug("Formatted promotion cred: ", promoCred);
    
    908
    -
    
    909
    -    request = lazy.trust_migration(
    
    910
    -      this.#getCredentials(loxId),
    
    911
    -      promoCred,
    
    912
    -      this.#pubKeys
    
    936
    +        let response = await this.#makeRequest("trustpromo", request);
    
    937
    +        // FIXME: Store response to "trustpromo" in case handle_trust_promotion
    
    938
    +        // or "trustmig" fails. The Lox authority will not accept a re-request
    
    939
    +        // to "trustpromo" with the same credentials.
    
    940
    +        let promoCred = lazy.handle_trust_promotion(request, response);
    
    941
    +        lazy.logger.debug("Formatted promotion cred: ", promoCred);
    
    942
    +
    
    943
    +        request = lazy.trust_migration(cred, promoCred, this.#pubKeys);
    
    944
    +        response = await this.#makeRequest("trustmig", request);
    
    945
    +        lazy.logger.debug("Got new credential: ", response);
    
    946
    +
    
    947
    +        // FIXME: Store response to "trustmig" in case handle_trust_migration
    
    948
    +        // fails. The Lox authority will not accept a re-request to "trustmig" with
    
    949
    +        // the same credentials.
    
    950
    +        return lazy.handle_trust_migration(request, response);
    
    951
    +      })
    
    913 952
         );
    
    914
    -    response = await this.#makeRequest("trustmig", request);
    
    915
    -    lazy.logger.debug("Got new credential: ", response);
    
    916
    -
    
    917
    -    // FIXME: Store response to "trustmig" in case handle_trust_migration
    
    918
    -    // fails. The Lox authority will not accept a re-request to "trustmig" with
    
    919
    -    // the same credentials.
    
    920
    -    let cred = lazy.handle_trust_migration(request, response);
    
    921
    -    this.#changeCredentials(loxId, cred);
    
    922
    -    return true;
    
    923 953
       }
    
    924 954
     
    
    925 955
       /**