489415 - Handle multi-principal data in cache

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Consider the following attack:
1) User loads a <video> element from origin A
2) <video> element loads a resource from origin A, which redirects to origin B
3) Data from origin B is loaded into the media cache
4) Seek triggered; we open a new channel to origin A, which this time does not redirect
5) Our principal changes to origin A
6) Another seek triggered to go back to the cached data from origin B, but no new channel is opened because all the data is cached
7) canvas.drawWindow is now able to draw data from origin B into A's canvas without tainting it. FAIL.

The most logical thing to do here, it seems to me, is to pass the principal along with any data we send to the media cache. The media cache could track just one principal per stream; if data is received from more than one principal, we just mark it as "multi-principal" (by setting the principal reference to null?). nsMediaChannelStream::GetPrincipal would then just ask the cache, and consumers of this API would handle null by rejecting any request (I think they already do that).

Boris, how does that sound?

Comment 1

[Boris Zbarsky [:bzbarsky]]

That sounds like if you serve your video via a load-balancer you wouldn't be able to reliably paint it into a canvas, no?

You could reset the principal to a null principal (as opposed to a null pointer).  Then you could paint reliably, and would taint the canvas unreliably.

Another option is to drop the existing cached data if we're adding to the cache with a different principal.  That would possibly make seeks suck in the load-balancer case even if canvas is not being used.

Yet another option is to return as the principal whatever the principal of the data shown in the video right now is.  That sounds complicated, though.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #1)
> That sounds like if you serve your video via a load-balancer you wouldn't be
> able to reliably paint it into a canvas, no?

If you serve via one that uses different domains, yes. We don't bake the IP address into the principal, do we?

> You could reset the principal to a null principal (as opposed to a null
> pointer).  Then you could paint reliably, and would taint the canvas
> unreliably.

A real null principal is probably better, but it's basically the same effect as what I proposed.

> Another option is to drop the existing cached data if we're adding to the cache
> with a different principal.  That would possibly make seeks suck in the
> load-balancer case even if canvas is not being used.

I would hope that most load balancers use the same origin, but yes it would be unfortunate to flush data just for this rare case.

> Yet another option is to return as the principal whatever the principal of the
> data shown in the video right now is.  That sounds complicated, though.

Yes, that's not feasible in general since data for the currently shown frame may come from a range of stream data that spans principals, and figuring out exactly what that range is is codec-dependent. So it would be fragile, complicated and unpredictable.

I'll go with the null principal I guess.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> We don't bake the IP address into the principal, do we?

Right, just the hostname.  So if your load-balancer does redirects, it'll lose.  Your point that most load-balance on the DNS level is a good one, though.

> but it's basically the same effect as what I proposed.

Not quite.  An nsNullPrincipal will let you paint to the canvas but not read data from the canvas after that.  An nsnull nsIPrincipal* here will throw when trying to paint into the canvas, as currently written.

> So it would be fragile, complicated and unpredictable.

"Complicated", like I said.  ;)  Let's not go here.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This took much longer than I expected :-(. Getting the tests to work was especially hard.

I had to make us listen for OnChannelRedirect so we can carry forward Range headers across redirected channels, that's mainly to make the tests work. That's also a good place to update mChannel so we get the right principal to pass to NotifyDataReceived.

The test is a bit tricky. I hope the comments are adequate.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The test depends on a bug fix to httpd.js in bug 482418 (for which I have a patch).

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

I think that we should assert at all call sites of SetNotificationCallbacks() that we're not clobbering an existing callback; it can get one from the load group.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Guys, review here?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We only call SetNotificationCallbacks when we just created the channel, so I don't think we can clobber any existing callbacks there.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In particular, the only case where we don't create the channel ourselves is when it's passed in to us when we load a standalone video document. In that case, nsHTMLMediaElement::LoadWithChannel does not call SetNotificationCallbacks, because we don't need to (the channel is already fully redirected).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 374225 [details] [diff] [review]
fix

This looks pretty good.  One comment: it might make sense to get the principal at the callsite of ReadSegments instead of inside CopySegmentToCache, if we happen to have more than one segment's worth of data available.  Getting the principal from a channel is not super-cheap.  The closure could be a stack struct with the stream and principal pointers.

Also, a nit.  Some places in this patch indent second and later lines of a function decl or NS_IMPL_ISUPPORTSn macro by what looks like 4 spaces, while others indent to line up with the args on the first line.  I'd probably prefer the latter throughout.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I landed this, but backed it out because a bunch of errors showed up on Linux:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242255008.1242259088.10535.gz#err3
The videoDocumentTitle error and the bug428288.html error are known random issues that predate this patch. Not sure about the others. There were two more cycles of this patch on Linux that had no new issues, and none of the Mac/Windows builds had no new issues. So I'm considering just relanding it.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/1721b571672d

Relanded. Windows and Mac are green again, still waiting on Linux. It looks like all the errors in the previous Linux landing were random issues unrelated to this patch.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we please get this landed on 191?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63b01c67c56b

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Decloaking since this will be fixed in all our releases, and anyway, the cross-origin attacks this bug allows are only relevant for non-public Ogg videos and frankly there probably aren't any.