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"
I'll take a quick look at this.
Created attachment 146095 [details] Patch
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?
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.
Created attachment 146105 [details] Patch
Comment on attachment 146105 [details] Patch Just saw Adam's comment, removing r?.
(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 on attachment 146105 [details] Patch (sorry, actually removing r?, not setting r-. :/ )
Comment on attachment 146105 [details] Patch (Double sorry: I see that Adam set r-. I'll stop touching things now.)
(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.
Created attachment 146236 [details] Patch
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 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).
Created attachment 146501 [details] Patch
(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 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.
Created attachment 146618 [details] Patch
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 on attachment 146618 [details] Patch Clearing flags on attachment: 146618 Committed r119883: <http://trac.webkit.org/changeset/119883>
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.
> 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.