Bug 84054 – webkitURL.createObjectURL uris generated from an SSL site are marked insecure

• Component: DOM
• Status: RESOLVED FIXED

[Charles Pritchard]

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"

[Mike West]

I'll take a quick look at this.

[Mike West]

Created attachment 146095 [details]
Patch

[Mike West]

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 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?

[Adam Barth]

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.

[Mike West]

Created attachment 146105 [details]
Patch

[Mike West]

Comment on attachment 146105 [details]
Patch

Just saw Adam's comment, removing r?.

[Mike West]

(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?

[Mike West]

Comment on attachment 146105 [details]
Patch

(sorry, actually removing r?, not setting r-. :/ )

[Mike West]

Comment on attachment 146105 [details]
Patch

(Double sorry: I see that Adam set r-. I'll stop touching things now.)

[Adam Barth]

(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.

[Mike West]

Created attachment 146236 [details]
Patch

[Mike West]

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?

[Adam Barth]

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).

[Mike West]

Created attachment 146501 [details]
Patch

[Mike West]

(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.

[Adam Barth]

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.

[Mike West]

Created attachment 146618 [details]
Patch

[Mike West]

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. :)

[WebKit Review Bot]

Comment on attachment 146618 [details]
Patch

Clearing flags on attachment: 146618

Committed r119883: <http://trac.webkit.org/changeset/119883>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(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.

[Adam Barth]

> 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.

(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.