Closed Bug 1560495 (CVE-2019-11743) Opened 1 year ago Closed 1 year ago

[Navigation Timing] unload event test failures

Categories

(Core :: DOM: Core & HTML, defect, P2)

66 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 69+ fixed
firefox-esr68 69+ verified
firefox69 + verified
firefox70 + verified

People

(Reporter: yoav, Assigned: whawkins)

References

(Depends on 1 open bug)

Details

(Keywords: csectype-sop, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main69+][adv-esr68.1+][adv-esr60.9+])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.90 Safari/537.36

Steps to reproduce:

http://w3c-test.org/navigation-timing/unload-event-same-origin-check.html

Actual results:

3 of the test cases are failing

Expected results:

The spec was recently changed as TAO checks in the context of Navigation Timing made little sense: https://github.com/w3c/navigation-timing/pull/108

These failures can have security implications as exposing the unload event's timing can contain sensitive information about the previous page

Assignee: nobody → whawkins
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Priority: -- → P2
Keywords: checkin-needed
Keywords: checkin-needed

I'm not sure if the checkin-needed tag will work because this is a Security Issue. Can you advise, :baku? If that tag does not work, would you mind landing it?

Flags: needinfo?(amarchesini)
Keywords: checkin-needed

https://hg.mozilla.org/integration/autoland/rev/65f2110d9bb9a28c621117c24526c1613c261192

checkin-needed for security bugs can be slower because fewer people can access that, but eventually the bugs shall land.

Flags: needinfo?(amarchesini)
Keywords: checkin-needed

Backed out for wpt failures at navigation-timing/nav2_test_unloadEvents_with_cross_origin_redirect_opt_in.html:

https://hg.mozilla.org/integration/autoland/rev/31d23e8d5dfcc1e307bc14143b75858e85bc73ef

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=65f2110d9bb9a28c621117c24526c1613c261192
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=254389939&repo=autoland
TEST-UNEXPECTED-FAIL | /navigation-timing/nav2_test_unloadEvents_with_cross_origin_redirect_opt_in.html | Navigation Timing 2 WPT - uncaught exception: Error: assert_not_equals: Expected unloadEventStart to not be 0. got disallowed value 0
TEST-UNEXPECTED-PASS | /navigation-timing/nav2_test_unloadEvents_with_cross_origin_redirects.html | Navigation Timing 2 WPT - expected FAIL

Flags: needinfo?(whawkins)

Ugh. The revert came because the tests in our tree are not up to date. I have an updated patch that disables the tests temporarily which, hopefully, means that this can land. I asked for a sanity check of the new patch and then we'll be able to move forward, I hope.

Will

Flags: needinfo?(whawkins)

(You can ask jgraham about the next wpt merge)

(In reply to Olli Pettay [:smaug] from comment #6)

(You can ask jgraham about the next wpt merge)

I did, thanks :-) He's on PTO. Back tomorrow and he will do a wpt merge later this week, according to those in #automation. Thanks for the pointer :-)

You could also just locally apply the upstream test changes and land them, fwiw.

I did that. They were auto reverted for causing failure in wpt tests.

Just to make sure we're on the same page, https://hg.mozilla.org/integration/autoland/rev/65f2110d9bb9a28c621117c24526c1613c261192 doesn't have wpt changes other than failure annotation removal, right?. What I meant is that you could apply the changes from https://github.com/web-platform-tests/wpt/commit/94f51553f1413cf2118b9bf89762bfad16d8a18a locally as part of your changeset and push them to our tree; presumably that should fix the test issues.

Ah. Yes, I could do that. Sorry, I now understand. I am not 100% confident in doing that at this point. I don't want to mess up things that the automation team normally handles. This workaround is ok for now. Thanks for the suggestion -- it is definitely something that I will do as I get more confident working with the tree, etc.

Thanks again, :bzbarsky!

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Group: dom-core-security → core-security-release

Do we need to backport this to Beta/ESR68/ESR60?

Flags: needinfo?(whawkins)
Flags: in-testsuite+

I honestly don't know. There are a few things to consider:

  1. This change is the result of a recent change to a version of the spec that is not yet even official.

  2. This is part of the Level 2 of the Navigation Timing API. I'm not sure there is an official stance on which version of Firefox supports which level of the API. If Beta/ESR68/ESR60 are advertised as supporting this level of the spec, then I think we have to backport. If they do not support Level 2, then there's no reason to backport.

I hope that helps you make a decision. I'm sorry that I don't have more information or an opinion.

Will

Andrea, maybe you can help shed some light?

Flags: needinfo?(whawkins) → needinfo?(amarchesini)

PerformanceNavigationTiming::UnloadEventStart() and UnloadEventEnd() are in firefox since version 58.
I think it's good and important to uplift the patch to beta/esr68/esr60: same-origin check has been a problem elsewhere recently (canvas API, for instance) and we should land similar fixes as soon as we can. The patch doesn't introduce extra complexity, and it's safe to uplift it.

Flags: needinfo?(amarchesini)

Thanks for the feedback. Looks like this patch grafts cleanly to Beta and ESR68 but will need some minor rebasing for ESR60. Can you please attach that rebased patch and then request Beta/ESR68/ESR60 approval where applicable, Will? Thanks!

Thanks for the feedback, baku!

I'd be happy to do that, but it'll be the first time that I am doing such an uplift to an ESR. Is there documentation on where those branches live in the tree and how to make the request for uplift? Thanks Ryan!

Flags: needinfo?(ryanvm)

Assuming you're using a unified repository, you can just run |hg update -C esr60|. Otherwise, if you need to clone it for some reason, you can pull it from https://hg.mozilla.org/releases/mozilla-esr60

To request approval, click the Details link on the attachment, select '?' from the appropriate approval-mozilla-<whatever> dropdown and answer the questions which pop up.

Flags: needinfo?(ryanvm)

FWIW, you don't have to let the ESR60 rebase block the Beta and ESR68 uplifts. Especially for Beta, it would be good to get the patch in front of a wider audience sooner.

Comment on attachment 9075222 [details]
Bug 1560495: Update unload event security information for Navigation-Timing Level 2 specification.

Beta/Release Uplift Approval Request

  • User impact if declined: This is a security bug and could expose users to expose information about their browsing history.
  • 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): This patch adds no increased complexity to the code base and shields users from potential information leakage.
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a security bug and could expose users to expose information about their browsing history.
  • User impact if declined: This is a security bug and could expose users to expose information about their browsing history.
  • Fix Landed on Version: Nightly
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch adds no increased complexity to the code base and shields users from potential information leakage.
  • String or UUID changes made by this patch:
Flags: needinfo?(whawkins)
Attachment #9075222 - Flags: approval-mozilla-esr68?
Attachment #9075222 - Flags: approval-mozilla-beta?

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: If this is not addressed in this ESR, it is possible that user browsing history will be exposed through timing side-channels.
  • User impact if declined: If this is not addressed in this ESR, it is possible that user browsing history will be exposed through timing side-channels.
  • Fix Landed on Version: Nightly
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The change does not add any additional functionality or complexity.
  • String or UUID changes made by this patch:
Attachment #9079241 - Flags: approval-mozilla-esr60?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)

FWIW, you don't have to let the ESR60 rebase block the Beta and ESR68 uplifts. Especially for Beta, it would be good to get the patch in front of a wider audience sooner.

Thanks, Ryan! All flags are updated appropriately (I think) and the rebased patch for ESR60 uplift has been attached. I hope that I handled adding all the flags, etc correctly. Please let me know if I did something wrong.

Flags: needinfo?(ryanvm)

Comment on attachment 9075222 [details]
Bug 1560495: Update unload event security information for Navigation-Timing Level 2 specification.

Fixes a security issue which can cause browsing history leakage. Approved for 68.0b7.

Flags: needinfo?(ryanvm)
Attachment #9075222 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9075222 [details]
Bug 1560495: Update unload event security information for Navigation-Timing Level 2 specification.

This has baked on Nightly and Beta without issue for awhile now. Approved for 68.1esr and 60.9esr.

Attachment #9075222 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9079241 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

Confirmed issue with: 69.0a1 (2019-06-21), 70.0a1 (2019-08-04) on Windows 10.
Fix verified with 69.0b10, esr68.1.0, 70.0a1 (2019-08-04), on Windows 10, macOS 10.14,

With 60.9.0esr - 20190801163606 (treeherder build) there is a test Failing:
Fail No previous document with same origin redirect assert_equals: Expected redirectCount to be 1 expected 1 but got 0

run_test@http://w3c-test.org/navigation-timing/unload-event-same-origin-check.html:101:17
create_test/<@http://w3c-test.org/navigation-timing/unload-event-same-origin-check.html:108:25

@Will, mind taking a look and confirming if that test was indeed supposed to pass as well?

Flags: needinfo?(whawkins)

Absolutely.

Flags: needinfo?(whawkins)
QA Whiteboard: [qa-triaged]

I rolled back to before the fix for this issue landed in esr60 and found that this test is not something that we ever passed in esr60.

We now pass this test in later releases. I believe that if we want to resolve this issue for esr60 we should track it in a separate, non-security-sensitive issue.

Thoughts?

Flags: needinfo?(cristian.fogel)
Flags: needinfo?(brindusa.tot)

Sounds ok, assuming it wouldn't still cause any vulnerability to pop up.

Flags: needinfo?(cristian.fogel)

Ok from my side. Just link the new bug for esr60 here.

Flags: needinfo?(brindusa.tot)
Depends on: 1571989
Depends on: 1571990
No longer depends on: 1571989

Marking bug 1571990 for the follow-up.

Flags: qe-verify+
Alias: CVE-2019-11743
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main69+][adv-esr68.1+][adv-esr60.9+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.