Bug 61403 – Video fails to play on Vimeo

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

Video fails to play on Vimeo

[Jer Noble]

<rdar://problem/9488551>

[Jer Noble]

Created attachment 94710 [details]
Patch

[Jer Noble]

Created attachment 94720 [details]
Patch

[WebKit Review Bot]

Comment on attachment 94710 [details]
Patch

Attachment 94710 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8732554

[WebKit Review Bot]

Comment on attachment 94720 [details]
Patch

Attachment 94720 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8733424

[WebKit Commit Bot]

Comment on attachment 94720 [details]
Patch

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

[Maciej Stachowiak]

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.

[Jer Noble]

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

[Jer Noble]

Created attachment 94758 [details]
Patch

Fixed mac build, and made the changes to MediaPlayerPrivateQTKit >SnowLeopard only.

[WebKit Review Bot]

Comment on attachment 94758 [details]
Patch

Attachment 94758 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8731660

[WebKit Commit Bot]

Comment on attachment 94758 [details]
Patch

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

[Jer Noble]

Created attachment 94794 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 94794 [details]
Patch

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

[Jer Noble]

Created attachment 94800 [details]
Patch

One last piece wrapped with >SL conditionals.

[Jer Noble]

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.

[Jer Noble]

Created attachment 94822 [details]
Patch

[WebKit Review Bot]

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.

[Darin Adler]

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.

[Jer Noble]

Created attachment 94853 [details]
Patch

[Darin Adler]

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.

[Adam Barth]

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.

[Jer Noble]

Committed r87328: <http://trac.webkit.org/changeset/87328>

[Darin Adler]

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.

[Adam Barth]

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?

[Darin Adler]

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

[Ademar Reis]

Revision r87328 cherry-picked into qtwebkit-2.2 with commit b7169cc <http://gitorious.org/webkit/qtwebkit/commit/b7169cc>