Bug 84054 - webkitURL.createObjectURL uris generated from an SSL site are marked insecure
Summary: webkitURL.createObjectURL uris generated from an SSL site are marked insecure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-16 11:01 PDT by Charles Pritchard
Modified: 2012-06-10 09:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.49 KB, patch)
2012-06-06 13:17 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (7.22 KB, patch)
2012-06-06 14:06 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (7.48 KB, patch)
2012-06-07 02:09 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2012-06-08 01:06 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (9.33 KB, patch)
2012-06-08 11:32 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charles Pritchard 2012-04-16 11:01:58 PDT
Cross-posted from Chromium: http://code.google.com/p/chromium/issues/detail?id=112166

Using <img src="webkitURL.createObjectURL(file)" /> will result in a warning that insecure resources are being loaded, though the blob uri is secure, looking like: "blob:https://example.com/uuid"
Comment 1 Mike West 2012-06-06 12:24:59 PDT
I'll take a quick look at this.
Comment 2 Mike West 2012-06-06 13:17:22 PDT
Created attachment 146095 [details]
Patch
Comment 3 Mike West 2012-06-06 13:19:34 PDT
This change adds schemes that SchemeRegistry::canDisplayOnlyIfCanRequest matches to the schemes that FrameLoader::isMixedContent treats as secure. What could go wrong?

Jochen, can you help me think through the security implications this change might have?
Comment 4 jochen 2012-06-06 13:36:29 PDT
Comment on attachment 146095 [details]
Patch

Generally looks good

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

> Source/WebCore/ChangeLog:14
> +        Test: http/tests/security/mixedContent/blob-url-in-iframe.html

can you add a test for the filesystem: case as well?
Comment 5 Adam Barth 2012-06-06 13:53:22 PDT
Comment on attachment 146095 [details]
Patch

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

I'm sorry I don't have time at the moment to sort out the correct patch for this problem.

> Source/WebCore/loader/FrameLoader.cpp:872
> -    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()))
> +    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()) || SchemeRegistry::canDisplayOnlyIfCanRequest(url.protocol()))

This is not correct.  The fact that you can only display the scheme if you can request it has no bearing on whether it's mixed content.  In particular, imagine a document with universal access.  It can request every URL, even insecure ones.
Comment 6 Mike West 2012-06-06 14:06:21 PDT
Created attachment 146105 [details]
Patch
Comment 7 Mike West 2012-06-06 14:07:37 PDT
Comment on attachment 146105 [details]
Patch

Just saw Adam's comment, removing r?.
Comment 8 Mike West 2012-06-06 14:48:01 PDT
(In reply to comment #5)
> I'm sorry I don't have time at the moment to sort out the correct patch for this problem.

Who would you suggest that I ask?

> > Source/WebCore/loader/FrameLoader.cpp:872
> > -    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()))
> > +    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()) || SchemeRegistry::canDisplayOnlyIfCanRequest(url.protocol()))
> 
> This is not correct.  The fact that you can only display the scheme if you can request it has no bearing on whether it's mixed content.  In particular, imagine a document with universal access.  It can request every URL, even insecure ones.

Is your objection to the patch the general idea of considering blob: and filesystem: schemes secure for the purposes of resolving isMixedContent?

We treat data: as a secure scheme at the moment, as it isn't exposed to an active network attacker. It seems that the same holds true for blob: and filesystem: resources. I'd hesitate to add them directly as secure schemes, but they certainly seem to fall into the same category as data:.

Perhaps this is a naming issue? My understanding is that the thrust of the method is to enumerate those schemes that can only be requested by specific origins. At the moment, the only schemes that return true for this method are `blob:` and `filesystem:`. If there was another method called something in the direction of "SchemeRegistry::isOriginBoundScheme()", would you have the same objection?
Comment 9 Mike West 2012-06-06 14:48:51 PDT
Comment on attachment 146105 [details]
Patch

(sorry, actually removing r?, not setting r-. :/ )
Comment 10 Mike West 2012-06-06 14:51:13 PDT
Comment on attachment 146105 [details]
Patch

(Double sorry: I see that Adam set r-. I'll stop touching things now.)
Comment 11 Adam Barth 2012-06-06 14:57:07 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > I'm sorry I don't have time at the moment to sort out the correct patch for this problem.
> 
> Who would you suggest that I ask?
> 
> > > Source/WebCore/loader/FrameLoader.cpp:872
> > > -    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()))
> > > +    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()) || SchemeRegistry::canDisplayOnlyIfCanRequest(url.protocol()))
> > 
> > This is not correct.  The fact that you can only display the scheme if you can request it has no bearing on whether it's mixed content.  In particular, imagine a document with universal access.  It can request every URL, even insecure ones.
> 
> Is your objection to the patch the general idea of considering blob: and filesystem: schemes secure for the purposes of resolving isMixedContent?

Knowing that the scheme is blob or filesystem is insufficient for determining whether it's mixed content.

> We treat data: as a secure scheme at the moment, as it isn't exposed to an active network attacker.

Knowing that the scheme is data is sufficient for determining whether it's mixed content.

> It seems that the same holds true for blob: and filesystem: resources.

Except that it's not true in the case of a document with universal access.
Comment 12 Mike West 2012-06-07 02:09:45 PDT
Created attachment 146236 [details]
Patch
Comment 13 Mike West 2012-06-07 02:14:44 PDT
Does this look better, Adam/Jochen? SecurityOrigin understands the concept of a scheme whose "inner URL" ought to be used for evaluation of its secure status. Routing the KURL through SecurityOrigin in addition to checking its status directly seems to behave correctly, though I'm not sure of its performance impact.

If that's a concern, an option would be to move `shouldUseInnerURL` to SecurityOrigin's public interface in order to check whether it's necessary to create a SecurityOrigin object.

WDYT?
Comment 14 Adam Barth 2012-06-07 15:44:55 PDT
Comment on attachment 146236 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:876
> +    RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url);
> +    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()) || SchemeRegistry::shouldTreatURLSchemeAsSecure(targetOrigin->protocol()))

I like this approach, but it might be kind of slow.  I wonder if we should add a static method SecurityOrigin::isSecure(url) and have it do the innerURL thing directly (avoiding a bunch of mallocs).
Comment 15 Mike West 2012-06-08 01:06:55 PDT
Created attachment 146501 [details]
Patch
Comment 16 Mike West 2012-06-08 01:08:46 PDT
(In reply to comment #14)
> (From update of attachment 146236 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146236&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:876
> > +    RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url);
> > +    if (!url.isValid() || SchemeRegistry::shouldTreatURLSchemeAsSecure(url.protocol()) || SchemeRegistry::shouldTreatURLSchemeAsSecure(targetOrigin->protocol()))
> 
> I like this approach, but it might be kind of slow.  I wonder if we should add a static method SecurityOrigin::isSecure(url) and have it do the innerURL thing directly (avoiding a bunch of mallocs).

This makes a lot of sense. It's cleaner than exposing the innerURL logic. Patch attached.
Comment 17 Adam Barth 2012-06-08 11:13:09 PDT
Comment on attachment 146501 [details]
Patch

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

This looks great.  Thanks for iterating on the patch.

> Source/WebCore/page/SecurityOrigin.cpp:212
> +// static

nit: We usually skip these comments in WebKit.  There might be some in this file already because I used to add them before I knew the right style.
Comment 18 Mike West 2012-06-08 11:32:26 PDT
Created attachment 146618 [details]
Patch
Comment 19 Mike West 2012-06-08 11:34:06 PDT
Comment on attachment 146618 [details]
Patch

(In reply to comment #17)
> (From update of attachment 146501 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146501&action=review
> 
> This looks great.  Thanks for iterating on the patch.

Thanks for helping me make it better.

> > Source/WebCore/page/SecurityOrigin.cpp:212
> > +// static
> 
> nit: We usually skip these comments in WebKit.  There might be some in this file already because I used to add them before I knew the right style.

New patch attached. I checked the rest of the file, it's clean. I think I just added the comment out of habit, not because I saw you doing it elsewhere. :)
Comment 20 WebKit Review Bot 2012-06-08 18:51:42 PDT
Comment on attachment 146618 [details]
Patch

Clearing flags on attachment: 146618

Committed r119883: <http://trac.webkit.org/changeset/119883>
Comment 21 WebKit Review Bot 2012-06-08 18:51:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 mitz 2012-06-10 09:06:38 PDT
(In reply to comment #20)
> (From update of attachment 146618 [details])
> Clearing flags on attachment: 146618
> 
> Committed r119883: <http://trac.webkit.org/changeset/119883>

The tests added in this change have been failing ever since. See bug 88736.
Comment 23 Adam Barth 2012-06-10 09:10:28 PDT
> The tests added in this change have been failing ever since. See bug 88736.

I replied on that bug.  Looks like the tests are failing because they're testing features that aren't enabled for that port.  They should probably be skipped until you enable the tested features.
Comment 24 mitz 2012-06-10 09:17:45 PDT
(In reply to comment #23)
> > The tests added in this change have been failing ever since. See bug 88736.
> 
> I replied on that bug.  Looks like the tests are failing because they're testing features that aren't enabled for that port.  They should probably be skipped until you enable the tested features.

It seems unlikely that the LEGACY_WEBKIT_BLOB_BUILDER feature will ever be enabled in ports that don’t already have it enabled.