Closed Bug 451958 Opened 16 years ago Closed 16 years ago

Implement same-origin check plus Access Controls for video

Categories

(Core :: Audio/Video, defect, P1)

Product:
Core
Component:
Audio/Video
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: guninski, Assigned: cpearce)

References

Details

(Keywords: verified1.9.1, Whiteboard: [sg:moderate?])

Attachments

(10 files, 11 obsolete files)

more reliable testcase - video3b.html
1.41 KB, text/html
Details
Patch v9
61.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
Patch v10 - 1 of 2 - Allow-Origin check
58.83 KB, patch
Details | Diff | Splinter Review
Patch v10 - 2 of 2 - Load-Policy check
3.71 KB, patch
Details | Diff | Splinter Review
v10 part 2a
1.04 KB, patch
Details | Diff | Splinter Review
v10 part 2b
673 bytes, patch
Details | Diff | Splinter Review
v10 part 2c
894 bytes, patch
Details | Diff | Splinter Review
v10 part 2d
892 bytes, patch
Details | Diff | Splinter Review
v10 part 2e
881 bytes, patch
Details | Diff | Splinter Review
Disable Ogg seek tests
1.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached file testcase - video3.html — 
<video src="file:///stuff"> tries to load. via events it is possible to tell if the file exists locally.

testcase may crash because of bug Bug 451943
frames of homemade pron can be borrowed via Bug 451938


don't know if the testcase is somewhat broken or hits some |alert| dependent bugs
Component: General → Video/Audio
Product: Firefox → Core
are there serious bugs in the testcase ?

if i remove all of the |alert|s, the generated content of +/- in the span is lost and i see no reason for removing the dynamic content
ooops, accidentally i posted a testcase for this in the wrong *open* bug
Bug 451943
https://bugzilla.mozilla.org/show_bug.cgi?id=451943

someone with power may consider scrubbing the testcase from bugzilla.
mitigating is it is trunk only.

sorry.

more reliable testcase
Marked that comment and my own as private.
10x. btw, didn't receive any bugmail from the recent comments this morning, including this bug.
please ignore comment #5 except for "10x"
I guess we need to call CheckLoadURI.
i guess CheckLoadURI should be put inside Load to make misuse harder
this works for other protocol:

|javascript| asserts badly, doesn't seem loaded

|chrome| seems to load, ugly assertions
Whiteboard: [sg:moderate?]
playing local files works in mail preview and editor. this doesn't seem sane. is this the same bug or worth a new one?
Assignee: nobody → chris
What we want to do here is implement a full same-origin check plus Access Controls.
We must do something similar to bug 457825 here. We must only allow videos to be read from the same domain, unless the owning webserver provides access control headers to allow cross-site use.

See bug 457825 and http://www.w3.org/TR/access-control/#access-control-allow-origin
Summary: checking for local file existence via <video> element and events → Implement same-origin check plus Access Controls for video
If there a link to part of <video> spec that says cross-site usage is disallowed?
Discussion starting by <video> users about the change:

http://lists.xiph.org/pipermail/theora/2008-November/001930.html
Amazon S3 is a service used by some sites to serve video. It's not possible to have custom headers added to downloads using S3 as far as I know. Adding same-origin access controls prevents these sites from migrating to <video> until such time as Amazon S3 implements the functionality.

Wikimedia <video> usage serves their video from a subdomain. Wikimedia does not currently support sending the access control headers. Until they do <video> usage on their site will break. 

Other early adopter <video> sites have similar issues.

Could we either hold off implementing this change until these users of video have been informed and can make changes, or perhaps at least make this bug public so that they know it's happening.
By "this bug" being made public I mean implementing the access control restriction, not the problem with local file access.
The problem is, no-one's going to add the access-control header until they have to. And if we don't enforce this before we ship 3.1, no one will never be able to.
(Whereas if we ship the same-origin restriction in 3.1, but we decide that was a bad idea, we do still have the freedom to relax it in the future.)
I realized that I've been pushing too hard on this. Chris Double is the module owner and doesn't want to do this, and it's not my place to override that. I think it's probably best to decide this in the security review. Unfortunately that's not yet scheduled:
https://wiki.mozilla.org/Firefox3.1/Security
So we need to schedule that ASAP.

I'm not sure what we should do for beta2. Maybe it's already too late to do anything.
why not treat <video> just like <img>?

<img> had very similar problems with <canvas>
I don't think the problems with cross-site image loading are something we want to emulate.

Also, the <video> element has a much richer API than the <img> element, and it will get richer over time. So there's more potential for cross-site information leakage.
Work In Progress patch #1.

Implements access control check for <media>. As requested, there's a pref browser.media.checkAllowOrigin (on by default), that user can set to false to disable the check.

The original security bug that this was filed for still exists; the testcase still works with this patch. The access check is blocking cross domain loads, but not loads to local files. I'll complete this tomorrow.
Drive-by comments:

+    CASE_RETURN( TYPE_VIDEO             );

Maybe TYPE_MEDIA? I assume audio is restricted too.

+   /**
+    * Indicates a video.
+    */
+    const unsigned long TYPE_VIDEO = 15;  

Ditto.

What happens with media elements that are removed from the document and have their src set? Might want to test that case.
(In reply to comment #24)
> What happens with media elements that are removed from the document and have
> their src set? Might want to test that case.

The media element's loads progress as if the media element was still in its original parent document, and the loads are still subject to the same cross-domain restrictions as if the element was still physically in its original parent document.

I assume that's OK?
Attached patch Patch v2 (obsolete) — — Splinter Review 
WIP completed, can no longer load files from localhost from webpage on another domain.
Attachment #347132 - Attachment is obsolete: true
Attachment #347219 - Flags: review?(roc)
Attachment #347219 - Attachment is patch: true
Attachment #347219 - Attachment mime type: application/octet-stream → text/plain
+  PRBool checkOrigin = PR_FALSE;

default to PR_TRUE in case something goes wrong with the pref system.

I'd call the pref "browser.media.enforce_same_site_origin" to be more consistent with the font pref.

+    nsCOMPtr<nsIDocument> doc = GetOwnerDoc();
+    NS_ENSURE_TRUE(doc, NS_ERROR_FAILURE);
+    nsCOMPtr<nsIPrincipal> elementPrincipal = doc->NodePrincipal();

Why don't you just use the nsHTMLMediaElement's NodePrincipal? I guess there are four places where you're doing this.

I think that the call to NS_CheckContentLoadPolicy should happen unconditionally. ShouldCheckAllowOrigin should only control the access-controls/same-origin stuff.

Why can't we just do the nsCrossSiteListenerProxy dance in nsMediaStream::Open? It seems to me that for an already-open channel we should be OK, and it will do the CheckLoadURIWithPrincipal check that you currently have for files.

What kind of error DOM event gets returned here? It looks like that error is not distinguishable from other kinds of network errors. We should fix that.
(In reply to comment #28)
> Why can't we just do the nsCrossSiteListenerProxy dance in nsMediaStream::Open?
> It seems to me that for an already-open channel we should be OK, and it will do
> the CheckLoadURIWithPrincipal check that you currently have for files.

Oh, and I think nsByteRangeEvent should re-Open the nsMediaStream instead of resetting the nsHttpStreamStrategy. That would reduce code duplication. You'll still want to create the channel and set the header, then pass the channel into nsMediaStream::Open.
(In reply to comment #28)
> Why can't we just do the nsCrossSiteListenerProxy dance in nsMediaStream::Open?

Because we need to call AsyncOpen() on the channel, and pass in the nsCrossSiteListenerProxy, and we don't want to do that for nsFileStreamStrategy.

> What kind of error DOM event gets returned here? It looks like that error is
> not distinguishable from other kinds of network errors. We should fix that.

ATM it's MEDIA_ERR_NETWORK. We'd want need a spec change to make it distinguishable.
(In reply to comment #30)
> (In reply to comment #28)
> > Why can't we just do the nsCrossSiteListenerProxy dance in nsMediaStream::Open?
> 
> Because we need to call AsyncOpen() on the channel, and pass in the
> nsCrossSiteListenerProxy, and we don't want to do that for
> nsFileStreamStrategy.

OK then, how about having nsByteRangeEvent call nsHttpStreamStrategy::Open? You would need to add a parameter or otherwise reorganize the code so that mPosition doesn't get set to 0 for that call.

> > What kind of error DOM event gets returned here? It looks like that error is
> > not distinguishable from other kinds of network errors. We should fix that.
> 
> ATM it's MEDIA_ERR_NETWORK. We'd want need a spec change to make it
> distinguishable.

Hmm. I think we should add, say, MEDIA_ERR_NETWORK_SECURITY with some large random value for now and ask Hixie (possibly in that W3C bug) to add it to the spec with a real value.
Flags: blocking1.9.1? → blocking1.9.1+
Attached patch Patch v3 WIP (Leaky) (obsolete) — — Splinter Review 
Patch v3, with review comments addressed. It's leaking, so I'll look at the leaks after dinner and post another (hopefully final) patch.
Attachment #347219 - Attachment is obsolete: true
Attachment #347219 - Flags: review?(roc)
does the patch work correctly with HTTP redirects?
i.e. => same site => sensitive site
I believe so. nsCrossSiteListenerProxy handles that for us by observing redirects.
+  nsCOMPtr<nsIPrincipal> elementPrincipal = NodePrincipal();

This can just be nsIPrincipal* (3 times).

   } else {
+
+    // Ensure that we never load a local file from some page on a 

Lose the blank line.

+  // as those with the same name in nsMediaStream. Defers to
+  // Open(nsIStreamListener**, PR_TRUE). 
   virtual nsresult Open(nsIStreamListener** aListener);
+
+  // Reopens the stream strategy for |aChannel|. Defers to
+  // Open(nsIStreamListener**, PR_FALSE). This must be called on the main thread
+  // only, and at a time when the strategy is not reading from the
+  // current channel/stream. It's primary purpose is to be called from
+  // a Seek to reset to the new byte range request http channel.
+  nsresult Open(nsIChannel* aChannel);
+
+  // Sets up listeners and streams for reading from channel. If aResetPosition
+  // is true, it resets mPosition to 0.
+  nsresult Open(nsIStreamListener **aStreamListener, PRBool aResetPosition);

Might as well lose the aResetPosition flag and just set mPosition to 0 in Open(nsIStreamListener**). Rename this to OpenInternal.
Worth adding at least one test for cross-site redirection, then, no?  Never assume, they say...
I notice that nsHttpStreamStrategy::Reset used to take a lock but the equivalent code path no longer does. That should be fixed.
> Never assume, they say...

never trust developers about lack of bugs ;)
Nit: I think it might actually be worth adding separate nsIContentPolicy types for audio and video. Though I guess the content policy could just look at the localName of the element.
Attached patch Patch v5 (obsolete) — — Splinter Review 
* Adds redirect tests to mochitest.
* Changed mochitest to listen for onloadedfirstframe rather than onmetadataloaded, as it was more reliable, seems onmetadataloaded isn't always delivered?
* Fixes leaks, was caused by nsChannelReader not being freed when nsOggDecodeStateMachine was shutdown before it'd had a chance to init mPlayer field, which was responsible for freeing nsChannelReader.
* Also prevents us creating and load()ing a new nsOggDecoder when src="" (when media element bound to a doc).

The two issues were picked up last night in Bug 463999, but the mochitests will cause leaks if these fixes aren't in when it lands.
Attachment #347235 - Attachment is obsolete: true
Attachment #347415 - Flags: review?(roc)
So, when are we aiming to land this, if we are indeed going to land it?

Do we want to extract the content policies stuff out, e.g. to bug 451004?
I think the best thing to do right now would be to land it as-is with the pref set to false. That doesn't change behaviour, lets people test what it would be like with the pref on, and lets us flip the pref on with low risk if/when we feel like it.

Oh, but we should do another rev of the patch so that even if enforce_same_site_origin is false, we still do a CheckLoadURIWithPrincipal instead of doing an nsCrossSiteListenerProxy in the HTTP case. I'm not sure exactly what that checks since we know the URL is already HTTP/HTTPS, but it seems worth doing.

One thing which came up in the security review is that we don't want to tell the enclosing page the difference between "there was no resource there" and "there was a resource but we denied access" --- that lets people probe for the existence of arbitrary files. This is a problem since people want to be able to detect cross-origin situations and produce error messages :-(. I need to think about this a bit.

+  if (!mPlayer && mReader) {
+    // We've shutdown the state machine before meta data was loaded, before
+    // mPlayer could be inited. We must destroy mReader here, since its
+    // ownership has not been taken over by mPlayer.
+    delete mReader;
+    mReader = 0;

Doesn't this leave nsOggDecoder::mReader dangling? Seems to me it would be cleaner to take mReader out of nsOggDecoderStateMachine, just get it through nsOggDecoder, and make mReader an nsAutoPtr in nsOggDecoder.

-  if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
+  if (GetAttr(kNameSpaceID_None, nsGkAtoms::src, src) && !src.IsEmpty()) {

This doesn't seem necessary. We'd still instantiate the Ogg decoder on any non-empty string. What we really need to do is delay decoder instantiation until we have opened the channel and received the MIME type. We need a bug on that (and a fix ASAP).

+          !src.IsEmpty() &&
           source->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type) &&
+          !type.IsEmpty() &&

Ditto, the src and type checks here shouldn't be necessary.
Oh, one thing I've wondered about: the nsDefaultStreamStrategy should have nsCrossSiteListenerProxy/CheckLoadURIWithPrincipal, just like HTTP, I think.
Depends on: 462878
As part of the changes to support access-control headers for downloaded fonts, I removed the pref to disable same-site checks.  In the security review, Jonas felt that it was generally not a good idea to allow prefs like this.  I'm happy to add it back in but either way I think we probably want to be consistent about this.
I think it's OK to leave it in while we don't know what the setting should be.

I started a WHATWG thread on this, by the way:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2008-November/017083.html
*why* do people want to tell "failed because of security restrictions" apart from "failed for other random reason"? Or more specifically, why do they want the loading webpage to tell the two apart?

It makes a lot of sense to me to log in the error console any failure due do security, so we should probably add that to the access-control code, but I don't see what script could do that is useful with that information.
See the end of this message:
http://lists.xiph.org/pipermail/theora/2008-November/001949.html

They want to be able to display scripted inline error messages, basically.
From the discussion in #theora they also want fallback. So if it can't be accessed via <video> they can fall back to <iframe> or a plugin.
Seems to me like the average user (joe the plumber) isn't going to be able to do much with that information. Even if he/she did understand the generally cryptic error message, it's not really something that the user can do anything about.

It also seems unlikely that this isn't something that the developer would have run in to before deploying the site.

While I agree there is a (weak) use case for this, it seems like the risks are much higher than the rewards.
Hmm.. fallback is a decent use case. Though you could try that no matter what error you get. Or simply use it from the get-go if you know you're embedding a site that doesn't support Access-Control.
This is in the context of writing a general purpose script (like mv_embed) that handles embedding videos with <video> if it's supported and falls back to java, plugins, etc. The target audience of the script is for someone who knows nothing about http servers, etc and just wants to play video. They know nothing about Access-Control and the script deals with the support or lack of support for it.
Yeah, I agree that it'd be useful in that case. Not sure what we can do really.

If we always disables cookies and auth when doing cross-site loading (which access-control initially would) we'd only be leaking information regarding the existence of files behind firewalls.

Maybe we can simply say that firewalls aren't useful for hiding the existence of files, we are effectively already saying that they aren't useful for hiding the existence of servers since you can launch timing attacks using <img>.

It seems very scary to say that that is in general ok. Proposals welcome.
The "general purpose script" can point webmasters to Firefox's Error Console :)
I brought this up on the whatwg mailing list and discussed it further on #whatwg with Hixie, Maciej and others. This seems to be the current status:

[11:59am] • Hixie looks at the API to see what is currently exposed
[12:00pm] Hixie: dimensions, size, bit rate, bandwidth to hosting site
[12:00pm] Hixie: and existence
...
[12:00pm] Hixie: sure
[12:01pm] roc: duration
[12:01pm] roc: format
[12:01pm] Hixie: format?
[12:01pm] roc: yeah
...
[12:08pm] Hixie: i guess what we should do is not have cross-site restrictions for now, but when we add them to make it so that if there is AC metadata and it is negative, to block everything
[12:08pm] Hixie: allowing opt-out of all video access, and opt-in to full metadata access
[12:08pm] Hixie: defaulting to basically the current API set
[12:09pm] Hixie: but we should be prepared to block that altogether if someone comes up with an unexpected attack vector
[12:10pm] roc: ok
[12:10pm] roc: so we'll leak all those things you mentioned, for media types?
[12:10pm] Hixie: yeah

Jonas later said to me (in private) "i'm firmly on same-origin when it comes to exposing data such as filesizes and playlengths".

My conclusion: for now we should not do Access Controls. We will need CheckLoadURIFromPrincipal of course. It's probably worth keeping the support code we've got in the current patch, but leaving the pref set to 'false'; that way we can enable cross-site checks instantly if we need to. That should be enough to close out this bug. Separately, we will need to ensure that load of a non-media type is treated identically to a 404 --- in particular, no progress events may be fired until we've determined we can handle the type. Someone should file a bug for that.

We may need to open separate bugs to determine the accessibility of file sizes and durations for cross-site loads of media objects. I'll leave that to Jonas. I'll note that if we want to restrict access to duration, that means we need to lobotomize the API pretty heavily --- no currentTime, no seeking, no reading of the current playback state to see if the video ended... that would break our XBL UI completely too.
I'd be a lot more comfortable if we in addition to that also didn't send any cookies with the request as we'd then only be leaking information that's protected by firewalls, not by passwords etc (something that is much easier to find).

We also need to prevent sending the loadstart event until we know that we're dealing with a video file rather than a random HTML file as the loadstart event gives away the filesize.

In any case I think we need to bring it to a wider discussion, especially with the moz security folks.
We had the discussion with the Moz security folks yesterday, and they didn't give us any definitive guidance.
And I launched a thread on the WHATWG list yesterday, with just a few responses so far. Where else should this be discussed?
Can we send cookies with same-site requests but not for cross-site requests? How hard is that?
Admittedly I wasn't able to follow everything that was said on the call yesterday, but it sounded to me like a lot of issues were settled as "it's not an issue if we use access-control opt-in". Also, I don't know if any of our security folks are reading the whatwg list. I've been a bit behind due to being sick for most part of last weekend which is why I haven't replied there yet, though I have elsewhere.

Yes, we can definitely allow cookies for same-site but not for cross-site. You just need to set the LOAD_ANONYMOUS flag whenever you're doing a cross-site load (also remember to check for redirects).
I was hoping the call would resolve the cross-site issue, and I said so from the beginning of the call. By the end, when we hadn't resolved it, I raised it again and asked for a definite conclusion, but no-one offered one. So I said I'd take it to WHATWG instead and everyone seemed fine with that. So I don't see the value of having another discussion with the same group.
Sorry if I'm coming across as ornery here, but given the competing interests and the fact that there is no "right answer", I'm afraid of having this discussion drag out interminably, and we don't have time or energy for that.
How about disable cross domain requests as originally discussed in this thread, provide the error code for 'cross domain disallowed' to help those that want scripting solutions. Then, as discussion matures on WHATWG, decide what to selectively enable/disable. Or will that not be possible between beta2 and whatever the next build is?

It may turn out the outcry over the disabling of cross domains was over nothing, everyone is fine with it, and it can stay with no issues.

If that's too extreme what you said in comment 54 sounds fine too.
i am missing what the recent discussion is about.

afaict, the only real concern is telling the existence of and some metadata of a file behind a firewall - this is no issue for me, because it probably can be done in other ways having in mind the current state of the interwebs.

stealing porno movies was fixed in another canvas bug.

so if this bug fixes file:// and chrome:// loading what problems remain?
Attached patch Patch v6 (obsolete) — — Splinter Review 
* Set pref browser.media.enforce_same_site_origin to false by default, so access control checks are OFF by default.
* Had to refactor nsHttpStreamStrategy::Open() to include a seek offset to resolve conflicts with recent changes.
* Added nsCrossSiteListenerProxy/CheckLoadURIWithPrincipal checks to nsDefaultStreamStrategy, just like the HTTP case.
* Added HTTP redirect test cases to mochitest.
* I checked, it looks like nsCrossSiteListenerProxy will set LOAD_ANONYMOUS when doing cross site loads, see end of nsCrossSiteListenerProxy::UpdateChannel(). That means our cross-site-permitted loads will by default set LOAD_ANONYMOUS. nsCrossSiteListenerProxy also checks redirects.
* I am still sending an error event to the media element when loads are disallowed.
Attachment #347415 - Attachment is obsolete: true
Attachment #347917 - Flags: review?(roc)
Attachment #347415 - Flags: review?(roc)
I believe Stop() can only be called on the main thread, so you don't need mIsStopping to be volatile or even to hold the lock while setting/testing it.

I think we should take out the new error code. We will never want to distinguish cross-site prohibitions from 404s.
Well, except this still sends credentials for cross-site requests when same-origin/Access Control is disabled, which Jonas didn't want.

To fix that, how about always using nsCrossSiteListenerProxy, but extending it with another parameter (or better still, a setter method) that when set forces CheckRequestApproved to always return NS_OK. Then we'd get nsCrossSiteListenerProxy's cross-site LOAD_ANONYMOUS behaviour but without its actual access checks.
Hmm, let's deal with comment #67 in a separate bug, if we do it at all. Just fix comment #68 and we can land this.
Attached patch Patch v7 (obsolete) — — Splinter Review 
* Removed new error code.
* Changed nsHTMLMediaElement::NetworkError(code) back to NetworkError().
* Made removed lock from nsOggDecoder::Stop(), made nsOggDecoder::mIsStopping non-volatile.
Attachment #347917 - Attachment is obsolete: true
Attachment #348037 - Flags: review?(roc)
Attachment #347917 - Flags: review?(roc)
Comment on attachment 348037 [details] [diff] [review]
Patch v7

Just remove the diff to nsIDOMHTMLMediaError.idl

+  // Flags if we've called Stop(). Prevents multiple events being
+  // sent to call Shutdown().
+  PRBool mIsStopping;

PRPackedBool, and mention that it's only used on the main thread.

Then post a new patch, carry forward r+sr, and add to the beta2 checkin queue.
Attachment #348037 - Flags: superreview+
Attachment #348037 - Flags: review?(roc)
Attachment #348037 - Flags: review+
Attached patch Patch v8 (obsolete) — — Splinter Review 
With review comments addressed. r+/sr+ roc.
Attachment #348037 - Attachment is obsolete: true
Attachment #348047 - Flags: superreview+
Attachment #348047 - Flags: review+
Attachment #348037 - Flags: approval1.9.1b2?
Comment on attachment 348037 [details] [diff] [review]
Patch v7

Can I have approval for beta? This patch prevents local files from being loadable (and thus detectable) in <video> from a webpage on another domain.
Attached patch Patch v8.1 (obsolete) — — Splinter Review 
V8 updated to reflect changes in bug 462878, upon which this relies. r+/sr+ roc.
Attachment #348047 - Attachment is obsolete: true
Attachment #348503 - Flags: superreview+
Attachment #348503 - Flags: review+
Attached patch v8.2 (obsolete) — — Splinter Review 
v8.2 - needed two more do_QueryInterface() to compile under Linux. Carrying forward r+/sr+ roc.
Attachment #348503 - Attachment is obsolete: true
Attachment #348521 - Flags: superreview+
Attachment #348521 - Flags: review+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Backed out due to test failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed ab6c0cda36b2
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
So I have some nits:

* The indent in nsIContentPolicy.idl is off
* No need to null-check the return value of NodePrincipal() (it's never null),
  or for that matter to store it in a strong pointer, necessarily.  At least
  across the sort of calls that you do here.
* nsHTMLMediaElement::ShouldCheckAllowOrigin should just be:

    return nsContentUtils::GetBoolPref("browser.media.enforce_same_site_origin",
                                       PR_TRUE);

* There's a fair amount of copy/paste in the nsMediaStream changes.  Could we
  refactor the security checks into a method on nsStreamStrategy that takes a
  boolean for whether to do the cross-site proxy stuff?  Or is that too hard
  due to maybe having to change the listener for some strategies but not others?
* We should probably file a followup bug on the fact that the CheckLoadURI
  check on the channel URI that nsCrossSiteListenerProxy does is not equivalent
  to a CheckLoadURI on the actual URI the channel was constructed with.  The
  latter is what we want.
* As far as that goes, how are the mURIs around correlated with the mChannels?
* In nsFileStreamStrategy::Open, why do we get the channel URI if we only use
  mURI?
* Can I assume that we never get into the stream strategy code if the content
  policy check in nsHTMLMediaElement failed?  Or is there some other reason
  that we don't need to do content policy checks in the strategies?
(In reply to comment #79)
> So I have some nits:

OK, cool.

> * There's a fair amount of copy/paste in the nsMediaStream changes.  Could we
>   refactor the security checks into a method on nsStreamStrategy that takes a
>   boolean for whether to do the cross-site proxy stuff?  Or is that too hard
>   due to maybe having to change the listener for some strategies but not
> others?

Originally I didn't do this, as I didn't want to imply that every strategy had to have an mListener; nsFileStreamStrategy doesn't need one. I could add another class nsCrossSiteStreamStrategy, from which nsDefaultStreamStrategy and nsHTTPStreamStratey inherit, and put the common code in there?

> * We should probably file a followup bug on the fact that the CheckLoadURI
>   check on the channel URI that nsCrossSiteListenerProxy does is not equivalent
>   to a CheckLoadURI on the actual URI the channel was constructed with.  The
>   latter is what we want.

Do you mean that every CheckLoadURIWithPrincipal() call should get the URI from the channel and check with that, rather than use mURI? I take it this handles redirects, and in general ensures we're checking what we think we're checking?

> * As far as that goes, how are the mURIs around correlated with the mChannels?

All the mURI's around should be references to the nsOggDecoder::mURI, each nsOggDecoder has one nsChannelReader which reads one channel through an nsMediaStream. nsOggDecoder::mURI is the initial URI used to open the channel. The nsMediaStream may recreate its channel upon seek. If we're loading an nsVideoDocument, we are passed an open channel, and nsMediaStream assumes ownership of that, and nsOggDecoder::mURI is that's channel's NS_GetFinalChannelURI().

> * In nsFileStreamStrategy::Open, why do we get the channel URI if we only use
>   mURI?

Oops, that's dead code.

> * Can I assume that we never get into the stream strategy code if the content
>   policy check in nsHTMLMediaElement failed?  Or is there some other reason
>   that we don't need to do content policy checks in the strategies?

Umm... It looks like we could get in through an nsVideoDocument load, we don't do a NS_CheckContentLoadPolicy check on the channel's URI on that code path. I'd not considered that until you mentioned it.
> I could add another class nsCrossSiteStreamStrategy, from which
> nsDefaultStreamStrategy and nsHTTPStreamStratey inherit, and put the common
> code in there?

That would work, but still duplicate the CheckLoadURI checks, no?

Don't worry about the nsCrossSiteListenerProxy; that's actually covered by a different patch.

> nsOggDecoder::mURI is the initial URI used to open the channel.

So it's the URI passed to newChannel, not the URI gotten from the channel via GetURI()?  OK, good.

If the only way we can get here without hitting content policy is through nsVideoDocument that should be fine.  I'm just worrying about cases like XHR result documents, where we actually depend on content policy to prevent subresource loads.  ;)
(In reply to comment #78)
> Pushed ab6c0cda36b2

For the record, this was backed out as tests where failing on the tinderbox.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Which test is failing? I didn't run all of them due to strange JS issues but still quite many - only one failing was a Mochitest testcase that seems to be font-dependent. Please tell me where to look, I would really like to see this patch checked in soon.
Wladimir, you could look this up yourself on tinderbox, if the logs are still there... just go back to that day and look at the log.
Last test failure was a Mochitest timeout on WinXP: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1227062530.1227065305.9769.gz

I think jwalden's patch for bug 465921 may fix this, we should get that landed.

We should get bug 468721 in as well, it may stop some of the crashes we got on Mac when this landed previously.

It would be lovely if we could get stacks for crashes on Tinderbox...

I've been pounding away at the intermittent test failures on <video> for the past few weeks, and I think if we get bug 465921 and bug 468721 landed we'll now stand a much better chance of this patch sticking...
Depends on: 465921, 468721
Minor changes:
* Fixed indent in nsIContentPolicy.idl.
* Now uses result of NodePrincipal() directly, without storing in strong ref.
* nsHTMLMediaElement::ShouldCheckAllowOrigin() now uses nsContentUtils::GetBoolPref().

Carrying r+/sr+ roc forward.
Attachment #348521 - Attachment is obsolete: true
Attachment #352621 - Flags: superreview+
Attachment #352621 - Flags: review+
I don't think this should be under "browser". I think we should just make "media" the root. So, just "media.enforce_same_site_origin".
No longer depends on: 465921
Depends on: 465921
Attached patch Patch v8.4 unbitrotten (obsolete) — — Splinter Review 
Unbitrotten and with media.enforce_same_site_origin renamed.
Attachment #352621 - Attachment is obsolete: true
Attachment #352958 - Flags: superreview+
Attachment #352958 - Flags: review+
Whiteboard: [sg:moderate?] → [sg:moderate?][needs landing]
Attached patch Patch v9 — — Splinter Review 
Now even more unbitrotten. Updated to reflect recent changes in the load algorithm. I had to move some code around, Roc you should probably cast your eye over this to make sure all the code movement is appropriate.
Attachment #352958 - Attachment is obsolete: true
Attachment #353615 - Flags: review?(roc)
Pushed a5587354082a with bustage fix 23021d6ef8b2 for mention of non-existent test
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [sg:moderate?][needs landing] → [sg:moderate?][needs 191 landing]
Backed out again to try to fix Windows failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231121149.1231124876.19286.gz
*** 28133 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_seek4.html | Video currentTime should be around 2: 0
*** 28135 INFO Running /tests/content/media/video/test/test_seek5.html...
*** 28136 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_seek5.html | First seek

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231122487.1231125951.22207.gz
*** 28118 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_seek1.html | Video currentTime should be around 2: 0
...
*** 28269 INFO Running /tests/content/media/video/test/test_timeupdate3.html...
command timed out: 300 seconds without output
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Whiteboard: [sg:moderate?][needs 191 landing] → [sg:moderate?][needs new patch]
At roc's request, I've split Patch v9 into two parts, the allow-origin check part, and the load policy checks part.

This patch is the access controls allow-origin check part. This should not change *any* behaviour as it's disabled by the pref by default. The load policy check part will follow.

When I had both patches applied, I also spotted an intermittent hang when I was running mochitest content/media/video/test/test_access_controls.html. I'm running on Vista. I only ever saw the hang once, but I often see these assertion failures:

###!!! ASSERTION: Potential deadlock between media.decoderMonitor@654a650 and Lock@6297e50: 'Error', file c:/work/objdirs/orange/x
pcom/build/nsAutoLock.cpp, line 318

That's a potential deadlock between nsOggDecoder::mMonitor and nsHttpStreamStrategy::mLock. Unfortunately I didn't grab stacktraces when I encountered the hang, but presumably that's the cause of the hang I saw. I tried to track that down, but didn't solve it.

I'm about to head overseas for about three weeks, with limited internet access, so it's unlikely that I'll be able to work on this for a few weeks...
This is the load policy checks. Requires Patch v10 1 of 2 to be applied to work.
Just to be clear, we split the patch up so that it can be checked in piecemeal, to help identify which part is causing test failures. Nothing in these patches should affect the locking in the ogg decoder, so I think the potential deadlock assertion and hang are just tickled by this patch's testcase, not caused by the patch.
I'm not sure whether it was the landing or the backout, but this warning came up:

content/media/video/src/nsMediaStream.cpp:608 - control reaches end of non-void function

http://office.smedbergs.us:8080/warning?signature=hg%3Acontent%2Fmedia%2Fvideo%2Fsrc%2FnsMediaStream.cpp%3Af941129cae01%3A510%3Acontrol+reaches+end+of+non-voi
Yeah, needs a return NS_OK there.
Part 1 was checked in and seems to have stuck OK --- changeset 698bf7ff3992.
(In reply to comment #96)
> Yeah, needs a return NS_OK there.

It's fixed with the checkin of part 1.
Attached patch v10 part 2a — — Splinter Review 
Part 2 can basically be landed hunk by hunk.
I landed part 2a. So far, we've had 4 test machines cycle on each platform. Mac and Linux are all fine, but the very first cycle (out of four) with this patch had this failure on Windows:

*** 27718 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_seek4.html | Video currentTime should be around 2: 0.032999999821186066
*** 27721 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_seek5.html | First seek

This resembles the previous failures that caused this patch to be backed out. But since the other cycles are green, and it's nigh impossible to see how part 2a could cause any sort of failure, I don't much feel like this patch is responsible.
I landed parts 2b-2e. Similar seek-time failures have happened in 2 of the last 7 cycles since that landing. When I have time, hopefully later today, I'll try backing out part 2a to see if that fixes them.
Backing out 2a did not fix the time-after-seek failures. So it seems adding any of these security checks can cause seeks to fail :-(.
I think we're going to have to just disable all seeking-related tests on Windows for now. These security checks are more important. We should have another bug on file to get those tests fixed and reenabled.
Disables seeking tests on windows.
Attachment #358975 - Flags: review?(roc)
Ok, this is fixed now.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [sg:moderate?][needs new patch] → [sg:moderate?][needds 191 landing]
I think I may have found a possible cause of the seeking problems while working on bug 469923. I noticed that the mSeeking and mOffset values were never set in nsChannelToPipeListener. 

The nsByteRangeEvent class used to create the nsChannelToPipeListener via:

 mListener = new nsChannelToPipeListener(mDecoder, PR_TRUE, mOffset);

It set mSeeking to PR_TRUE and mOffset to the offset value being seeked too. 

The new code from this bug uses OpenInternal, which never passes the offset or seeking value through to the nsChannelToPipeListener.
Interesting!

My canplaythrough patch removes mOffset BTW, but I think it's worth putting in a separate patch to fix your issue ASAP, especially if fixes the seek tests on Windows tinderbox.
Ok, I'll do the patch under bug 475369.
Depends on: 475517
This needs to land on 1.9.1, pronto. Please mark fixed1.9.1 when done.
Whiteboard: [sg:moderate?][needds 191 landing] → [sg:moderate?][needs 191 landing]
Landed on 1.9.1. Changesets:
fd75066f9135
c545752d355d
24ed98277b09
e57e9239d114
fc391a8e5130
ad44d5c0826b
f4576275b5ac
Keywords: fixed1.9.1
Whiteboard: [sg:moderate?][needs 191 landing] → [sg:moderate?]
Group: core-security
Depends on: 478652
Flags: wanted1.9.0.x-
hi, given the complexity of this patch and discussion, can someone here comfortably verify this is fixed for 1.9.1branch and trunk?   I'd do it if there are clear cut steps, but i think it'd be easier if one of you did.  Thanks!
One way to verify it is to check whether content policies are triggered for HTML media. E.g. with the following steps:

1. Install Adblock Plus 1.0.2.
2. Go to http://www.skierpage.com/moz_bugs/test_audio_video_tags.html
3. Click ABP icon in toolbar or press Ctrl+Shift+V to open the list of blockable items.

There should be three items in that list with type "audio/video", corresponding with the two audio files and one video embedded in that page. This works fine on 1.9.1 branch and trunk. However, content policies are only one aspect of this patch of course.
checking for existence of local files is fixed on trunk.
to verify, open the 2 testcass, select "check". getting security error in javascript console means it is fixed.
seems fixed on branch too.

don't get an error, don't get any result about local file existence,
i mean my testcases are fixed.
Wladimir, Thanks for the testcase!   Verified fixed on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090601 Shiretoko/3.5pre Ubiquity/0.1.5
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090601 Minefield/3.6a1pre
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Comment on attachment 353615 [details] [diff] [review]
Patch v9

>diff --git a/content/base/public/nsContentPolicyUtils.h b/content/base/public/nsContentPolicyUtils.h
>--- a/content/base/public/nsContentPolicyUtils.h
>+++ b/content/base/public/nsContentPolicyUtils.h
>@@ -138,6 +138,7 @@ NS_CP_ContentTypeName(PRUint32 contentTy
>     CASE_RETURN( TYPE_OBJECT_SUBREQUEST );
>     CASE_RETURN( TYPE_DTD               );
>     CASE_RETURN( TYPE_FONT              );
>+    CASE_RETURN( TYPE_MEDIA             );
>    default:
>     return "<Unknown Type>";
>   }
>diff --git a/content/base/public/nsIContentPolicy.idl b/content/base/public/nsIContentPolicy.idl
>--- a/content/base/public/nsIContentPolicy.idl
>+++ b/content/base/public/nsIContentPolicy.idl
>@@ -130,6 +130,11 @@ interface nsIContentPolicy : nsISupports
>    * Indicates a font loaded via @font-face rule.
>    */
>   const unsigned long TYPE_FONT = 14;
>+
>+  /**
>+   * Indicates a video or audio load.
>+   */
>+  const unsigned long TYPE_MEDIA = 15;  
We probably ought to update nsContentBlocker.cpp as well. I'll file a bug.
Thanks, though we never enabled this code, so I was wondering if we should actually remove the code and test instead.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: