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

Commits: eb5aaf7e by Henry Wilkes at 2024-11-12T15:43:54+00:00 fixup! Lox integration

Bug 42492: Ensure operations that change lox credentials do not overlap.

Not linted to improve readability.

- - - - - d4097f9c by Henry Wilkes at 2024-11-12T15:45:39+00:00 fixup! Lox integration

Bug 42492: Lint Lox.sys.mjs

- - - - -

1 changed file:

- toolkit/components/lox/Lox.sys.mjs

Changes:

===================================== toolkit/components/lox/Lox.sys.mjs ===================================== @@ -144,9 +144,9 @@ class LoxImpl { /** * The latest credentials for a given lox id. * - * @type {Object<string, string>} + * @type {Map<string, string>} */ - #credentials = {}; + #credentials = new Map(); /** * The list of accumulated blockage or upgrade events. * @@ -257,25 +257,73 @@ class LoxImpl { }

/** - * Change some existing credentials for an ID to a new value. + * Stores a promise for the last task that was performed to change + * credentials for a given lox ID. This promise completes when the task + * completes and it is safe to perform a new action on the credentials. + * + * This essentially acts as a lock on the credential, so that only one task + * acts on the credentials at any given time. See tor-browser#42492. + * + * @type {Map<string, Promise>} + */ + #credentialsTasks = new Map(); + + /** + * Attempt to change some existing credentials for an ID to a new value. + * + * Each call for the same lox ID must await the previous call. As such, this + * should *never* be called recursively. * * @param {string} loxId - The ID to change the credentials for. - * @param {string} newCredentials - The new credentials to set. + * @param {Function} task - The task that performs the change in credentials. + * The method is given the current credentials. It should either return the + * new credentials as a string, or null if the credentials should not + * change, or throw an error which will fall through to the caller. + * + * @returns {?string} - The credentials returned by the task, if any. */ - #changeCredentials(loxId, newCredentials) { - // FIXME: Several async methods want to update the credentials, but they - // might race and conflict with each. tor-browser#42492 - if (!newCredentials) { - // Avoid overwriting and losing our current credentials. - throw new LoxError(`Empty credentials being set for ${loxId}`); + async #changeCredentials(loxId, task) { + // Read and replace #credentialsTasks before we do any async operations. + // I.e. this is effectively atomic read and replace. + const prevTask = this.#credentialsTasks.get(loxId); + let taskComplete; + this.#credentialsTasks.set( + loxId, + new Promise(res => { + taskComplete = res; + }) + ); + + // Wait for any previous task to complete first, to avoid making conflicting + // changes to the credentials. See tor-browser#42492. + // prevTask is either undefined or a promise that should not throw. + await prevTask; + + // Future calls now await us. + + const cred = this.#getCredentials(loxId); + let newCred = null; + try { + // This task may throw, in which case we do not set new credentials. + newCred = await task(cred); + if (newCred) { + this.#credentials.set(loxId, newCred); + // Store the new credentials. + this.#store(); + lazy.logger.debug("Changed credentials"); + } + } finally { + // Stop awaiting us. + taskComplete(); } - if (!this.#credentials[loxId]) { - // Unexpected, but we still want to save the value to storage. - lazy.logger.warn(`Lox ID ${loxId} is missing existing credentials`); + + if (!newCred) { + return null; }

- this.#credentials[loxId] = newCredentials; - this.#store(); + // Let listeners know we have new credentials. We do this *after* calling + // taskComplete to avoid a recursive call to await this.#changeCredentials, + // which would cause us to hang.

// NOTE: In principle we could determine within this module whether the // bridges, remaining invites, or next unlock changes in value when @@ -289,6 +337,8 @@ class LoxImpl { // Let UI know about changes. Services.obs.notifyObservers(null, LoxTopics.UpdateRemainingInvites); Services.obs.notifyObservers(null, LoxTopics.UpdateNextUnlock); + + return newCred; }

/** @@ -299,7 +349,7 @@ class LoxImpl { * @returns {string} - The credentials. */ #getCredentials(loxId) { - const cred = loxId ? this.#credentials[loxId] : undefined; + const cred = loxId ? this.#credentials.get(loxId) : undefined; if (!cred) { throw new LoxError(`No credentials for ${loxId}`); } @@ -376,7 +426,7 @@ class LoxImpl { Services.prefs.setStringPref(LoxSettingsPrefs.constants, this.#constants); Services.prefs.setStringPref( LoxSettingsPrefs.credentials, - JSON.stringify(this.#credentials) + JSON.stringify(Object.fromEntries(this.#credentials)) ); Services.prefs.setStringPref( LoxSettingsPrefs.invites, @@ -390,7 +440,7 @@ class LoxImpl {

#load() { const cred = Services.prefs.getStringPref(LoxSettingsPrefs.credentials, ""); - this.#credentials = cred ? JSON.parse(cred) : {}; + this.#credentials = new Map(cred ? Object.entries(JSON.parse(cred)) : []); const invites = Services.prefs.getStringPref(LoxSettingsPrefs.invites, ""); this.#invites = invites ? JSON.parse(invites) : []; const events = Services.prefs.getStringPref(LoxSettingsPrefs.events, ""); @@ -421,17 +471,15 @@ class LoxImpl { if (prevKeys !== null) { // check if the lox pubkeys have changed and update the lox // credentials if so. - // - // The UpdateCredOption rust struct serializes to "req" rather than - // "request". - const { updated, req: request } = JSON.parse( - lazy.check_lox_pubkeys_update( - pubKeys, - prevKeys, - this.#getCredentials(this.#activeLoxId) - ) - ); - if (updated) { + await this.#changeCredentials(this.#activeLoxId, async cred => { + // The UpdateCredOption rust struct serializes to "req" rather than + // "request". + const { updated, req: request } = JSON.parse( + lazy.check_lox_pubkeys_update(pubKeys, prevKeys, cred) + ); + if (!updated) { + return null; + } // Try update credentials. // NOTE: This should be re-callable if any step fails. // TODO: Verify this. @@ -444,9 +492,8 @@ class LoxImpl { // is refactored to send repeat responses: // https://gitlab.torproject.org/tpo/anti-censorship/lox/-/issues/74) let response = await this.#makeRequest("updatecred", request); - let cred = lazy.handle_update_cred(request, response, pubKeys); - this.#changeCredentials(this.#activeLoxId, cred); - } + return lazy.handle_update_cred(request, response, pubKeys); + }); } // If we arrive here we haven't had other errors before, we can actually // store the new public key. @@ -648,7 +695,7 @@ class LoxImpl { this.#pubKeyPromise = null; this.#encTablePromise = null; this.#constantsPromise = null; - this.#credentials = {}; + this.#credentials = new Map(); this.#events = []; if (this.#backgroundInterval) { clearInterval(this.#backgroundInterval); @@ -712,9 +759,9 @@ class LoxImpl { let loxId; do { loxId = this.#genLoxId(); - } while (Object.hasOwn(this.#credentials, loxId)); + } while (this.#credentials.has(loxId)); // Set new credentials. - this.#credentials[loxId] = cred; + this.#credentials.set(loxId, cred); this.#store(); return loxId; } @@ -760,21 +807,17 @@ class LoxImpl { if (level < 1) { throw new LoxError(`Cannot generate invites at level ${level}`); } - let request = lazy.issue_invite( - this.#getCredentials(loxId), - this.#encTable, - this.#pubKeys - ); - let response = await this.#makeRequest("issueinvite", request); - // TODO: Do we ever expect handle_issue_invite to fail (beyond - // implementation bugs)? - // TODO: What happens if #pubkeys for `issue_invite` differs from the value - // when calling `handle_issue_invite`? Should we cache the value at the - // start of this method? - let cred = lazy.handle_issue_invite(request, response, this.#pubKeys);

- // Store the new credentials as a priority. - this.#changeCredentials(loxId, cred); + const cred = await this.#changeCredentials(loxId, async cred => { + let request = lazy.issue_invite(cred, this.#encTable, this.#pubKeys); + let response = await this.#makeRequest("issueinvite", request); + // TODO: Do we ever expect handle_issue_invite to fail (beyond + // implementation bugs)? + // TODO: What happens if #pubkeys for `issue_invite` differs from the value + // when calling `handle_issue_invite`? Should we cache the value at the + // start of this method? + return lazy.handle_issue_invite(request, response, this.#pubKeys); + });

const invite = lazy.prepare_invite(cred); this.#invites.push(invite); @@ -804,35 +847,26 @@ class LoxImpl { }

async #blockageMigration(loxId) { - let request; - try { - request = lazy.check_blockage(this.#getCredentials(loxId), this.#pubKeys); - } catch { - lazy.logger.log("Not ready for blockage migration"); - return false; - } - let response = await this.#makeRequest("checkblockage", request); - // NOTE: If a later method fails, we should be ok to re-call "checkblockage" - // from the Lox authority. So there shouldn't be any adverse side effects to - // loosing migrationCred. - // TODO: Confirm this is safe to lose. - const migrationCred = lazy.handle_check_blockage( - this.#getCredentials(loxId), - response - ); - request = lazy.blockage_migration( - this.#getCredentials(loxId), - migrationCred, - this.#pubKeys - ); - response = await this.#makeRequest("blockagemigration", request); - const cred = lazy.handle_blockage_migration( - this.#getCredentials(loxId), - response, - this.#pubKeys + return Boolean( + await this.#changeCredentials(loxId, async cred => { + let request; + try { + request = lazy.check_blockage(cred, this.#pubKeys); + } catch { + lazy.logger.log("Not ready for blockage migration"); + return null; + } + let response = await this.#makeRequest("checkblockage", request); + // NOTE: If a later method fails, we should be ok to re-call "checkblockage" + // from the Lox authority. So there shouldn't be any adverse side effects to + // loosing migrationCred. + // TODO: Confirm this is safe to lose. + const migrationCred = lazy.handle_check_blockage(cred, response); + request = lazy.blockage_migration(cred, migrationCred, this.#pubKeys); + response = await this.#makeRequest("blockagemigration", request); + return lazy.handle_blockage_migration(cred, response, this.#pubKeys); + }) ); - this.#changeCredentials(loxId, cred); - return true; }

/** @@ -850,25 +884,26 @@ class LoxImpl { // attempt trust promotion instead return this.#trustMigration(loxId); } - let request = lazy.level_up( - this.#getCredentials(loxId), - this.#encTable, - this.#pubKeys + return Boolean( + await this.#changeCredentials(loxId, async cred => { + let request = lazy.level_up(cred, this.#encTable, this.#pubKeys); + let response; + try { + response = await this.#makeRequest("levelup", request); + } catch (error) { + if ( + error instanceof LoxError && + error.code === LoxError.ErrorResponse + ) { + // Not an error. + lazy.logger.debug("Not ready for level up", error); + return null; + } + throw error; + } + return lazy.handle_level_up(request, response, this.#pubKeys); + }) ); - let response; - try { - response = await this.#makeRequest("levelup", request); - } catch (error) { - if (error instanceof LoxError && error.code === LoxError.ErrorResponse) { - // Not an error. - lazy.logger.debug("Not ready for level up", error); - return false; - } - throw error; - } - const cred = lazy.handle_level_up(request, response, this.#pubKeys); - this.#changeCredentials(loxId, cred); - return true; }

/** @@ -884,42 +919,37 @@ class LoxImpl { this.#getPubKeys(); return false; } - let request; - try { - request = lazy.trust_promotion( - this.#getCredentials(loxId), - this.#pubKeys - ); - } catch (err) { - // This function is called routinely during the background tasks without - // previous checks on whether an upgrade is possible, so it is expected to - // fail with a certain frequency. Therefore, do not relay the error to the - // caller and just log the message for debugging. - lazy.logger.debug("Not ready to upgrade", err); - return false; - } + return Boolean( + await this.#changeCredentials(loxId, async cred => { + let request; + try { + request = lazy.trust_promotion(cred, this.#pubKeys); + } catch (err) { + // This function is called routinely during the background tasks without + // previous checks on whether an upgrade is possible, so it is expected to + // fail with a certain frequency. Therefore, do not relay the error to the + // caller and just log the message for debugging. + lazy.logger.debug("Not ready to upgrade", err); + return null; + }

- let response = await this.#makeRequest("trustpromo", request); - // FIXME: Store response to "trustpromo" in case handle_trust_promotion - // or "trustmig" fails. The Lox authority will not accept a re-request - // to "trustpromo" with the same credentials. - let promoCred = lazy.handle_trust_promotion(request, response); - lazy.logger.debug("Formatted promotion cred: ", promoCred); - - request = lazy.trust_migration( - this.#getCredentials(loxId), - promoCred, - this.#pubKeys + let response = await this.#makeRequest("trustpromo", request); + // FIXME: Store response to "trustpromo" in case handle_trust_promotion + // or "trustmig" fails. The Lox authority will not accept a re-request + // to "trustpromo" with the same credentials. + let promoCred = lazy.handle_trust_promotion(request, response); + lazy.logger.debug("Formatted promotion cred: ", promoCred); + + request = lazy.trust_migration(cred, promoCred, this.#pubKeys); + response = await this.#makeRequest("trustmig", request); + lazy.logger.debug("Got new credential: ", response); + + // FIXME: Store response to "trustmig" in case handle_trust_migration + // fails. The Lox authority will not accept a re-request to "trustmig" with + // the same credentials. + return lazy.handle_trust_migration(request, response); + }) ); - response = await this.#makeRequest("trustmig", request); - lazy.logger.debug("Got new credential: ", response); - - // FIXME: Store response to "trustmig" in case handle_trust_migration - // fails. The Lox authority will not accept a re-request to "trustmig" with - // the same credentials. - let cred = lazy.handle_trust_migration(request, response); - this.#changeCredentials(loxId, cred); - return true; }

/**

View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/compare/8a4eb9d...