From 97875bccddf8ee600aa9a15652ba84b8d262be11 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 26 May 2020 11:20:03 +0000 Subject: [PATCH] Bug 1637178 - Distinguish local DB hijacks from sign errors in Uptake Telemetry r=Gijs Differential Revision: https://phabricator.services.mozilla.com/D75529 --- services/common/uptake-telemetry.js | 2 + services/settings/RemoteSettingsClient.jsm | 107 ++++++++++++------ .../test/unit/test_remote_settings.js | 79 +++++++++++-- .../unit/test_remote_settings_signatures.js | 91 ++++++--------- 4 files changed, 180 insertions(+), 99 deletions(-) diff --git a/services/common/uptake-telemetry.js b/services/common/uptake-telemetry.js index a501df996bf4..0fc5d9d254e9 100644 --- a/services/common/uptake-telemetry.js +++ b/services/common/uptake-telemetry.js @@ -122,6 +122,7 @@ class UptakeTelemetry { * Only supported in Events Telemetry: * * - `SHUTDOWN_ERROR`: Error occuring during shutdown. + * - `CORRUPTION_ERROR`: Error related to corrupted local data. * * @type {Object} */ @@ -130,6 +131,7 @@ class UptakeTelemetry { ...UptakeTelemetry.HISTOGRAM_LABELS, // Events only. SHUTDOWN_ERROR: "shutdown_error", + CORRUPTION_ERROR: "corruption_error", }; } diff --git a/services/settings/RemoteSettingsClient.jsm b/services/settings/RemoteSettingsClient.jsm index 36fcad1cfd54..43a6bddc205b 100644 --- a/services/settings/RemoteSettingsClient.jsm +++ b/services/settings/RemoteSettingsClient.jsm @@ -133,13 +133,22 @@ class InvalidSignatureError extends Error { } } -class MissingSignatureError extends Error { +class MissingSignatureError extends InvalidSignatureError { constructor(cid) { - super(`Missing signature (${cid})`); + super(cid); + this.message = `Missing signature (${cid})`; this.name = "MissingSignatureError"; } } +class CorruptedDataError extends InvalidSignatureError { + constructor(cid) { + super(cid); + this.message = `Corrupted local data (${cid})`; + this.name = "CorruptedDataError"; + } +} + class UnknownCollectionError extends Error { constructor(cid) { super(`Unknown Collection "${cid}"`); @@ -218,6 +227,9 @@ class RemoteSettingsClient extends EventEmitter { static get MissingSignatureError() { return MissingSignatureError; } + static get CorruptedDataError() { + return CorruptedDataError; + } static get UnknownCollectionError() { return UnknownCollectionError; } @@ -371,7 +383,7 @@ class RemoteSettingsClient extends EventEmitter { if (syncIfEmpty && ObjectUtils.isEmpty(metadata)) { // No sync occured yet, may have records from dump but no metadata. console.debug( - `Required metadata for ${this.identifier}, fetching from server.` + `${this.identifier} Required metadata, fetching from server.` ); metadata = await this.httpClient().getData(); await this.db.saveMetadata(metadata); @@ -565,9 +577,12 @@ class RemoteSettingsClient extends EventEmitter { ); } } catch (e) { - if (e instanceof RemoteSettingsClient.InvalidSignatureError) { + if (e instanceof InvalidSignatureError) { // Signature verification failed during synchronization. - reportStatus = UptakeTelemetry.STATUS.SIGNATURE_ERROR; + reportStatus = + e instanceof CorruptedDataError + ? UptakeTelemetry.STATUS.CORRUPTION_ERROR + : UptakeTelemetry.STATUS.SIGNATURE_ERROR; // If sync fails with a signature error, it's likely that our // local data has been modified in some way. // We will attempt to fix this by retrieving the whole @@ -679,9 +694,6 @@ class RemoteSettingsClient extends EventEmitter { if (e instanceof RemoteSettingsClient.NetworkOfflineError) { reportStatus = UptakeTelemetry.STATUS.NETWORK_OFFLINE_ERROR; - } else if (e instanceof RemoteSettingsClient.MissingSignatureError) { - // Collection metadata has no signature info, no need to retry. - reportStatus = UptakeTelemetry.STATUS.SIGNATURE_ERROR; } else if (e instanceof IDBHelpers.ShutdownError) { reportStatus = UptakeTelemetry.STATUS.SHUTDOWN_ERROR; } else if (/unparseable/.test(e.message)) { @@ -739,8 +751,8 @@ class RemoteSettingsClient extends EventEmitter { async _validateCollectionSignature(records, timestamp, metadata) { const start = Cu.now() * 1000; - if (!metadata || !metadata.signature) { - throw new RemoteSettingsClient.MissingSignatureError(this.identifier); + if (!metadata?.signature) { + throw new MissingSignatureError(this.identifier); } if (!this._verifier) { @@ -767,7 +779,7 @@ class RemoteSettingsClient extends EventEmitter { this.signerName )) ) { - throw new RemoteSettingsClient.InvalidSignatureError(this.identifier); + throw new InvalidSignatureError(this.identifier); } if (gTimingEnabled) { const end = Cu.now() * 1000; @@ -867,33 +879,60 @@ class RemoteSettingsClient extends EventEmitter { metadata ); } catch (e) { - // Signature failed, clear local data. + console.error( + `${this.identifier} Signature failed ${retry ? "again" : ""} ${e}` + ); + if (!(e instanceof InvalidSignatureError)) { + // If it failed for any other kind of error (eg. shutdown) + // then give up quickly. + throw e; + } + + // In order to distinguish signature errors that happen + // during sync, from hijacks of local DBs, we will verify + // the signature on the data that we had before syncing. + let localTrustworthy = false; + try { + await this._validateCollectionSignature( + localRecords, + localTimestamp, + localMetadata + ); + localTrustworthy = true; + } catch (sigerr) { + if (!(sigerr instanceof InvalidSignatureError)) { + // If it fails for other reason, keep original error and give up. + throw sigerr; + } + } + + // Signature failed, clear local DB because it contains + // bad data (local + remote changes). console.debug(`${this.identifier} clear local data`); await this.db.clear(); - // If signature failed again after retry, then - // we restore the local data that we had before sync. - if (retry) { - try { - // Make sure the local data before sync was not tampered. - await this._validateCollectionSignature( - localRecords, - localTimestamp, - localMetadata - ); - // Signature of data before sync is good. Restore it. - await this.db.importBulk(localRecords); - await this.db.saveLastModified(localTimestamp); - await this.db.saveMetadata(localMetadata); - } catch (_) { - // Local data before sync was tampered. Restore dump if available. - if ( - await Utils.hasLocalDump(this.bucketName, this.collectionName) - ) { - await this._importJSONDump(); - } - } + if (!localTrustworthy && !retry) { + // Local data was tampered, throw and it will retry from empty DB. + console.error(`${this.identifier} local data was corrupted`); + throw new CorruptedDataError(this.identifier); + } else if (localTrustworthy) { + // Signature of data before importing changes was good. + // If we retry, we will pull and import changes again on top of them. + // If we retried already, we will restore the local data before + // throwing. + await this.db.importBulk(localRecords); + await this.db.saveLastModified(localTimestamp); + await this.db.saveMetadata(localMetadata); + } else if ( + // The local data was tampered. + // We retried and signature failed again. + // So restore the dump if available. + await Utils.hasLocalDump(this.bucketName, this.collectionName) + ) { + console.info(`${this.identifier} restore dump`); + await this._importJSONDump(); } + throw e; } } else { diff --git a/services/settings/test/unit/test_remote_settings.js b/services/settings/test/unit/test_remote_settings.js index 889680d0785c..0fc9283f06b7 100644 --- a/services/settings/test/unit/test_remote_settings.js +++ b/services/settings/test/unit/test_remote_settings.js @@ -6,6 +6,9 @@ const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); const { AppConstants } = ChromeUtils.import( "resource://gre/modules/AppConstants.jsm" ); +const { ObjectUtils } = ChromeUtils.import( + "resource://gre/modules/ObjectUtils.jsm" +); const IS_ANDROID = AppConstants.platform == "android"; @@ -68,6 +71,10 @@ function run_test() { "/v1/buckets/monitor/collections/changes/records", handleResponse ); + server.registerPathHandler( + "/v1/buckets/main/collections/password-fields", + handleResponse + ); server.registerPathHandler( "/v1/buckets/main/collections/password-fields/changeset", handleResponse @@ -304,8 +311,8 @@ add_task(async function test_get_ignores_synchronization_errors() { }); add_task(clear_state); -add_task(async function test_get_can_verify_signature() { - // No signature in metadata. +add_task(async function test_get_verify_signature_no_sync() { + // No signature in metadata, and no sync if empty. let error; try { await client.get({ verifySignature: true, syncIfEmpty: false }); @@ -313,7 +320,35 @@ add_task(async function test_get_can_verify_signature() { error = e; } equal(error.message, "Missing signature (main/password-fields)"); +}); +add_task(clear_state); +add_task(async function test_get_can_verify_signature_pulled() { + // Populate the local DB (only records, eg. loaded from dump previously) + await client._importJSONDump(); + + let calledSignature; + client._verifier = { + async asyncVerifyContentSignature(serialized, signature) { + calledSignature = signature; + return true; + }, + }; + + // No metadata in local DB, but gets pulled and then verifies. + ok(ObjectUtils.isEmpty(await client.db.getMetadata()), "Metadata is empty"); + + await client.get({ verifySignature: true }); + + ok( + !ObjectUtils.isEmpty(await client.db.getMetadata()), + "Metadata was pulled" + ); + ok(calledSignature.endsWith("some-sig"), "Signature was verified"); +}); +add_task(clear_state); + +add_task(async function test_get_can_verify_signature() { // Populate the local DB (record and metadata) await client.maybeSync(2000); @@ -325,12 +360,14 @@ add_task(async function test_get_can_verify_signature() { return JSON.parse(serialized).data.length == 1; }, }; + ok(await Utils.hasLocalData(client), "Local data was populated"); await client.get({ verifySignature: true }); - ok(calledSignature.endsWith("abcdef")); + + ok(calledSignature.endsWith("abcdef"), "Signature was verified"); // It throws when signature does not verify. await client.db.delete("9d500963-d80e-3a91-6e74-66f3811b99cc"); - error = null; + let error = null; try { await client.get({ verifySignature: true }); } catch (e) { @@ -967,7 +1004,9 @@ wNuvFqc= ], status: { status: 200, statusText: "OK" }, responseBody: { - metadata: {}, + metadata: { + signature: {}, + }, timestamp: 4000, changes: [ { @@ -995,7 +1034,9 @@ wNuvFqc= ], status: { status: 200, statusText: "OK" }, responseBody: { - metadata: {}, + metadata: { + signature: {}, + }, timestamp: 5000, changes: [ { @@ -1084,6 +1125,24 @@ wNuvFqc= ], }, }, + "GET:/v1/buckets/main/collections/password-fields": { + sampleHeaders: [ + "Access-Control-Allow-Origin: *", + "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff", + "Content-Type: application/json; charset=UTF-8", + "Server: waitress", + 'Etag: "3000"', + ], + status: { status: 200, statusText: "OK" }, + responseBody: { + data: { + signature: { + signature: "some-sig", + x5u: `http://localhost:${port}/fake-x5u`, + }, + }, + }, + }, "GET:/v1/buckets/main/collections/password-fields/changeset?_expected=1337": { sampleHeaders: [ "Access-Control-Allow-Origin: *", @@ -1094,7 +1153,9 @@ wNuvFqc= ], status: { status: 200, statusText: "OK" }, responseBody: { - metadata: {}, + metadata: { + signature: {}, + }, timestamp: 3000, changes: [ { @@ -1202,7 +1263,9 @@ wNuvFqc= status: { status: 200, statusText: "OK" }, responseBody: { timestamp: 3000, - metadata: {}, + metadata: { + signature: {}, + }, changes: [ { id: "1f5c98b9-6d93-4c13-aa26-978b38695096", diff --git a/services/settings/test/unit/test_remote_settings_signatures.js b/services/settings/test/unit/test_remote_settings_signatures.js index fff9a2712759..0f96750870f7 100644 --- a/services/settings/test/unit/test_remote_settings_signatures.js +++ b/services/settings/test/unit/test_remote_settings_signatures.js @@ -9,6 +9,9 @@ const { RemoteSettings } = ChromeUtils.import( const { UptakeTelemetry } = ChromeUtils.import( "resource://services-common/uptake-telemetry.js" ); +const { TelemetryTestUtils } = ChromeUtils.import( + "resource://testing-common/TelemetryTestUtils.jsm" +); const PREF_SETTINGS_SERVER = "services.settings.server"; const PREF_SIGNATURE_ROOT = "security.content.signature.root_hash"; @@ -590,6 +593,7 @@ add_task(async function test_check_synchronization_with_signatures() { // the local DB contains same id as RECORD2 and a fake record. // the final server collection contains RECORD2 and RECORD3 await client.db.clear(); + await client.db.saveMetadata({ signature: { x5u, signature: "abc" } }); await client.db.create( { ...RECORD2, last_modified: 1234567890, serialNumber: "abc" }, { synced: true, useRecordId: true } @@ -602,11 +606,34 @@ add_task(async function test_check_synchronization_with_signatures() { syncData = data; }); - await client.maybeSync(5000); + // Clear events snapshot. + TelemetryTestUtils.assertEvents([], {}, { process: "dummy" }); - // Local data was replaced. But we use records IDs to determine - // what was created and deleted. So fake local data will appear - // in the sync event. + await withFakeChannel("nightly", async () => { + // Events telemetry is sampled on released, use fake channel. + await client.maybeSync(5000); + + // We should report a corruption_error. + TelemetryTestUtils.assertEvents([ + [ + "uptake.remotecontent.result", + "uptake", + "remotesettings", + UptakeTelemetry.STATUS.CORRUPTION_ERROR, + { + source: client.identifier, + duration: v => v > 0, + trigger: "manual", + }, + ], + ]); + }); + + // The local data was corrupted, and the Telemetry status reflects it. + // But the sync overwrote the bad data and was eventually a success. + // Since local data was replaced, we use records IDs to determine + // what was created and deleted. And bad local data will appear + // in the sync event as deleted. equal(syncData.current.length, 2); equal(syncData.created.length, 1); equal(syncData.created[0].id, RECORD3.id); @@ -636,7 +663,7 @@ add_task(async function test_check_synchronization_with_signatures() { metadata: { signature: { x5u, - signature: "wrong-sig-here-too", + signature: "aaaaaaaaaaaaaaaaaaaaaaaa", // sig verifier wants proper length or will crash. }, }, changes: [ @@ -671,13 +698,13 @@ add_task(async function test_check_synchronization_with_signatures() { checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements); // When signature fails after retry, the local data present before sync - // should be maintained. + // should be maintained (if its signature is valid). ok( arrayEqual( (await client.get()).map(r => r.id), [RECORD3.id, RECORD2.id] ), - "Remote changes were not changed" + "Local records were not changed" ); // And local data should still be valid. await client.get({ verifySignature: true }); // Not raising. @@ -703,54 +730,4 @@ add_task(async function test_check_synchronization_with_signatures() { } // Since local data was tampered, it was cleared. equal((await client.get()).length, 0, "Local database is now empty."); - - // - // 11. - // - collection: [] -> [] - // - timestamp: 4000 -> 6000 - // - // Check that we don't apply changes when signature is missing in remote. - - const RESPONSE_NO_SIG = { - sampleHeaders: [ - "Content-Type: application/json; charset=UTF-8", - `ETag: \"123456\"`, - ], - status: { status: 200, statusText: "OK" }, - responseBody: JSON.stringify({ - metadata: { - last_modified: 123456, - }, - changes: [], - timestamp: 123456, - }), - }; - - const missingSigResponses = { - // In this test, we deliberately serve metadata without the signature attribute. - // As if the collection was not signed. - "GET:/v1/buckets/main/collections/signed/changeset?_expected=6000": [ - RESPONSE_NO_SIG, - ], - }; - - // Local data was empty after last test. - equal((await client.get()).length, 0); - - startHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY); - registerHandlers(missingSigResponses); - try { - await client.maybeSync(6000); - do_throw("Sync should fail (the signature is missing)"); - } catch (e) { - equal((await client.get()).length, 0, "Local remains empty"); - } - - // Ensure that the failure is reflected in the accumulated telemetry: - endHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY); - expectedIncrements = { - [UptakeTelemetry.STATUS.SIGNATURE_ERROR]: 1, - [UptakeTelemetry.STATUS.SIGNATURE_RETRY_ERROR]: 0, // Not retried since missing. - }; - checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements); });