Bug 61403

Summary: Video fails to play on Vimeo
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ademar, darin, dglazkov, sam, webkit.review.bot
Priority: P1 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Attachments:
Description Flags
Patch
webkit.review.bot: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Jer Noble 2011-05-24 16:22:18 PDT
Video fails to play on Vimeo
Comment 1 Jer Noble 2011-05-24 16:23:06 PDT
<rdar://problem/9488551>
Comment 2 Jer Noble 2011-05-24 16:31:23 PDT
Created attachment 94710 [details]
Patch
Comment 3 Jer Noble 2011-05-24 17:21:35 PDT
Created attachment 94720 [details]
Patch
Comment 4 WebKit Review Bot 2011-05-24 17:47:23 PDT
Comment on attachment 94710 [details]
Patch

Attachment 94710 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8732554
Comment 5 WebKit Review Bot 2011-05-24 20:08:33 PDT
Comment on attachment 94720 [details]
Patch

Attachment 94720 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8733424
Comment 6 WebKit Commit Bot 2011-05-24 20:43:43 PDT
Comment on attachment 94720 [details]
Patch

Attachment 94720 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8726937
Comment 7 Maciej Stachowiak 2011-05-24 21:23:39 PDT
Comment on attachment 94720 [details]
Patch

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

r- because this appears to break the build. I am not entirely sure why, and otherwise it seems ok though see other comment.

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:259
> -                        [NSNumber numberWithInt:AVAssetReferenceRestrictionForbidCrossSiteReference], AVURLAssetReferenceRestrictionsKey, 
> +                             [NSNumber numberWithInt:AVAssetReferenceRestrictionForbidRemoteReferenceToLocal | AVAssetReferenceRestrictionForbidLocalReferenceToRemote], AVURLAssetReferenceRestrictionsKey, 

Is there any way we can allow cross-origin redirects (which seem safe enough) while still preventing cross-origin auxiliary resource references (e.g. reference movies)? I think that should be the goal, but if that's not achievable, this approach seems ok.
Comment 8 Jer Noble 2011-05-25 01:52:22 PDT
(In reply to comment #7)
> (From update of attachment 94720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94720&action=review
> 
> r- because this appears to break the build. I am not entirely sure why, and otherwise it seems ok though see other comment.

Looks like it's because of a missing newline from WebCoreSystemInterface.mm.  I'll get that fixed.

> Is there any way we can allow cross-origin redirects (which seem safe enough) while still preventing cross-origin auxiliary resource references (e.g. reference movies)? I think that should be the goal, but if that's not achievable, this approach seems ok.

Unfortunately, QTKit doesn't make any distinction between reference movies and redirects.  We could, in theory, take over loading somewhat by checking for HTTP redirects before passing the request on to QTKit, but this would be a major disruptive change.
Comment 9 Jer Noble 2011-05-25 02:22:48 PDT
Created attachment 94758 [details]
Patch

Fixed mac build, and made the changes to MediaPlayerPrivateQTKit >SnowLeopard only.
Comment 10 WebKit Review Bot 2011-05-25 02:40:23 PDT
Comment on attachment 94758 [details]
Patch

Attachment 94758 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8731660
Comment 11 WebKit Commit Bot 2011-05-25 02:41:25 PDT
Comment on attachment 94758 [details]
Patch

Attachment 94758 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8734385
Comment 12 Jer Noble 2011-05-25 09:40:51 PDT
Created attachment 94794 [details]
Patch
Comment 13 WebKit Commit Bot 2011-05-25 09:51:00 PDT
Comment on attachment 94794 [details]
Patch

Attachment 94794 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8735106
Comment 14 Jer Noble 2011-05-25 10:08:54 PDT
Created attachment 94800 [details]
Patch

One last piece wrapped with >SL conditionals.
Comment 15 Jer Noble 2011-05-25 11:06:38 PDT
Comment on attachment 94800 [details]
Patch

After talking with Eric Carlson, we've decided that the QTKit fix should be platform independent.  So I'm taking down this patch for review until I whip up a new one.
Comment 16 Jer Noble 2011-05-25 11:54:30 PDT
Created attachment 94822 [details]
Patch
Comment 17 WebKit Review Bot 2011-05-25 11:57:22 PDT
Attachment 94822 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

WebKitLibraries/WebKitSystemInterface.h:210:  The parameter name "movie" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Darin Adler 2011-05-25 13:24:24 PDT
Comment on attachment 94822 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:785
> +    return requestedURL.host() == resolvedURL.host() && requestedURL.port() == resolvedURL.port();

Normally, security origin checks need to cover protocol as well as host and port.

It’s probably safest to do security origin checks by constructing SecurityOrigin objects and using the member functions of the SecurityOrigin class rather than writing custom logic here. But if we have to have custom logic, it should be sure to check protocol.

> Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1621
> +    return requestedURL.host() == resolvedURL.host() && requestedURL.port() == resolvedURL.port();

Same comment.
Comment 19 Jer Noble 2011-05-25 14:05:21 PDT
Created attachment 94853 [details]
Patch
Comment 20 Darin Adler 2011-05-25 14:08:04 PDT
Comment on attachment 94853 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:786
> +    return resolvedOrigin->isSameSchemeHostPort(requestedOrigin.get());

This check is probably too strict.

The correct check would probably be to check if the original security origin SecurityOrigin::canRequest the resolved asset URL. The original assertURL doesn’t matter at all.

We can live with this for now.
Comment 21 Adam Barth 2011-05-25 14:41:45 PDT
Comment on attachment 94853 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObjC.mm:786
>> +    return resolvedOrigin->isSameSchemeHostPort(requestedOrigin.get());
> 
> This check is probably too strict.
> 
> The correct check would probably be to check if the original security origin SecurityOrigin::canRequest the resolved asset URL. The original assertURL doesn’t matter at all.
> 
> We can live with this for now.

Yep.
Comment 22 Jer Noble 2011-05-25 15:53:37 PDT
Committed r87328: <http://trac.webkit.org/changeset/87328>
Comment 23 Darin Adler 2011-05-26 10:36:23 PDT
When we want to improve the security check further, there are three considerations I can think of:

    1) We will have to double check whether canAccess, canRequest, or canDisplay is the correct check to make for media. My initial thought is that it should be canDisplay, despite the fact that my earlier comment mentioned canRequest. Someone needs to think it through and presumably check what we already do in the other media code paths.

    2) The canDisplay check establishes that it’s OK for the page to display the media, but I’m not sure if it sufficiently checks that the media can be trusted to affect the page. Sorry this is worded so vaguely; I’m not sure what language to use to talk about this type of issue or how we already deal with it.

    3) There are resources other than the mail asset URL and we would like WebKit to perform the security checks for all resources. Even though QuickTime has an ability to do its own security checks, we would instead like to plumb the checks back through to WebKit so it can establish the policy.
Comment 24 Adam Barth 2011-05-26 10:46:47 PDT
I'm not sure I fully understand the issues here.

>     1) We will have to double check whether canAccess, canRequest, or canDisplay is the correct check to make for media. My initial thought is that it should be canDisplay, despite the fact that my earlier comment mentioned canRequest. Someone needs to think it through and presumably check what we already do in the other media code paths.

Certainly we should check whether the web page can display the media (which involves checking each hop in the redirect chain).

>     2) The canDisplay check establishes that it’s OK for the page to display the media, but I’m not sure if it sufficiently checks that the media can be trusted to affect the page. Sorry this is worded so vaguely; I’m not sure what language to use to talk about this type of issue or how we already deal with it.

How can the media affect the page?  My understanding is the media cannot script the embedding page, so the affect that it can have on the embedding page is minimal.

If the media tries to script the page (i.e., if that's possible), we need to check whether the media can access the page, like we do for iframes.

>     3) There are resources other than the mail asset URL and we would like WebKit to perform the security checks for all resources. Even though QuickTime has an ability to do its own security checks, we would instead like to plumb the checks back through to WebKit so it can establish the policy.

If the media's security context cannot be described by a single URL, it's going to be complicated to do the access checks correctly.  For example, images have a notion of whether they have a single security origin.  In cases where they have more than one security origin, we just cause all access checks to fail (e.g., tainting canvases).

I guess the main thing I don't understand is how media can affect the embedding page.  I was under the impression that embedding a video was much like embedding an image.  Is there more that media can do to the enclosing page?
Comment 25 Darin Adler 2011-05-26 16:11:49 PDT
(In reply to comment #24)
> Certainly we should check whether the web page can display the media (which involves checking each hop in the redirect chain).

Makes sense. We should call canDisplay on each.

> >     2) The canDisplay check establishes that it’s OK for the page to display the media, but I’m not sure if it sufficiently checks that the media can be trusted to affect the page. Sorry this is worded so vaguely; I’m not sure what language to use to talk about this type of issue or how we already deal with it.
> 
> How can the media affect the page?  My understanding is the media cannot script the embedding page, so the affect that it can have on the embedding page is minimal.
> 
> If the media tries to script the page (i.e., if that's possible), we need to check whether the media can access the page, like we do for frames.

OK, makes total sense. I think the media can’t. I’m thinking back to QuickTime and the QuickTime plug-in, which at one point could host more-active media with scripts in it. But I don’t think this is true for the media element.

> >     3) There are resources other than the mail asset URL and we would like WebKit to perform the security checks for all resources. Even though QuickTime has an ability to do its own security checks, we would instead like to plumb the checks back through to WebKit so it can establish the policy.
> 
> If the media's security context cannot be described by a single URL, it's going to be complicated to do the access checks correctly. For example, images have a notion of whether they have a single security origin. In cases where they have more than one security origin, we just cause all access checks to fail (e.g., tainting canvases).

I see. This might apply to some media, but again I may be thinking more of the more-complex QuickTime past here.

> I guess the main thing I don't understand is how media can affect the embedding page. I was under the impression that embedding a video was much like embedding an image. Is there more that media can do to the enclosing page?

I think that was my mistake, and the answer is no.
Comment 26 Ademar Reis 2011-06-03 14:02:31 PDT
Revision r87328 cherry-picked into qtwebkit-2.2 with commit b7169cc <http://gitorious.org/webkit/qtwebkit/commit/b7169cc>