Closed Bug 1462891 (CVE-2018-12367) Opened 2 years ago Closed 2 years ago

PerformanceNavigationTiming should reduce the time precision

Categories

(Core :: DOM: Security, defect, P2)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 61+ fixed
firefox60 --- wontfix
firefox61 + fixed
firefox62 + fixed

People

(Reporter: baku, Assigned: tjr)

Details

(Keywords: sec-moderate, Whiteboard: [domsecurity-active][adv-main61+][adv-esr60.1+][post-critsmash-triage])

Attachments

(2 files, 1 obsolete file)

It doesn't seem that PerformanceNavigationTiming::{UnloadEventStart,UnloadEventEnd,DomInteractive,...} use nsRFPService::ReduceTimePrecisionAsMSecs.
Tom, can you take a look?
Flags: needinfo?(tom)
Component: DOM → DOM: Security
I think you're right. This doesn't affect Tor; it does affect Resist Fingerprinting and Spectre.
Group: dom-core-security
Flags: needinfo?(tom)
Assignee: nobody → tom
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Are these timing issues that can be used in Spectre attacks sec-high? Or is this one less serious?
Flags: needinfo?(tom)
No, I'd say the resolution is quite good. If that's what we're calling sec-high (I believe we don't know of a practical vector for exploitation even with a high res timer) then yea this is sec-high.
Flags: needinfo?(tom)
Attached patch central.patch (obsolete) — Splinter Review
Attachment #8980420 - Flags: review?(amarchesini)
Attachment #8980420 - Attachment is patch: true
Comment on attachment 8980420 [details] [diff] [review]
central.patch

Review of attachment 8980420 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the macro.

::: dom/performance/PerformanceNavigationTiming.cpp
@@ +26,5 @@
>  {
> +  DOMHighResTimeStamp rawValue =
> +    mPerformance->GetDOMTiming()->GetUnloadEventStartHighRes();
> +
> +  if (mPerformance->IsSystemPrincipal())

{}

wondering if we can have a macro here:

#define REDUCE_TIME_PRECISION                               \
  if (mPerformance->IsSystemPrincipal()) {                  \
    return rawValue;                                        \
  }                                                         \
  return nsRFPService::ReduceTimePrecisionAsMSecs(rawValue, \
    mPerformance->GetRandomTimelineSeed());
Attachment #8980420 - Flags: review?(amarchesini) → review+
Attached patch central.patchSplinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily, one also needs to find a Spectre gadget.

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?  All of them.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Not difficult.

How likely is this patch to cause regressions; how much testing does it need?  It might cause some intermittents.
Attachment #8980420 - Attachment is obsolete: true
Attachment #8980564 - Flags: sec-approval?
Attachment #8980564 - Flags: review+
After checking with dveditz, I sent this to try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de0976144eecf31b1e44ec14773a1beddefd3668&selectedJob=180263607

I'm going to need to clean up at least /fetch/api/redirect/redirect-count.any.html but I think that's it.
Comment on attachment 8980564 [details] [diff] [review]
central.patch

Clearing sec-approval? since Dan made this a sec-moderate and it doesn't need approval to land on trunk.

Should this ride the trains or be backported, do you think?
Attachment #8980564 - Flags: sec-approval?
(In reply to Al Billings [:abillings] from comment #9)
> Comment on attachment 8980564 [details] [diff] [review]
> central.patch
> 
> Clearing sec-approval? since Dan made this a sec-moderate and it doesn't
> need approval to land on trunk.
> 
> Should this ride the trains or be backported, do you think?

We're earlyish in beta so I guess I'd like to uplift it there and then it ride out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe352021e444c9e94fbb6869107b498259b30302

How sure are we that we don't eventually want this on ESR60 as well?
Flags: needinfo?(tom)
Flags: in-testsuite+
We probably do =)
Flags: needinfo?(tom)
https://hg.mozilla.org/mozilla-central/rev/fe352021e444
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Please nominate this for uplift whenever you feel it's had enough bake time on Nightly.
Flags: needinfo?(tom)
Attached patch esr60.patchSplinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:  It expands our Spectre timing mitigations to cover a situation we missed.

User impact if declined: Low impact; but it would be good to be thorough in ESR.

Fix Landed on Version: 62, requesting uplift to 61.

Risk to taking this patch (and alternatives if risky): Low risk, includes test coverage.
Flags: needinfo?(tom)
Attachment #8983154 - Flags: approval-mozilla-esr60?
Comment on attachment 8980564 [details] [diff] [review]
central.patch

Approval Request Comment
[Feature/Bug causing the regression]: We missed a component when we developed our spectre timing mitigations.

[User impact if declined]: Low impact but it would be nice to be thorough in our coverage

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: Not really.

[Why is the change risky/not risky?]: Small affected component, includes test
Attachment #8980564 - Flags: approval-mozilla-beta?
Comment on attachment 8980564 [details] [diff] [review]
central.patch

Spectre mitigation for a spot we missed before. Approved for 61.0b12 and ESR 60.1.
Attachment #8980564 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8983154 - Attachment is patch: true
Attachment #8983154 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main61+][adv-esr60.1+]
Flags: qe-verify-
Whiteboard: [domsecurity-active][adv-main61+][adv-esr60.1+] → [domsecurity-active][adv-main61+][adv-esr60.1+][post-critsmash-triage]
Alias: CVE-2018-12367
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.