Don't treat loopback IP addresses (127.0.0.0/8, ::1/128) as mixed content

[Frédéric Wang (:fredw)]

Preliminary step for bug 171934, where we only consider IP addresses 127.0.0.0/8, ::1/128

[Frédéric Wang (:fredw)]

Created attachment 413323 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 413413 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 413418 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 413420 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 413422 [details]
Patch with error logging

[Frédéric Wang (:fredw)]

Created attachment 413564 [details]
Patch (try to fix api ios)

[Frédéric Wang (:fredw)]

Created attachment 413687 [details]
Patch

[Frédéric Wang (:fredw)]

I'm still stuck on the remaining failures:

* imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-content-to-inscope.https.html
  imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-content-to-outscope.https.html

   They do a complicate nesting of resource loading which arrives at service-workers/service-worker/resources/fetch-mixed-content-iframe-inscope-to-*html which loads

   http://127.0.0.1:8800(...)fetch-access-control.py?PNGIMAGE" 
  ./dummy?url=http://127.0.0.1:8800(...)fetch-access-control.py?PNGIMAGE" 

  as images. The test expects them to be blocked, but that's no longer the case with http://127.0.0.1 allowed (and I fail to disable the new behavior with this nested loading of resources). I wonder if this is just revealing an existing issue that was hidden by the fact we block loopback address.

* http/tests/websocket/tests/hybi/non-document-mixed-content-blocked.https.html (Windows). I don't understand why it should behave differently from other platforms (and I can't debug it).

* TestWebKitAPI.TLSVersion.BackForwardHasOnlySecureContent (iOS)
  I'm not really able to debug it, with the public SDK I get a lot of timeout with API tests. I checked that my new code really disables TrustworthyLoopbackIPAddressesEnabled for the URLs loaded before the timeout.

* TestSSL:/webkit/WebKitWebView/insecure-content (GTK).
  This was discussed in but 142469 comment 2 but basically the test assumes http://127.0.0.1 is insecure and we can't easily disable the flag in GTK API tests.

[youenn fablet]

(In reply to Frédéric Wang (:fredw) from comment #8)
> I'm still stuck on the remaining failures:
> 
> *
> imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-
> content-to-inscope.https.html
>  
> imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-
> content-to-outscope.https.html

Are these tests passing in http://wpt.live with your patch?
It might indeed be a 'bug' in our test infra.

[Frédéric Wang (:fredw)]

(In reply to youenn fablet from comment #9)
> (In reply to Frédéric Wang (:fredw) from comment #8)
> > I'm still stuck on the remaining failures:
> > 
> > *
> > imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-
> > content-to-inscope.https.html
> >  
> > imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-
> > content-to-outscope.https.html
> 
> Are these tests passing in http://wpt.live with your patch?
> It might indeed be a 'bug' in our test infra.

mmh, good idea :-) Indeed they still pass with the patch.

[Frédéric Wang (:fredw)]

Created attachment 413820 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 413821 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 413920 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/71332562>

[Michael Catanzaro]

Comment on attachment 413920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=413920&action=review

I'm going to give r=me because I don't see any technical problems with the patch, and I agree with the technical merits of this change. But since Apple has previously expressed concern with this change -- although I hope such concerns have been well-addressed by now -- it ought to be approved by an Apple reviewer as well before landing.

> LayoutTests/http/tests/security/mixedContent/import-ipv4-loopback-script-in-iframe.html:16
> +window.addEventListener("message", function (error) {
> +    window.data = error.data;
> +    shouldBe(`window.data`, `"TypeError: Cross-origin script load denied by Cross-Origin Resource Sharing policy."`);
> +    finishJSTest();
> +}, false);

Nice test.

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-content-to-inscope.https-expected.txt:3
> -PASS Verify Mixed content of fetch() in a Service Worker
> +FAIL Verify Mixed content of fetch() in a Service Worker assert_equals: expected "finish" but got "FAIL(4)finish"

Hm, I see you reported bug #218795 for this. Interesting. Certainly merits further investigation.

> Source/WTF/ChangeLog:8
> +        * Scripts/Preferences/WebPreferencesExperimental.yaml: Introduce a new experimental
> +        preference for the new behavior (enabled by default) in case some ports or users need to
> +        disable it.

Once upon a time, I successfully proposed a rule that experimental preferences must not be enabled by default, because if it's enabled by default, it's no longer experimental. I don't remember that rule ever changing, but it seems that it's clearly not being followed anymore. So I guess this is OK. I don't see why it needs to be considered experimental, though: it's a well-understood change, not a brand new feature that might have bugs. And the preference needs to exist forever, for use by layout tests. So I think it would make more sense among non-experimental features.

> Source/WebCore/loader/MixedContentChecker.h:61
> -    static bool isMixedContent(SecurityOrigin&, const URL&);
> +    static bool isMixedContent(const Settings&, SecurityOrigin&, const URL&);

Nit: consider listing the Settings parameter last, because the SecurityOrigin and URL are the important parameters that the code is trying to test, while the Settings are just ancillary data that is used to perform the test. Same for SecurityOrigin::isAPrioriAuthenticatedURL. (This may be one of my least-important review comments ever. Who complains about parameter order? :)

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:127
> +    // That's not longer the case after bug 218623, we should probably just revised how we treat mixed content.

revised -> review

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:132
> -    g_assert_true(test->m_insecureContentDisplayed);
> +    g_assert_false(test->m_insecureContentDisplayed);

Hm, actually, I bet you can save this test for now by changing WebKitTestServer.cpp to use localhost instead of 127.0.0.1. That should work for the time being, until the default value of the preference added in bug #218627 gets changed to true. Maybe we can find time to deprecate and remove our mixed content handling before then.

[Frédéric Wang (:fredw)]

Created attachment 414329 [details]
Patch

[Frédéric Wang (:fredw)]

Thanks Michael for the review.

(In reply to Michael Catanzaro from comment #15)
> I'm going to give r=me because I don't see any technical problems with the
> patch, and I agree with the technical merits of this change. But since Apple
> has previously expressed concern with this change -- although I hope such
> concerns have been well-addressed by now -- it ought to be approved by an
> Apple reviewer as well before landing.

My understanding is that they are ok with that part. The localhost one seems more controversial until we can guarantee all systems resolve to loopback. Anyway, will double check with them for sure before landing.

> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-mixed-content-to-inscope.https-expected.txt:3
> > -PASS Verify Mixed content of fetch() in a Service Worker
> > +FAIL Verify Mixed content of fetch() in a Service Worker assert_equals: expected "finish" but got "FAIL(4)finish"
> 
> Hm, I see you reported bug #218795 for this. Interesting. Certainly merits
> further investigation.

Right, I'd prefer to handle this separately though has that's probably a separate bug.

> Once upon a time, I successfully proposed a rule that experimental
> preferences must not be enabled by default, because if it's enabled by
> default, it's no longer experimental. I don't remember that rule ever
> changing, but it seems that it's clearly not being followed anymore. So I
> guess this is OK. I don't see why it needs to be considered experimental,
> though: it's a well-understood change, not a brand new feature that might
> have bugs. And the preference needs to exist forever, for use by layout
> tests. So I think it would make more sense among non-experimental features.

That makes sense. I also had similar feeling, but I'm not really clear about the different types of features. I moved it to "internal" one in the last patch.

> > Source/WebCore/loader/MixedContentChecker.h:61
> > -    static bool isMixedContent(SecurityOrigin&, const URL&);
> > +    static bool isMixedContent(const Settings&, SecurityOrigin&, const URL&);
> 
> Nit: consider listing the Settings parameter last, because the
> SecurityOrigin and URL are the important parameters that the code is trying
> to test, while the Settings are just ancillary data that is used to perform
> the test. Same for SecurityOrigin::isAPrioriAuthenticatedURL. (This may be
> one of my least-important review comments ever. Who complains about
> parameter order? :)

Done.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:127
> > +    // That's not longer the case after bug 218623, we should probably just revised how we treat mixed content.
> 
> revised -> review
> 
> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:132
> > -    g_assert_true(test->m_insecureContentDisplayed);
> > +    g_assert_false(test->m_insecureContentDisplayed);
> 
> Hm, actually, I bet you can save this test for now by changing
> WebKitTestServer.cpp to use localhost instead of 127.0.0.1. That should work
> for the time being, until the default value of the preference added in bug
> #218627 gets changed to true. Maybe we can find time to deprecate and remove
> our mixed content handling before then.

OK, s/127.0.0.1/localhost/ ; the patch is not tested yet, I'll rebuild webkit and check later this afternoon on GTK.

[Frédéric Wang (:fredw)]

Quick check:

- api-ios test now fails because Tools/TestWebKitAPI/Tests/WebKitCocoa/TLSDeprecation.mm was assuming experimental feature (which I changed to internal)

- api-gtk fails after the change from 127.0.0.1 to localhost, need to check that.

- not sure what's happening with api-mac, this is something new.

[Frédéric Wang (:fredw)]

Created attachment 414551 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 414571 [details]
Patch

> > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:132
> > > -    g_assert_true(test->m_insecureContentDisplayed);
> > > +    g_assert_false(test->m_insecureContentDisplayed);
> > 
> > Hm, actually, I bet you can save this test for now by changing
> > WebKitTestServer.cpp to use localhost instead of 127.0.0.1. That should work
> > for the time being, until the default value of the preference added in bug
> > #218627 gets changed to true. Maybe we can find time to deprecate and remove
> > our mixed content handling before then.
> 
> OK, s/127.0.0.1/localhost/ ; the patch is not tested yet, I'll rebuild
> webkit and check later this afternoon on GTK.

I was not really successful on this, other tests are assuming things about "localhost" or "127.0.0.1".

I guess a quick hack would be to add a flag to the constructor so that each test can decide to start "localhost" or "127.0.0.1".

However, the right way to fix that in the long term is probably to do something like https://web-platform-tests.org/running-tests/from-local-system.html#hosts-file-setup and listen to such non-loopback / non-localhost address.

WebKitTestServer also needs to be upgraded to more modern API, I see it still uses SOUP_SERVER_INTERFACE for example.

I think I'll stick to disabling the test for now.

[Frédéric Wang (:fredw)]

Created attachment 414838 [details]
Patch (219257+218623) for gtk-api EWS

[Frédéric Wang (:fredw)]

Created attachment 414842 [details]
Patch (applies on top of 219257)

[Frédéric Wang (:fredw)]

Created attachment 414928 [details]
Patch

New version which is a bit stricter and does not use the definition "a priori authenticated url" (removed in https://github.com/w3c/webappsec-mixed-content/issues/35)

[Frédéric Wang (:fredw)]

Created attachment 414929 [details]
Patch (without test & pref change, for EWS)

[Alex Christensen]

Michael, could you point me to where Apple has previously expressed concern with this change?

[Michael Catanzaro]

(In reply to Alex Christensen from comment #25)
> Michael, could you point me to where Apple has previously expressed concern
> with this change?

See Alexey's comments in bug #171934.

[Alex Christensen]

Comment on attachment 414928 [details]
Patch

It seems like their comments probably don't just apply to "localhost" but also to "127.0.0.1" or "::1".  I'm not going to approve this without their approval, which seems unlikely.

[Frédéric Wang (:fredw)]

(In reply to Alex Christensen from comment #27)
> Comment on attachment 414928 [details]
> Patch
> 
> It seems like their comments probably don't just apply to "localhost" but
> also to "127.0.0.1" or "::1".  I'm not going to approve this without their
> approval, which seems unlikely.

See bug 171934 comment 36 though. Maciej said last week he will take a look at the patch so I was waiting for him before doing anything else.

[John Wilander]

From the Slack conversation:

We discussed this extensively in the W3C WebAppSec group back in 2016 or so. Our position was that loopback connections should *only* be allowed in Secure Contexts or if the top frame is loaded from the loopback itself. No other browser was interested in that at the time so we didn't change anything.

Since then, the amount of fingerprinting attacks against the loopback interface has increased. Therefore, we are mostly interested in further *restricting* loopback connections.

I'm personally still in favor of restricting loopback to Secure Contexts which obviously requires not treating it as mixed content (in Secure Contexts).

[Michael Catanzaro]

Hi John, restricting loopback to secure contexts sounds like a good idea to me. I don't expect that to be controversial. At least, the developers requesting this change won't care, because if they're concerned about mixed content, they must already be using HTTPS. But that's a different issue entirely, right?

My understanding of your proposal: the insecure website http://example.com should not be allowed to access http://127.0.0.1 or https://127.0.0.1 since it does not use HTTPS. But the website https://example.com may be allowed to do so, since it uses HTTPS.

This bug: the website https://example.com should be allowed to access http://127.0.0.1 without triggering mixed content warnings, as if it were instead accessing https://127.0.0.1. (http://example.com shows no mixed content warnings regardless, because mixed content checks only apply if the main resource uses HTTPS.)

[John Wilander]

(In reply to Michael Catanzaro from comment #30)
> Hi John, restricting loopback to secure contexts sounds like a good idea to
> me. I don't expect that to be controversial. At least, the developers
> requesting this change won't care, because if they're concerned about mixed
> content, they must already be using HTTPS. But that's a different issue
> entirely, right?
> 
> My understanding of your proposal: the insecure website http://example.com
> should not be allowed to access http://127.0.0.1 or https://127.0.0.1 since
> it does not use HTTPS. But the website https://example.com may be allowed to
> do so, since it uses HTTPS.
> 
> This bug: the website https://example.com should be allowed to access
> http://127.0.0.1 without triggering mixed content warnings, as if it were
> instead accessing https://127.0.0.1. (http://example.com shows no mixed
> content warnings regardless, because mixed content checks only apply if the
> main resource uses HTTPS.)

Right. We're suggesting to reverse the current situation. Right now *only* HTTP pages can make requests to the loopback interface. I'm saying *only* Secure Contexts (HTTPS page and no *other* mixed content) should be able to make requests to the loopback interface.

Just changing it so that loopback requests are not mixed content only relaxes the rules which I don't think is the right direction. Hence, I think if we're going to make a change here, it should be a net benefit which restricting to Secure Contexts would be.

[Michael Catanzaro]

(In reply to John Wilander from comment #31)
> Right. We're suggesting to reverse the current situation. Right now *only*
> HTTP pages can make requests to the loopback interface. I'm saying *only*
> Secure Contexts (HTTPS page and no *other* mixed content) should be able to
> make requests to the loopback interface.

Again, sounds fine to me.

Well, mostly. WebKit doesn't currently have any notion of "HTTPS page and no *other* mixed content." It would be easy to change that, but it doesn't make sense to spend time on it since we should be obsoleting mixed content entirely, https://bugs.webkit.org/show_bug.cgi?id=142469#c3. Once that's done, then we can simply trust that HTTPS means secure.

> Just changing it so that loopback requests are not mixed content only
> relaxes the rules which I don't think is the right direction. Hence, I think
> if we're going to make a change here, it should be a net benefit which
> restricting to Secure Contexts would be.

So I see the point... but at the same time, the current rule is pointless. localhost is obviously not at risk of MITM attacks (unless the device is totally compromised, in which case the browser itself is no longer trusted). That's why the current restriction is annoying. Creating a new restriction for HTTPS pages is an orthogonal issue.

Anyway, I think that's all I have to add to this topic. I guess it's up to Fred to decide whether to take up the quest of blocking localhost access from HTTP pages. (That would need yet another setting, because it will need to be disabled for LayoutTests/http.)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #32)
> Well, mostly. WebKit doesn't currently have any notion of "HTTPS page and no
> *other* mixed content." It would be easy to change that, but it doesn't make
> sense to spend time on it since we should be obsoleting mixed content
> entirely, https://bugs.webkit.org/show_bug.cgi?id=142469#c3. Once that's
> done, then we can simply trust that HTTPS means secure.

Reported bug #219396 for this.

[John Wilander]

(In reply to Michael Catanzaro from comment #32)
> (In reply to John Wilander from comment #31)
> > Right. We're suggesting to reverse the current situation. Right now *only*
> > HTTP pages can make requests to the loopback interface. I'm saying *only*
> > Secure Contexts (HTTPS page and no *other* mixed content) should be able to
> > make requests to the loopback interface.
> 
> Again, sounds fine to me.
> 
> Well, mostly. WebKit doesn't currently have any notion of "HTTPS page and no
> *other* mixed content."

See SecurityContext::isSecureContext(). Document implements that function and is a ScriptExecutionContext which is a SecurityContext, and WorkerGlobalScope implements that function and is a WorkerOrWorkletGlobalScope which is a SecurityContext.

> It would be easy to change that, but it doesn't make
> sense to spend time on it since we should be obsoleting mixed content
> entirely, https://bugs.webkit.org/show_bug.cgi?id=142469#c3. Once that's
> done, then we can simply trust that HTTPS means secure.
> 
> > Just changing it so that loopback requests are not mixed content only
> > relaxes the rules which I don't think is the right direction. Hence, I think
> > if we're going to make a change here, it should be a net benefit which
> > restricting to Secure Contexts would be.
> 
> So I see the point... but at the same time, the current rule is pointless.
> localhost is obviously not at risk of MITM attacks (unless the device is
> totally compromised, in which case the browser itself is no longer trusted).
> That's why the current restriction is annoying. Creating a new restriction
> for HTTPS pages is an orthogonal issue.
> 
> Anyway, I think that's all I have to add to this topic. I guess it's up to
> Fred to decide whether to take up the quest of blocking localhost access
> from HTTP pages. (That would need yet another setting, because it will need
> to be disabled for LayoutTests/http.)

[Michael Catanzaro]

(In reply to John Wilander from comment #34)
> See SecurityContext::isSecureContext(). Document implements that function
> and is a ScriptExecutionContext which is a SecurityContext, and
> WorkerGlobalScope implements that function and is a
> WorkerOrWorkletGlobalScope which is a SecurityContext.

Document::isSecureContext only checks the document's URL, not whether insecure content was loaded. So if you want to treat the document as insecure if it loaded insecure content, it would need a bit more work. (But let's not. Let's do bug #219396 instead.)

[youenn fablet]

I think the concerns Alexey and I shared with regards to public pages getting access to local or lan pages could be addressed by https://wicg.github.io/cors-rfc1918.

[Markus Stange]

What's the next step here?

[John Wilander]

Hi!

(In reply to Markus Stange from comment #37)
> What's the next step here?

Sam Sneddon summarized it well in https://bugs.webkit.org/show_bug.cgi?id=171934#c96.

[Frédéric Wang (:fredw)]

Removing myself from assignee since I'm not working on this anymore.