Closed Bug 1639224 Opened 11 months ago Closed 11 months ago

Verify signature if local timestamp is in the future

Categories

(Firefox :: Remote Settings Client, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 78
Tracking Status
firefox-esr68 77+ fixed
firefox76 --- wontfix
firefox77 + fixed
firefox78 + fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

Details

(Keywords: csectype-other, sec-moderate, Whiteboard: [local attack][post-critsmash-triage][adv-main77-][adv-esr68.9-])

Attachments

(3 files, 2 obsolete files)

An attacker could tamper the local data and store a very high value as the collection timestamp, and the client would always report up-to-date (since the server data would never be newer). And currently we don't verify the signature of local data when up to date.

We could fix this issue by verifying the signature of local data on every sync, even if no new data was pulled from the server.
But this could have an impact on performance, especially on huge collections.

So instead of verifying it on every sync, we could detect the situations when the local timestamp is higher than the server one, and flush/re-sync. Assuming an attacker cannot guess timestamps of publication in advance, we don't verify signatures when timestamps are equal.
If some use-cases need to make sure that the local data is not tampered when they read, they can use .get({ verifySignature: true })

Assignee: nobody → mathieu
Status: NEW → ASSIGNED

Per Slack discussion with Mathieu. Also, this was pushed to autoland:
https://hg.mozilla.org/integration/autoland/rev/55761a2d367b46b7a85e9cb0b34f075cd8e5dc78

Group: mozilla-employee-confidential → firefox-core-security

Comment on attachment 9150626 [details]
Bug 1639224 - Verify signature if local timestamp is in the future

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: From the signature word in the commit message, an attacker could infer that storing a specific value could prevent sync and signature verification to happen.

Gain access to the IndexedDB file of Remote Settings (SQLite in profile) and write a high value in a specific table/row.

  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: 68
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?:
  • How likely is this patch to cause regressions; how much testing does it need?: The nominal behaviour isn't changed. We only now sync & verify the signature when the local timestamp is in the future (local data will then be detected as tampered, flushed and re-synced).

Beta/Release Uplift Approval Request

  • User impact if declined:
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The nominal behaviour isn't changed. We only now sync & verify the signature when the local timestamp is in the future (local data will then be detected as tampered, flushed and re-synced).
  • String changes made/needed:
Attachment #9150626 - Flags: sec-approval?
Attachment #9150626 - Flags: approval-mozilla-beta?
Attached patch 1639224-esr68.diff (obsolete) — Splinter Review

[Approval Request Comment]
If this is not a sec:crit bug, please state case for ESR consideration:
User impact if declined:

  • An attacker could manage to tamper local remote settings and disable updates.

Fix Landed on Version:

  • 77, 78

Risk to taking this patch (and alternatives if risky):

  • Very low. The default behaviour does not change if the local data was not tampered.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Attachment #9151010 - Flags: approval-mozilla-esr68?
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 78
Comment on attachment 9151010 [details] [diff] [review]
1639224-esr68.diff

This patch appears to only have test changes?
Flags: needinfo?(mathieu)
Attachment #9151010 - Flags: approval-mozilla-esr68?
Attached patch patch-esr68-2.diff (obsolete) — Splinter Review

[Approval Request Comment]
If this is not a sec:crit bug, please state case for ESR consideration:
User impact if declined:

An attacker could manage to tamper local remote settings and disable updates.

Fix Landed on Version:

77, 78

Risk to taking this patch (and alternatives if risky):

Very low. The default behaviour does not change if the local data was not tampered.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Attachment #9151010 - Attachment is obsolete: true
Flags: needinfo?(mathieu)
Attachment #9151172 - Flags: approval-mozilla-esr68?
Summary: Verify signature on sync even if up to date → Verify signature if local timestamp is in the future

Comment on attachment 9150626 [details]
Bug 1639224 - Verify signature if local timestamp is in the future

sec-approval+ = dveditz.

I'm not sure what your security model is for threats against local attackers. This patch fixes a problem where a tamperer could set the modified date into the future to prevent updates that would overwrite their modifications. That's good! But it doesn't look like you verify signatures before loading this data (or you would have detected the tampered timestamp), so an attacker that preserves the existing timestamp can still modify the data all they want. At least the next update will happen, but an active attacker could watch for updates and then re-modify the file.

In practice most of our remote settings collections have preferences that will turn off their updates (or use). I'm not sure the problem that was actually solved was much of a problem in practice until we get some kind of secure preference storage.

Attachment #9150626 - Flags: sec-approval? → sec-approval+

Ah, I see bug 1640126 is a follow-up to address that.

Comment on attachment 9150626 [details]
Bug 1639224 - Verify signature if local timestamp is in the future

a=dveditz for beta uplift (checked w/ryanvm)

Attachment #9150626 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9151172 [details] [diff] [review]
patch-esr68-2.diff

a=dveditz for ESR uplift
Attachment #9151172 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Whiteboard: [local attack]

Backed out for XPCShell failures in services/common/tests/unit/test_blocklist_onecrl.js:

https://hg.mozilla.org/releases/mozilla-esr68/rev/07f7d20b2c15a16e40fa809c69367611218595bd

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&group_state=expanded&selectedTaskRun=Cfxk6cnkR52QRa8XsqClZA-0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=c36bb4f2f5e5003ab4df862bd50368180aed0598

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=303568305&repo=mozilla-esr68

[task 2020-05-25T00:03:31.296Z] 00:03:31 INFO - TEST-PASS | services/common/tests/unit/test_blocklist_pinning.js | test_something - [test_something : 157] true == true
[task 2020-05-25T00:03:31.297Z] 00:03:31 INFO - TEST-PASS | services/common/tests/unit/test_blocklist_pinning.js | test_something - [test_something : 167] true == true
[task 2020-05-25T00:03:31.297Z] 00:03:31 INFO - PID 17959 | FATAL ERROR: Non-local network connections are disabled and a connection attempt to firefox.settings.services.mozilla.com (54.192.30.89) was made.
[task 2020-05-25T00:03:31.297Z] 00:03:31 INFO - PID 17959 | You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.
[task 2020-05-25T00:03:31.298Z] 00:03:31 INFO - PID 17959 | ExceptionHandler::GenerateDump cloned child 17988
[task 2020-05-25T00:03:31.298Z] 00:03:31 INFO - PID 17959 | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2020-05-25T00:03:31.298Z] 00:03:31 INFO - PID 17959 | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2020-05-25T00:03:31.299Z] 00:03:31 INFO - <<<<<<<

Flags: needinfo?(mathieu)
Flags: qe-verify-
Whiteboard: [local attack] → [local attack][post-critsmash-triage]

I couldn't manage to try a try build... but the tests I ran locally passed with this patch.

Attachment #9151172 - Attachment is obsolete: true
Flags: needinfo?(mathieu)
Whiteboard: [local attack][post-critsmash-triage] → [local attack][post-critsmash-triage][adv-main77-]
Whiteboard: [local attack][post-critsmash-triage][adv-main77-] → [local attack][post-critsmash-triage][adv-main77-][adv-esr68.9-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.