1462891 - (CVE-2018-12367) PerformanceNavigationTiming should reduce the time precision

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

It doesn't seem that PerformanceNavigationTiming::{UnloadEventStart,UnloadEventEnd,DomInteractive,...} use nsRFPService::ReduceTimePrecisionAsMSecs.

Comment 1

[Andrea Marchesini [:baku]]

Tom, can you take a look?

Comment 2

[Tom Ritter [:tjr]]

I think you're right. This doesn't affect Tor; it does affect Resist Fingerprinting and Spectre.

Comment 3

[Daniel Veditz [:dveditz]]

Are these timing issues that can be used in Spectre attacks sec-high? Or is this one less serious?

Comment 4

[Tom Ritter [:tjr]]

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.

Comment 5

[Tom Ritter [:tjr]]

Attachment: central.patch

Comment 6

[Andrea Marchesini [:baku]]

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());

Comment 7

[Tom Ritter [:tjr]]

Attachment: central.patch

[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.

Comment 8

[Tom Ritter [:tjr]]

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 9

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 10

[Tom Ritter [:tjr]]

(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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe352021e444c9e94fbb6869107b498259b30302

How sure are we that we don't eventually want this on ESR60 as well?

Comment 12

[Tom Ritter [:tjr]]

We probably do =)

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fe352021e444

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for uplift whenever you feel it's had enough bake time on Nightly.

Comment 15

[Tom Ritter [:tjr]]

Attachment: esr60.patch

[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.

Comment 16

[Tom Ritter [:tjr]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/30b75eae500b
https://hg.mozilla.org/releases/mozilla-esr60/rev/e62057262371