Check the response URL origin in BufferedDataSource to avoid mixing cross-origin responses.

media/blink/buffered_data_source.cc

Index: media/blink/buffered_data_source.cc
diff --git a/media/blink/buffered_data_source.cc b/media/blink/buffered_data_source.cc
index d5f32b1ca298d4c787731c70eb26ead26632bea0..9d10a58e4a3a9910ccfa69554621f7d7dbc261e8 100644
--- a/media/blink/buffered_data_source.cc
+++ b/media/blink/buffered_data_source.cc
@@ -10,6 +10,8 @@
 #include "base/single_thread_task_runner.h"
 #include "media/base/media_log.h"
 #include "net/base/net_errors.h"
+#include "third_party/WebKit/public/platform/WebSecurityOrigin.h"
+#include "third_party/WebKit/public/web/WebFrame.h"
 
 using blink::WebFrame;
 
@@ -356,6 +358,7 @@ void BufferedDataSource::StartCallback(
     loader_->Stop();
     return;
   }
+  response_original_url_ = loader_->response_original_url();
 
   // All responses must be successful. Resources that are assumed to be fully
   // buffered must have a known content length.
@@ -403,8 +406,8 @@ void BufferedDataSource::PartialReadStartCallback(
     BufferedResourceLoader::Status status) {
   DCHECK(render_task_runner_->BelongsToCurrentThread());
   DCHECK(loader_.get());
-
-  if (status == BufferedResourceLoader::kOk) {
+  if (status == BufferedResourceLoader::kOk &&
+      CheckPartialResponseURL(loader_->response_original_url())) {
     // Once the request has started successfully, we can proceed with
     // reading from it.
     ReadInternal();
@@ -422,6 +425,25 @@ void BufferedDataSource::PartialReadStartCallback(
   ReadOperation::Run(read_op_.Pass(), kReadError);
 }
 
+bool BufferedDataSource::CheckPartialResponseURL(
+    const GURL& partial_response_original_url) const {
+  // If the SecurityOrigin of the frame can read content of the new response, we

[hubbe, 2015/07/06 17:22:47]

Why?
Why would we ever support redirects pointing to a new place in the middle of
playback?

Same question for the service worker thingy below.

[horo, 2015/07/07 01:06:46]

In current implementation, redirects pointing to a new place in the middle of
playback is supported.
And what I want to fix here is to prevent the data leak attack which is using
malicious header bytes and redirect pointing to target URL
(http://crbug.com/489060#c32).

If canRequest() is true, the content in the new URL is readable by the document
(eg: same origin.).
Service Worker scripts must be placed on the same origin server.
So when the response is generated in a Service Worker, it is OK to treat the
response as readable by the document.
When the new URL is readable by the document, it is OK to support the redirect.

According to https://crbug.com/74451#c8, YouTube and Vimeo are using redirect.
This patch http://codereview.chromium.org/6580014 "Make playback fail if
redirected to a different origin" was reverted because of those existing users.
Do you think we can say that there is no user who is using redirects in the
middle of playback now?

[falken, 2015/07/07 02:32:49]

To clarify: YouTube and Vimeo use redirects for the first response, but not
redirects to a new place in the middle of playback (so, we believe this patch
won't break them, unlike the patch that was reverted).

[hubbe, 2015/07/08 18:13:23]

That's what I thought too. Which I think means that this function doesn't need
the extra exceptions.

[horo, 2015/07/09 00:14:27]

I don't know the real world usage of the media elements.
Could you please let me know whether it is OK to allow same-origin redirects in
the middle of playback?

evn@ said OK. (crbug.com/505829#c15)
But if you say no, I will change to check the URL, not the origin URL.

+  // accept.
+  if (frame_->securityOrigin().canRequest(partial_response_original_url))
+    return true;
+
+  // If the response is generated in a Service Worker we accept.

[falken, 2015/07/07 02:32:49]

Please mention here something about why, i.e., the Service Worker script is
guaranteed to have the same origin as document requesting this URL, so it can
only lie to itself (is that correct?).

[horo, 2015/07/07 03:48:36]

yes.
added comments.

+  if (!partial_response_original_url.is_valid())

[falken, 2015/07/07 02:32:49]

is_empty() instead of is_valid

Sidenote: It's a bit sad this is the only way to detect a SW-generated response.

[horo, 2015/07/07 03:48:36]

Done.

+    return true;
+
+  // Otherwise we don't support mixing different origin responses. If we support
+  // this, malicious attackers can scan the bytes of other origin resources by
+  // mixing their generated bytes and the target response. See
+  // http://crbug.com/489060#c32 for details.
+  return response_original_url_.GetOrigin() ==
+         partial_response_original_url.GetOrigin();
+}
+
 void BufferedDataSource::ReadCallback(
     BufferedResourceLoader::Status status,
     int bytes_read) {