Bug 58378 – Add WebCore::Setting to block displaying and/or running insecure content on secure pages

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Adam Barth]

Add WebCore::Setting to block running insecure content on secure pages

[Adam Barth]

Created attachment 89276 [details]
Work in progress (no tests)

[Chris Evans]

Created attachment 91725 [details]
Patch

Doing this one in pieces -- piece #1.

[Adam Barth]

Comment on attachment 91725 [details]
Patch

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

Tests?  Like many WebCore::Settings, this is a PITA to test.  There's a layoutTestController method for overriding preferences, so one approach is to wire this setting up to a private WebKit setting on mac.

> Source/WebCore/loader/MainResourceLoader.cpp:217
> +    Frame* top = m_frame->tree()->top();
> +    if (top != m_frame) {
> +        if (!frameLoader()->checkIfDisplayInsecureContent(top->document()->securityOrigin(), newRequest.url())) {
> +            cancel();
> +            return;
> +        }
> +    }
> +
>      ResourceLoader::willSendRequest(newRequest, redirectResponse);

The problem is that willSendRequest can change the URL.  Can we move this check back after willSendRequest?  Also, I'm not 100% sure how to cancel the request at this stage.  If you look below, redirectResponse.isNull seems to be the "is canceled" bit.

> Source/WebCore/loader/FrameLoader.h:260
> +    // These two return false if policy dictates that the resource load should
> +    // not continue.

I'd skip this comment.

[Chris Evans]

- Tests. Yep! Definitely going to wire in the new settings and do tests. I was planning on doing this in an immediate follow-up change in the same way that we handled the "file access from files" setting. It's nice to separate the noise of all the platform-specific test setting wiring.

- FrameLoader.h comment: removed as requested.

- Canceling in MainResourceLoader.cpp.
Do we care that willSendRequest() can change the URL? It seems that we shouldn't, as:
a) The main case for that looks to be a redirect, which is a not a situation we'll hit since we're stopping the request going out.
b) The cancel() machinery isn't interested in the URL. It does seem to gracefully handle cancellation at any point, including this one.

The reason I moved the check up to before willSendRequest() is that it seems perverse to call willSendRequest() when in fact we want to be sure that we won't. Without the move, the network inspector _does_ show a canceled HTTP load for the resource. It doesn't appear that the resource fetch ever hits the network, but this is not something we wish to risk. I think it's safest if the callbacks in willSendRequest() never fire.

[Adam Barth]

We probably need to check both before and after willSendRequest in that case.

[Adam Barth]

> - Tests. Yep! Definitely going to wire in the new settings and do tests. I was planning on doing this in an immediate follow-up change in the same way that we handled the "file access from files" setting. It's nice to separate the noise of all the platform-specific test setting wiring.

I think that's ok, but be warned that I'll start secretly removing chairs from your office until you write the tests.  ;)

[Chris Evans]

Created attachment 91751 [details]
Patch

[Chris Evans]

Re: "We probably need to check both before and after willSendRequest in that case."

I believe the current patch is correct. If we take a redirect, we'll re-enter willSendRequest (and again and again etc. as we take more and more redirects). I confirmed this with a bit of debugging.

So we'll always do the check before sending out the request for every redirect in the chain (and block and stop at the first instance of any https -> http transition).

Of course, this is all indicating a great test. I don't think the existing mixed-content-load tests check for the https -> <iframe src="https"> -> 302 http case. I will add coverage for that alongside the upcoming new tests.

[Adam Barth]

Comment on attachment 91751 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Add WebCore::Setting to block displaying and/or running insecure content on secure pages
> +        https://bugs.webkit.org/show_bug.cgi?id=58378

It would be nice to add information to the ChangeLog about how you plan to wire up tests in a followup patch.

> Source/WebCore/loader/MainResourceLoader.cpp:215
> +    Frame* top = m_frame->tree()->top();
> +    if (top != m_frame) {
> +        if (!frameLoader()->checkIfDisplayInsecureContent(top->document()->securityOrigin(), newRequest.url())) {
> +            cancel();
> +            return;
> +        }
> +    }

I thought we decided to do this check both before and after willSendRequest...

[Adam Barth]

Comment on attachment 91751 [details]
Patch

Ok.  That sounds reasonable.  I look forward to the tests.  (The comments above would also be great to add to the ChangeLog.)

[Chris Evans]

Created attachment 91762 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 91762 [details]
Patch

Clearing flags on attachment: 91762

Committed r85378: <http://trac.webkit.org/changeset/85378>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Evans]

Re-opening; the new option per-platform wiring and tests are in-progress.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 91762 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: [e-mail])
security/block-test.html bug 55741 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[Chris Evans]

Created attachment 92901 [details]
Plumbing...

[Chris Evans]

Using EWS to make sure plumbing in the settings hasn't bust the ports... not soliciting review until they're green :)

[Chris Evans]

Created attachment 92909 [details]
Plumbing...

[Collabora GTK+ EWS bot]

Comment on attachment 92909 [details]
Plumbing...

Attachment 92909 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8646859

[Chris Evans]

Created attachment 93024 [details]
Plumbing #1

[Adam Barth]

Comment on attachment 93024 [details]
Plumbing #1

This looks fine, but we need a changelog.

[Chris Evans]

Created attachment 93041 [details]
Plumbing #1

[WebKit Commit Bot]

Comment on attachment 93041 [details]
Plumbing #1

Clearing flags on attachment: 93041

Committed r86201: <http://trac.webkit.org/changeset/86201>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Evans]

More to do.

[WebKit Review Bot]

http://trac.webkit.org/changeset/86201 might have broken SnowLeopard Intel Release (Build)

[Chris Evans]

Created attachment 93167 [details]
Plumbing #2

[Adam Barth]

Comment on attachment 93167 [details]
Plumbing #2

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

> Source/WebKit/qt/Api/qwebsettings.h:82
> -        HyperlinkAuditingEnabled
> +        HyperlinkAuditingEnabled,
> +        PagesCanDisplayInsecureContent,
> +        PagesCanRunInsecureContent

We shouldn't add APIs to Qt without approval from the Qt folks.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3289
> +    settings->setAllowDisplayOfInsecureContent(enableDisplayOfInsecureContent);
> +    settings->setAllowRunningOfInsecureContent(enableRunningOfInsecureContent);

GTK, I'm less sure what they'd like to do.  I'll CC folks.

[Adam Barth]

Thoughts on Qt / GTK settings APIs?
https://bugs.webkit.org/attachment.cgi?id=93167&action=review

[Chris Evans]

I'm basically copying the CL for adding the "allow file access from files" setting. We did help out the Qt / GTK folks in that CL.

I'm fine to not touch various ports, but won't that break the layout tests right out of the door due to missing layoutTestController.setAllowRunningOfInsecureContent() ?

[Adam Barth]

> I'm fine to not touch various ports, but won't that break the layout tests right out of the door due to missing layoutTestController.setAllowRunningOfInsecureContent() ?

Yeah, the two choices are to skip the tests on Qt and GTK or to implement the settings API for them.  In this case, we probably want to implement the API.  I just want to make sure we're making the choice that folks interested in those ports want us to make.

[Martin Robinson]

(In reply to comment #31)
> > I'm fine to not touch various ports, but won't that break the layout tests right out of the door due to missing layoutTestController.setAllowRunningOfInsecureContent() ?
> 
> Yeah, the two choices are to skip the tests on Qt and GTK or to implement the settings API for them.  In this case, we probably want to implement the API.  I just want to make sure we're making the choice that folks interested in those ports want us to make.

If you wish to have GTK+ pass the tests, but delay the decision about whether or not to add new API, you can simply add more methods to Source/WebKit/gtk/DumpRenderTreeSupportGtk.h class.

[Chris Evans]

Given that it's going to take a while longer to iterate through the EWS bots (GTK in particular is v. slow), I'm tending towards waiting for comment from the QT / GTK ports on the full wiring-in of the new option. It basically just models wiring added to all the other ports.

I'd love to get comment quickly enough to land this tonight or tomorrow. I'm very eager to land the tests for the new blocking :)

[Adam Barth]

What about the approach mrobinson suggests in Comment #32?

[Chris Evans]

All it needs is an aye from a GTK dev and then we can roll with the existing patch and no-one has to revisit later.

[Antonio Gomes]

> If you wish to have GTK+ pass the tests, but delay the decision about whether or not to add new API, you can simply add more methods to Source/WebKit/gtk/DumpRenderTreeSupportGtk.h class.

And similarly DumpRenderTreeSupportQt.h/cpp for Qt.

[Chris Evans]

Created attachment 93326 [details]
Plumbing #2

This one should go all green.

[Antonio Gomes]

Comment on attachment 93326 [details]
Plumbing #2

QtWebKit new APIs need discussion in qtwebkit mailing list before anything. Not sure about other ports ...

[Eric Seidel (no email)]

Comment on attachment 91751 [details]
Patch

Cleared Adam Barth's review+ from obsolete attachment 91751 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 93024 [details]
Plumbing #1

Cleared Adam Barth's review+ from obsolete attachment 93024 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Marco Peereboom]

I was in the act of writing a similar patch when I ran across this.  Is there any reason why this isn't moving forward?  I really really want to be able to disable running remote scripts and downloading remote content.  This is one of the preconditions of BEAST and I want to be able to control that in my browser.

[Adam Barth]

> Is there any reason why this isn't moving forward?

I think we just got stalled on getting approval from the GTK and Qt folks to add this API.

[Martin Robinson]

(In reply to comment #42)
> > Is there any reason why this isn't moving forward?
> 
> I think we just got stalled on getting approval from the GTK and Qt folks to add this API.

GTK+ and Qt both have DumpRenderTreeSupport, which is like a hacky version of .internals. This patch can use that and GTK+/Qt API decisions until later.

[Adam Barth]

> GTK+ and Qt both have DumpRenderTreeSupport, which is like a hacky version of .internals. This patch can use that and GTK+/Qt API decisions until later.

Sure, or we could just use window.internals itself.  I think marco was asking about exposing this feature to his application, which presumably uses GTK+ or Qt.

[Marco Peereboom]

> Sure, or we could just use window.internals itself.  I think marco was asking about exposing this feature to his application, which presumably uses GTK+ or Qt.

Right, I use GTK+ and I really want it :-)

[Loganaden Velvindron]

That would be really nice to have. It could
allow some nice security features to be developed :-)

[Marco Peereboom]

ping?

[Marco Peereboom]

Don't want to sound like a nag but is this patch going in?

[Eric Seidel (no email)]

It looks like this patch was long-ago abandoned and needs an updated version.  Anyone (not just Chris) could post such.

[Marco Peereboom]

I am willing to redo this patch if there is interest.  I tried it this morning and it isn't too badly out of whack.

The reason I really believe this should move forward is to be able to stave off things such as the BEAST attack.

So, if I redo the patch to apply to the current trunk does it stand a chance of getting committed?

[Owais Lone]

(In reply to comment #50)
> I am willing to redo this patch if there is interest.  I tried it this morning and it isn't too badly out of whack.
> 
> The reason I really believe this should move forward is to be able to stave off things such as the BEAST attack.
> 
> So, if I redo the patch to apply to the current trunk does it stand a chance of getting committed?


That would be really nice of you. This is blocking a lot of possible features in Gtk/Qt apps.

[Owais Lone]

Ping!

[Josh Rickmar]

Created attachment 150237 [details]
Plumbing #3

Marco asked me to update this patch.  This is my first time doing anything in the WebKit codebase, so forgive me if my patch is wrong (and let me know how to fix it!)  This patch updates the last one and adds the knobs to the GTK API.

[Josh Rickmar]

Created attachment 150416 [details]
Plumbing #3

[Josh Rickmar]

Comment on attachment 150416 [details]
Plumbing #3

Fixed+updated patch

[Build Bot]

Comment on attachment 150416 [details]
Plumbing #3

Attachment 150416 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13133109

[Build Bot]

Comment on attachment 150416 [details]
Plumbing #3

Attachment 150416 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13133113

[Josh Rickmar]

Created attachment 150422 [details]
Plumbing #3

[Gyuyoung Kim]

Comment on attachment 150422 [details]
Plumbing #3

Attachment 150422 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13137083

[Josh Rickmar]

Created attachment 150427 [details]
Plumbing #3

[Raphael Kubo da Costa (:rakuco)]

You probably need to mention which version these new webkit-gtk settings were introduced in. Other than that, it's up to the GTK+ port maintainers to take a look at this, but I don't know if they're actively working (or accepting new features) into their WebKit1 API.

[Marco Peereboom]

This knob is pretty important in order to prevent the BEAST attack.  I spoke with some other folks doing GTK based web browsers and basically everyone wants this.  So I'd like to plead with the GTK folk to push this one in.

[Josh Rickmar]

Created attachment 150612 [details]
Plumbing #3

After some discussion with mrobinson, here's another patch without any of the DumpRenderTree stuff since it didn't introduce any new tests.

[Martin Robinson]

Comment on attachment 150427 [details]
Plumbing #3

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

If you'd like your patch you be reviewed, when posting it you should change the "r" field to say "r?"

I'm not sure why you're adding hooks for this into DumpRenderTree, but we talked about that yesterday. Perhaps it would be better to add unit tests for GTK+ in Source/WebKit/gtk/tests.

The final thing is that maybe this should be only a single setting instead of two. Something like allow-display-and-running-of-insecure-content. I'm not sure about that.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1020
> +    * Boolean property to control whether insecure (non-https) images / frames 
> +    * can be loaded from https pages.

Nit: Should be HTTPS not https. It's probably not necessary to say this a boolean property, you could simply write.

Whether is it possible for pages loaded via HTTPS to display subresources, such as images and frames, from non-HTTPS URLs.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1026
> +                                                         _("Controls load of non-https display resources from https pages"),

How about:

Whether non-HTTPS resources can display on HTTPS pages.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1034
> +    * Boolean property to control whether insecure (non-https) script / CSS / 
> +    * plug-ins can be loaded from https pages.

Same here you could write something like:

Whether is it possible for pages loaded via HTTPS to run subresources, such as CSS, scripts and plugin, from non-HTTPS URLs.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1040
> +                                                         _("Controls load of non-https script / plug-in resources from https pages."),

Whether non-HTTPS resources can run on HTTPS pages.

[Marco Peereboom]

> The final thing is that maybe this should be only a single setting instead of two. Something like allow-display-and-running-of-insecure-content. I'm not sure about that.

I'd prefer two settings in order to be able to fine tune the behavior.

[Martin Robinson]

(In reply to comment #64)

> I'm not sure why you're adding hooks for this into DumpRenderTree, but we talked about that yesterday. Perhaps it would be better to add unit tests for GTK+ in Source/WebKit/gtk/tests.

It seems I was wrong about this, but the tests that exist are platform-specific. Not sure what the right approach is...perhaps moving them to a platform independent directory and skipping them on all other platforms.

platform/chromium/http/tests/security/mixedContent/insecure-image-in-main-frame-allowed.html
11:    testRunner.setAllowDisplayOfInsecureContent(true);

platform/chromium/http/tests/security/mixedContent/insecure-iframe-in-main-frame-allowed.html
11:    testRunner.setAllowDisplayOfInsecureContent(true);

[Josh Rickmar]

Created attachment 150931 [details]
Plumbing #3

No tests on this one, but this includes all the other fixes that have been mentioned.

[Josh Rickmar]

Bump... can we get another look at this patch?

[Gyuyoung Kim]

Comment on attachment 150931 [details]
Plumbing #3

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

> Source/WebKit/gtk/ChangeLog:3
> +        Reviewed by NOBODY (OOPS!).

Generally, "Reviewed by NOBODY" is placed below bug title and url.

> Source/WebKit/gtk/ChangeLog:5
> +        [GTK] Add WebCore::Setting to block displaying and/or running

There is no "[GTK]" in bug title. Generally, bug title has been written in a line.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1029
> +                                                         TRUE,

Though I'm not export for GTK port, I wonder if WK1 GTK port enables insecure-content by default. Does *TRUE* mean this feature is enabled by default ?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1046
> +                                                         TRUE,

ditto.

[Josh Rickmar]

Created attachment 155814 [details]
Plumbing #3

Cooked up a new patch with Gyuyoung's suggestions.

[Josh Rickmar]

> Though I'm not export for GTK port, I wonder if WK1 GTK port enables insecure-content by default. Does *TRUE* mean this feature is enabled by default ?

That is correct.  True is the default value, and keeps the current behavior.

[Marco Peereboom]

Can we please get this in?

[Eric Seidel (no email)]

Comment on attachment 155814 [details]
Plumbing #3

I'm happy to rubber-stamp this Gtk patch.  It looks reasonable.  If Gtk folks disagree, feel free to r-.

[Martin Robinson]

Comment on attachment 155814 [details]
Plumbing #3

Cannot use the commit-queue unfortunately since the "Since:" annotation is incorrect now. This looks good to me, but API changes need review from at least two GTK+ reviewers.

[Gustavo Noronha (kov)]

Comment on attachment 155814 [details]
Plumbing #3

The new API looks OK to me, let's wait for a second review, I'll set r- instead of setting cq+ because of the required changes above.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1020
> +    * Whether is it possible for pages loaded via HTTPS to display
> +    * subresources, such as images and frames, from non-HTTPS URLs.

Can we reword this to "Whether pages loaded via HTTPS should load subresources such as images and frames from non-HTTPS URLs."?

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1022
> +    * Since: 1.9.5

Since tags should say 2.0, since that's going to be the next stable release.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1037
> +    * Whether is it possible for pages loaded via HTTPS to run
> +    * subresources, such as CSS, scripts and plugins, from non-HTTPS
> +    * URLs.

Same rewording here would be good.

[Josh Rickmar]

Created attachment 173635 [details]
Plumbing #3

[WebKit Review Bot]

Attachment 173635 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:995:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:997:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:998:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:999:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1000:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1013:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1014:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1015:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1016:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 10 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #77)
> Attachment 173635 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:995:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:997:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:998:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:999:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1000:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1013:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1014:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1015:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1016:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 10 in 4 files
> 

Do you mind fixing these style errors? The indentation rules changed on us. Now things need to look like this:

function(argument
    argument,
    argument);

or
function(
    argument,
    argument,
    argument);

[Josh Rickmar]

Created attachment 173727 [details]
plumbing #3

fixed indenting

[WebKit Review Bot]

Comment on attachment 173727 [details]
plumbing #3

Clearing flags on attachment: 173727

Committed r134339: <http://trac.webkit.org/changeset/134339>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.