Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1220963004:
Check the response URL origin in BufferedDataSource to avoid mixing cross-origin responses. (Closed)
DescriptionCheck the response URL origin in BufferedDataSource to avoid mixing cross-origin responses.
In current implementation malicious attackers can scan the bytes of cross-origin resources by mixing their generated bytes and the target response.
See http://crbug.com/489060#c32 for details.
To avoid this, we have to deny mixing cross-origin responses in the middle of playback.
This CL introduces the check logic of the response URL origin of the partial responses.
When BufferedDataSource receives the first HTTP responses, it remembers the original URL of it.
And when BufferedDataSource receives the succeeding response, it checks the origin of the new response.
If the origin is not same as the origin of the first response, the response is treated as an error.
BUG=505829
TEST=media_blink_unittests with https://codereview.chromium.org/1221973002/, LayoutTests in https://codereview.chromium.org/1226473002/
Committed: https://crrev.com/7ab9c6c314f589251c85913bd0a360cba24eb76a
Cr-Commit-Position: refs/heads/master@{#338620}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : incorporated hubbe and falken's comment #
Total comments: 8
Patch Set 3 : Check Origin and canRequest #
Total comments: 11
Patch Set 4 : incorporated falken's comment #
Total comments: 2
Patch Set 5 : incorporated hubbe's comment #
Messages
Total messages: 43 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
dalecurtis@chromium.org changed reviewers: + hubbe@chromium.org
+hubbe
The description of this CL explains what it does, but not why. I tried looking at the bugreport, but that was not terribly enlightening either. Ideally, the explanation should be in the code I think.
I'm not the best reviewer for this, but some nits. https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... media/blink/buffered_data_source.h:242: // Service Worker this URL is empty. BufferedDataSource checks the orinal URL "original URL" (2 places) "a Service Worker" https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... media/blink/buffered_data_source.h:243: // of the succeeding response. If it is different from the original URL of the "of each successive response"? https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... File media/blink/buffered_data_source_unittest.cc (right): https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... media/blink/buffered_data_source_unittest.cc:111: static const char kHttpOtherUrl[] = "http://127.0.0.1/foo.webm"; The point is this url has a different origin than kHttpUrl. So kHttpOtherOriginUrl may be better. https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_re... File media/blink/buffered_resource_loader.h (right): https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_re... media/blink/buffered_resource_loader.h:208: // Returns the orinal URL of the response. If the response is generated in the "original URL" "a Service Worker"
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
On 2015/07/01 18:09:52, hubbe wrote: > The description of this CL explains what it does, but not why. I tried looking > at the bugreport, but that was not terribly enlightening either. > > Ideally, the explanation should be in the code I think. Done. Added comments in BufferedDataSource::PartialReadStartCallback() and in the cl description.
https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... media/blink/buffered_data_source.h:242: // Service Worker this URL is empty. BufferedDataSource checks the orinal URL On 2015/07/02 01:04:08, falken wrote: > "original URL" (2 places) > "a Service Worker" Done. https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... media/blink/buffered_data_source.h:243: // of the succeeding response. If it is different from the original URL of the On 2015/07/02 01:04:08, falken wrote: > "of each successive response"? Done. https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... File media/blink/buffered_data_source_unittest.cc (right): https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_da... media/blink/buffered_data_source_unittest.cc:111: static const char kHttpOtherUrl[] = "http://127.0.0.1/foo.webm"; On 2015/07/02 01:04:08, falken wrote: > The point is this url has a different origin than kHttpUrl. So > kHttpOtherOriginUrl may be better. > Done. https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_re... File media/blink/buffered_resource_loader.h (right): https://codereview.chromium.org/1220963004/diff/80001/media/blink/buffered_re... media/blink/buffered_resource_loader.h:208: // Returns the orinal URL of the response. If the response is generated in the On 2015/07/02 01:04:08, falken wrote: > "original URL" > "a Service Worker" Done.
non-owner lgtm https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source.cc:409: // We don't support mixed range responses. Otherwise malicious attackers can I'm not sure "mixed range response" is a term of art. Maybe "mixed-origin range response"
Actually I have some questions. https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source.cc:408: response_original_url_ == loader_->response_original_url()) { Actually a question. Should this really just compare origin instead of URL? And should you be able to mix your own SW's response with your own server's response? https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source.cc:427: ReadOperation::Run(read_op_.Pass(), kReadError); When we fail due to the security check, should we have a different error code so devs can know what failed? (It seems unlikely people would try to do this mixing legitimately though.)
Sorry comment again... https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... File media/blink/buffered_data_source_unittest.cc (right): https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source_unittest.cc:111: static const char kHttpOtherOriginUrl[] = "http://127.0.0.1/foo.webm"; I take my comment back, the current code is checking URL equality not origin equality. So it seems origin is not the point here, with the current code (maybe worth testing both cases?)
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source.cc:408: response_original_url_ == loader_->response_original_url()) { On 2015/07/03 04:00:25, falken wrote: > Actually a question. Should this really just compare origin instead of URL? And > should you be able to mix your own SW's response with your own server's > response? Changed to compare the origin. And added canRequest() check. https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source.cc:409: // We don't support mixed range responses. Otherwise malicious attackers can On 2015/07/03 03:43:44, falken wrote: > I'm not sure "mixed range response" is a term of art. Maybe "mixed-origin range > response" Changed to "mixing different origin responses". https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source.cc:427: ReadOperation::Run(read_op_.Pass(), kReadError); On 2015/07/03 04:00:25, falken wrote: > When we fail due to the security check, should we have a different error code so > devs can know what failed? > > (It seems unlikely people would try to do this mixing legitimately though.) It may be better to have. But BufferedDataSource BufferedResourceLoader are not providing such detailed error messages now. https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... File media/blink/buffered_data_source_unittest.cc (right): https://codereview.chromium.org/1220963004/diff/140001/media/blink/buffered_d... media/blink/buffered_data_source_unittest.cc:111: static const char kHttpOtherOriginUrl[] = "http://127.0.0.1/foo.webm"; On 2015/07/03 04:02:45, falken wrote: > I take my comment back, the current code is checking URL equality not origin > equality. So it seems origin is not the point here, with the current code (maybe > worth testing both cases?) Changed to use kHttpDifferentPathUrl and kHttpDifferentOriginUrl.
https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:430: // If the SecurityOrigin of the frame can read content of the new response, we 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.
https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:430: // If the SecurityOrigin of the frame can read content of the new response, we On 2015/07/06 17:22:47, hubbe wrote: > 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. 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?
I think this still looks lgtm https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:430: // If the SecurityOrigin of the frame can read content of the new response, we On 2015/07/07 01:06:46, horo wrote: > On 2015/07/06 17:22:47, hubbe wrote: > > 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. > > 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? 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). https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:435: // If the response is generated in a Service Worker we accept. 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?). https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:436: if (!partial_response_original_url.is_valid()) is_empty() instead of is_valid Sidenote: It's a bit sad this is the only way to detect a SW-generated response. https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_unittest.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_unittest.cc:528: ExecuteMixedResponseFailureTest(response1, response2); (as discussed offline) I was confused by this expectation since SW generated responses are OK in the CheckPartialResponseURL() function. It turns out kHttpUrl is treated as a different origin URL than the frame. It may be good to have a brief comment above each test explaining what's going on.
https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:435: // If the response is generated in a Service Worker we accept. On 2015/07/07 02:32:49, falken wrote: > 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?). yes. added comments. https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:436: if (!partial_response_original_url.is_valid()) On 2015/07/07 02:32:49, falken wrote: > is_empty() instead of is_valid > > Sidenote: It's a bit sad this is the only way to detect a SW-generated response. Done. https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source_unittest.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source_unittest.cc:528: ExecuteMixedResponseFailureTest(response1, response2); On 2015/07/07 02:32:49, falken wrote: > (as discussed offline) I was confused by this expectation since SW generated > responses are OK in the CheckPartialResponseURL() function. It turns out > kHttpUrl is treated as a different origin URL than the frame. It may be good to > have a brief comment above each test explaining what's going on. Done.
Patchset #4 (id:220001) has been deleted
hubbe@ Could you please review this? Thank you.
https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:430: // If the SecurityOrigin of the frame can read content of the new response, we On 2015/07/07 02:32:49, falken wrote: > On 2015/07/07 01:06:46, horo wrote: > > On 2015/07/06 17:22:47, hubbe wrote: > > > 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. > > > > 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? > > 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). That's what I thought too. Which I think means that this function doesn't need the extra exceptions. https://codereview.chromium.org/1220963004/diff/240001/media/blink/buffered_d... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/1220963004/diff/240001/media/blink/buffered_d... media/blink/buffered_data_source.h:244: // The original URL of the first response. If the response is generated in a This is the URL *after* redirects, right? Perhaps add that to the comments?
Patchset #5 (id:260001) has been deleted
https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... media/blink/buffered_data_source.cc:430: // If the SecurityOrigin of the frame can read content of the new response, we On 2015/07/08 18:13:23, hubbe wrote: > On 2015/07/07 02:32:49, falken wrote: > > On 2015/07/07 01:06:46, horo wrote: > > > On 2015/07/06 17:22:47, hubbe wrote: > > > > 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. > > > > > > 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? > > > > 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). > > That's what I thought too. Which I think means that this function doesn't need > the extra exceptions. 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. https://codereview.chromium.org/1220963004/diff/240001/media/blink/buffered_d... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/1220963004/diff/240001/media/blink/buffered_d... media/blink/buffered_data_source.h:244: // The original URL of the first response. If the response is generated in a On 2015/07/08 18:13:23, hubbe wrote: > This is the URL *after* redirects, right? > Perhaps add that to the comments? Yes. Done.
On 2015/07/09 00:14:27, horo wrote: > https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... > File media/blink/buffered_data_source.cc (right): > > https://codereview.chromium.org/1220963004/diff/200001/media/blink/buffered_d... > media/blink/buffered_data_source.cc:430: // If the SecurityOrigin of the frame > can read content of the new response, we > On 2015/07/08 18:13:23, hubbe wrote: > > On 2015/07/07 02:32:49, falken wrote: > > > On 2015/07/07 01:06:46, horo wrote: > > > > On 2015/07/06 17:22:47, hubbe wrote: > > > > > 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. > > > > > > > > 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? > > > > > > 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). > > > > That's what I thought too. Which I think means that this function doesn't need > > the extra exceptions. > > 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. > > https://codereview.chromium.org/1220963004/diff/240001/media/blink/buffered_d... > File media/blink/buffered_data_source.h (right): > > https://codereview.chromium.org/1220963004/diff/240001/media/blink/buffered_d... > media/blink/buffered_data_source.h:244: // The original URL of the first > response. If the response is generated in a > On 2015/07/08 18:13:23, hubbe wrote: > > This is the URL *after* redirects, right? > > Perhaps add that to the comments? > > Yes. Done. hubbe@ ping?
lgtm
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/1220963004/#ps280001 (title: "incorporated hubbe's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220963004/280001
The CQ bit was unchecked by horo@chromium.org
horo@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@ Could you please review this?
lgtm
On 2015/07/14 00:39:49, DaleCurtis wrote: > lgtm Thank you!
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1220963004/280001
Message was sent while issue was closed.
Committed patchset #5 (id:280001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7ab9c6c314f589251c85913bd0a360cba24eb76a Cr-Commit-Position: refs/heads/master@{#338620} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
