1474842 - mixed content blocker bypass: insecure sub-resources loaded by a localhost frame in a secure parent document

Status: NEW

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

Description

[Xiaoyin Liu]

Attachment: PoC

Suppose there is a webpage at https://example.com/parent.html. This webpage embeds http://127.0.0.1/child.html in an iframe. The child page loads a JavaScript from http://example.com/js.js . This JavaScript should be blocked as mixed content because the top page is a HTTPS page, but Firefox allows it to load, and the lock pad remains green.

Chrome blocks http://example.com/js.js as expected. Edge doesn't treat localhost as a secure origin.

Reproducible in Firefox Nightly 63.0a1 (2018-07-10) (64-bit) and Firefox Stable 61.0.1 (64-bit).

Comment 1

[Daniel Veditz [:dveditz]]

We need to check the "isSecure" check of the loading context; currently we see the two http: schemes and drop further checks.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Probably we should not only check whether the parent (meaning including context) is https but also if it's a secure context. That check could/should happen somewhere around here:
https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#709

Comment 3

[Niklas]

Attachment: Bug 1474842: MCB: Allow loads if the parent scheme is not https AND if the parent window is not a secure context. r=ckerschb

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16895673c015
MCB: Allow loads if the parent scheme is not https AND if the parent window is not a secure context. r=ckerschb,robwu

Comment 5

[Xiaoyin Liu]

Thank you for fixing this issue. Will this bug get a CVE assigned?

Comment 6

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing android geckoview failures. CLOSED TREE

Backout link : https://hg.mozilla.org/integration/autoland/rev/8077dd2baa24b2e0d9609c03e24807ee64a5d2b0

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=16895673c015eb6955fb602bb487879d796741cd&selectedTaskRun=a5RAmPfKSdaVn5lCPg8JMg.0

Link to failure log :
https://treeherder.mozilla.org/logviewer?job_id=356970217&repo=autoland&lineNumber=2942
https://treeherder.mozilla.org/logviewer?job_id=356970145&repo=autoland&lineNumber=9049
https://treeherder.mozilla.org/logviewer?job_id=356972890&repo=autoland&lineNumber=45198
https://treeherder.mozilla.org/logviewer?job_id=356979025&repo=autoland&lineNumber=1342

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Hey Tom, this has an r+ patch (including a test) but was backed out - is that something you feel like pushing over the finishing line?

Comment 8

[Tom Schuster (MoCo)]

Sure, I guess I will see what try turns up.

Comment 9

[Tom Schuster (MoCo)]

It seems like the Geckoview problems are still there: https://treeherder.mozilla.org/jobs?repo=try&revision=fba594428ecfc840fbf02ab433f050e328618167

Comment 10

[Tom Schuster (MoCo)]

So I haven't really had much luck with working on GeckoView before, so I am trying to debug this a bit by looking at the logs and reading the tests. Hopefully someone from the GeckoView team can help me out with this.

For example:

org.mozilla.geckoview.test.OpenWindowTest#openWindowNullDelegate | org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutException

This test seems to load some kind of page OPEN_WINDOW_PATH. If I am following the code correctly this ends up as a http:// URL because TEST_ENDPOINT uses http://. Furthermore this test uses Notification.requestPermission(), which I think shouldn't be possible in a non-secure context anyway.

Agi would you mind having a look at this?

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

If you want those tests to use https I think the easiest way would be to add the test file to https://searchfox.org/mozilla-central/rev/301e9545863b14ff85aa110fabd4274da4593f9d/mobile/android/moz.build#74 and then load it using https://example.org/tests/junit/<filename>: https://searchfox.org/mozilla-central/rev/301e9545863b14ff85aa110fabd4274da4593f9d/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt#717

Comment 12

[Tom S. (please needinfo tschuster)]

Attachment: WIP: Bug 1474842 - (not working) Update Android tests to use https for Notifications.

Depends on D120622

Comment 13

[Tom Schuster (MoCo)]

I just came back to this, thanks Agi for your hints. Sadly I haven't fully gotten this to work yet. I have to suspicion that the emulator isn't loading the test that I want it to load.

I still see the following warning even after adding the doctype:

[JavaScript Warning: "This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “<!DOCTYPE html>”." {file: "https://example.org/tests/junit/worker/open_window.html" line: 0}]

The console.log calls don't show up either.

Comment 14

[Tom Schuster (MoCo)]

So, it turns out it was indeed loading a 404 page and not open_window.html. The root cause seems to be that when hosted the sub-directory gets flattened and worker/ is removed.

Comment 15

[Tom Schuster (MoCo)]

So I think we need to change the logic in the original patch somehow, because we don't allow loading workers from http://localhost even when on that domain anymore. localhost is a secure-context so we don't early return after the patch. I think fixing this would also resolve the android failures.

Comment 16

[Tom Schuster (MoCo)]

So there is one remaining failure in org.mozilla.geckoview.test.WebExtensionTest#redirectToExtensionResource that points to a behavior change that I am not sure what we want to do about.

(Simplified) http://localhost/foo.bar is a secure context, because localhost is considered such, and loads an other http://example.org/script.js. Previously this was obviously not mixed content because both use https. But now one is a "secure context" and the other uses "https". I think to fix the original issue we would need to break this, or know when "inheriting" https from a parent frame.

Comment 17

[Frederik Braun [:freddy]]

My understanding is that we need to walk the frame tree and not check the protocol but rather always check the isSecureContext bit. (Sorry for the overly long wait)