Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1608573002:
RDH: Block a compromised renderer from reusing request ids (Closed)
|
Created:
4 years, 11 months ago by gzobqq Modified:
4 years, 10 months ago CC:
loading-reviews_chromium.org, darin-cc_chromium.org, jam, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlock a compromised renderer from reusing request ids.
BUG=578882
Committed: https://crrev.com/1af4fada49c4f3890f16daac31d38379a9d782b2
Cr-Commit-Position: refs/heads/master@{#372547}
Patch Set 1 #Patch Set 2 : Fix resource_loader_unittest.cc #
Total comments: 1
Patch Set 3 : Check pending_loaders_ and blocked_loaders_map_ instead #Patch Set 4 : Declaration order #Patch Set 5 : Add a test #
Total comments: 8
Patch Set 6 : Address comments #
Total comments: 12
Patch Set 7 : Rebase #Patch Set 8 : Address comments #Patch Set 9 : Rebase #
Messages
Total messages: 38 (11 generated)
Description was changed from ========== Fix RDH delegate_map_.end() dereference BUG=578882 ========== to ========== Block a compromised renderer from reusing request ids. BUG=578882 ==========
gzobqq@gmail.com changed reviewers: + mmenke@chromium.org
mmenke, PTAL. This adds a request_ids_in_use_ set to RDH. It is used to validate that a request id from the renderer is unique and doesn't overwrite pending_loaders_ or delegate_map_ entries. It's not so nice that information is duplicated. The set of request ids in use can also be obtained from pending_loaders_ and blocked_loaders_map_, but the latter would have to be iterated. Another option would probably be to generate request ids on browser side. I figured an extra set is the safest option for merging.
On 2016/01/19 14:11:16, gzobqq wrote: > mmenke, PTAL. > > This adds a request_ids_in_use_ set to RDH. It is used to validate that a > request id from the renderer is unique and doesn't overwrite pending_loaders_ or > delegate_map_ entries. It's not so nice that information is duplicated. The set > of request ids in use can also be obtained from pending_loaders_ and > blocked_loaders_map_, but the latter would have to be iterated. Another option > would probably be to generate request ids on browser side. I figured an extra > set is the safest option for merging. Could I get access to the bug this issue references?
https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1177: void ResourceDispatcherHostImpl::UpdateRequestForTransfer( BUG: This changes the id of a request, so needs to update the set as well. I'm not a huge fan of the information duplicating approach because it inevitably leads to bug like this. Wonder if we can come up with something better...
So here's my suggestion: Check blocked_loaders_map_ and pending_loaders_. blocked_loaders_map_ should generally be empty, anyways, so I'm not sure we need to go further than that (I believe it's only populated when we're showing an interstitial page). If we do, however, we could make it contain maps as well, and just jump to the one entry we care about.
On 2016/01/19 20:30:31, mmenke wrote: > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > BUG: This changes the id of a request, so needs to update the set as well. I'm > not a huge fan of the information duplicating approach because it inevitably > leads to bug like this. Wonder if we can come up with something better... Well darn. Can't believe I forgot about this. Thanks for catching. But yeah, let's go with checking pending_loaders_ and blocked_loaders_map_ instead then.
On 2016/01/20 05:51:01, gzobqq wrote: > On 2016/01/19 20:30:31, mmenke wrote: > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > BUG: This changes the id of a request, so needs to update the set as well. > I'm > > not a huge fan of the information duplicating approach because it inevitably > > leads to bug like this. Wonder if we can come up with something better... > > Well darn. Can't believe I forgot about this. Thanks for catching. But yeah, > let's go with checking pending_loaders_ and blocked_loaders_map_ instead then. Great. Also, how unit/browser-testable are bad_message::ReceivedBadMessage calls? I generally feel that every fix should come with a test. I'm less sure about issues like this, but then, perhaps it should be considered more important to have tests because it's considered a security issue.
On 2016/01/20 15:38:23, mmenke wrote: > On 2016/01/20 05:51:01, gzobqq wrote: > > On 2016/01/19 20:30:31, mmenke wrote: > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > > BUG: This changes the id of a request, so needs to update the set as well. > > I'm > > > not a huge fan of the information duplicating approach because it inevitably > > > leads to bug like this. Wonder if we can come up with something better... > > > > Well darn. Can't believe I forgot about this. Thanks for catching. But yeah, > > let's go with checking pending_loaders_ and blocked_loaders_map_ instead then. > > Great. Also, how unit/browser-testable are bad_message::ReceivedBadMessage > calls? I generally feel that every fix should come with a test. I'm less sure > about issues like this, but then, perhaps it should be considered more important > to have tests because it's considered a security issue. Ahh, you're not using a chromium.org account, sorry, I didn't notice. I'll look into the testing situation myself.
On 2016/01/20 15:41:44, mmenke wrote: > On 2016/01/20 15:38:23, mmenke wrote: > > On 2016/01/20 05:51:01, gzobqq wrote: > > > On 2016/01/19 20:30:31, mmenke wrote: > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > > > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > > > BUG: This changes the id of a request, so needs to update the set as > well. > > > I'm > > > > not a huge fan of the information duplicating approach because it > inevitably > > > > leads to bug like this. Wonder if we can come up with something better... > > > > > > Well darn. Can't believe I forgot about this. Thanks for catching. But yeah, > > > let's go with checking pending_loaders_ and blocked_loaders_map_ instead > then. > > > > Great. Also, how unit/browser-testable are bad_message::ReceivedBadMessage > > calls? I generally feel that every fix should come with a test. I'm less > sure > > about issues like this, but then, perhaps it should be considered more > important > > to have tests because it's considered a security issue. > > Ahh, you're not using a http://chromium.org account, sorry, I didn't notice. I'll look > into the testing situation myself. Here is the second attempt which checks the maps. About testing, perhaps overriding ResourceMessageFilter::ShutdownForBadMessage() in content/browser/loader/resource_dispatcher_host_unittest.cc would do it. There's something similar in content/browser/service_worker/service_worker_handle_unittest.cc. My gmail account is really not an excuse for not writing a test. But I'm very much willing to leave this to you if you're up for it. If you're busy though, I can take it. Might take me some time, I'm slow paced.
On 2016/01/20 18:57:48, gzobqq wrote: > On 2016/01/20 15:41:44, mmenke wrote: > > On 2016/01/20 15:38:23, mmenke wrote: > > > On 2016/01/20 05:51:01, gzobqq wrote: > > > > On 2016/01/19 20:30:31, mmenke wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > > > > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > > > > BUG: This changes the id of a request, so needs to update the set as > > well. > > > > I'm > > > > > not a huge fan of the information duplicating approach because it > > inevitably > > > > > leads to bug like this. Wonder if we can come up with something > better... > > > > > > > > Well darn. Can't believe I forgot about this. Thanks for catching. But > yeah, > > > > let's go with checking pending_loaders_ and blocked_loaders_map_ instead > > then. > > > > > > Great. Also, how unit/browser-testable are bad_message::ReceivedBadMessage > > > calls? I generally feel that every fix should come with a test. I'm less > > sure > > > about issues like this, but then, perhaps it should be considered more > > important > > > to have tests because it's considered a security issue. > > > > Ahh, you're not using a http://chromium.org account, sorry, I didn't notice. > I'll look > > into the testing situation myself. > > Here is the second attempt which checks the maps. About testing, perhaps > overriding ResourceMessageFilter::ShutdownForBadMessage() in > content/browser/loader/resource_dispatcher_host_unittest.cc would do it. There's > something similar in > content/browser/service_worker/service_worker_handle_unittest.cc. My gmail > account is really not an excuse for not writing a test. But I'm very much > willing to leave this to you if you're up for it. If you're busy though, I can > take it. Might take me some time, I'm slow paced. I'll do the legwork to figure out what would be involved in writing a test - I think it's reasonable to ask for a test, just since you're not working on Chrome full time, I don't feel it's reasonable to expect you to figure out a reasonable approach (And if I have time, I may also be willing to write it, we'll see).
On 2016/01/21 16:32:34, mmenke wrote: > On 2016/01/20 18:57:48, gzobqq wrote: > > On 2016/01/20 15:41:44, mmenke wrote: > > > On 2016/01/20 15:38:23, mmenke wrote: > > > > On 2016/01/20 05:51:01, gzobqq wrote: > > > > > On 2016/01/19 20:30:31, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > > > > > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > > > > > BUG: This changes the id of a request, so needs to update the set as > > > well. > > > > > I'm > > > > > > not a huge fan of the information duplicating approach because it > > > inevitably > > > > > > leads to bug like this. Wonder if we can come up with something > > better... > > > > > > > > > > Well darn. Can't believe I forgot about this. Thanks for catching. But > > yeah, > > > > > let's go with checking pending_loaders_ and blocked_loaders_map_ instead > > > then. > > > > > > > > Great. Also, how unit/browser-testable are > bad_message::ReceivedBadMessage > > > > calls? I generally feel that every fix should come with a test. I'm less > > > sure > > > > about issues like this, but then, perhaps it should be considered more > > > important > > > > to have tests because it's considered a security issue. > > > > > > Ahh, you're not using a http://chromium.org account, sorry, I didn't notice. > > > I'll look > > > into the testing situation myself. > > > > Here is the second attempt which checks the maps. About testing, perhaps > > overriding ResourceMessageFilter::ShutdownForBadMessage() in > > content/browser/loader/resource_dispatcher_host_unittest.cc would do it. > There's > > something similar in > > content/browser/service_worker/service_worker_handle_unittest.cc. My gmail > > account is really not an excuse for not writing a test. But I'm very much > > willing to leave this to you if you're up for it. If you're busy though, I can > > take it. Might take me some time, I'm slow paced. > > I'll do the legwork to figure out what would be involved in writing a test - I > think it's reasonable to ask for a test, just since you're not working on Chrome > full time, I don't feel it's reasonable to expect you to figure out a reasonable > approach (And if I have time, I may also be willing to write it, we'll see). Sounds good, thank you.
On 2016/01/21 17:34:33, gzobqq wrote: > On 2016/01/21 16:32:34, mmenke wrote: > > On 2016/01/20 18:57:48, gzobqq wrote: > > > On 2016/01/20 15:41:44, mmenke wrote: > > > > On 2016/01/20 15:38:23, mmenke wrote: > > > > > On 2016/01/20 05:51:01, gzobqq wrote: > > > > > > On 2016/01/19 20:30:31, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > > File content/browser/loader/resource_dispatcher_host_impl.cc > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > > > > > > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > > > > > > BUG: This changes the id of a request, so needs to update the set > as > > > > well. > > > > > > I'm > > > > > > > not a huge fan of the information duplicating approach because it > > > > inevitably > > > > > > > leads to bug like this. Wonder if we can come up with something > > > better... > > > > > > > > > > > > Well darn. Can't believe I forgot about this. Thanks for catching. But > > > yeah, > > > > > > let's go with checking pending_loaders_ and blocked_loaders_map_ > instead > > > > then. > > > > > > > > > > Great. Also, how unit/browser-testable are > > bad_message::ReceivedBadMessage > > > > > calls? I generally feel that every fix should come with a test. I'm > less > > > > sure > > > > > about issues like this, but then, perhaps it should be considered more > > > > important > > > > > to have tests because it's considered a security issue. > > > > > > > > Ahh, you're not using a http://chromium.org account, sorry, I didn't > notice. > > > > > I'll look > > > > into the testing situation myself. > > > > > > Here is the second attempt which checks the maps. About testing, perhaps > > > overriding ResourceMessageFilter::ShutdownForBadMessage() in > > > content/browser/loader/resource_dispatcher_host_unittest.cc would do it. > > There's > > > something similar in > > > content/browser/service_worker/service_worker_handle_unittest.cc. My gmail > > > account is really not an excuse for not writing a test. But I'm very much > > > willing to leave this to you if you're up for it. If you're busy though, I > can > > > take it. Might take me some time, I'm slow paced. > > > > I'll do the legwork to figure out what would be involved in writing a test - I > > think it's reasonable to ask for a test, just since you're not working on > Chrome > > full time, I don't feel it's reasonable to expect you to figure out a > reasonable > > approach (And if I have time, I may also be willing to write it, we'll see). > > Sounds good, thank you. Sorry for slowness. content/browser/security_exploit_browsertest.cc looks to be the place you want to add a test. See InvalidOriginHeaders, in particular. I think it's safe to use IpcSecurityTestUtil::PwnMessageReceived for both the initial request, and the followup with the duplicate id. Ideally we'd have a test where the initial request is in the BlockedLoadersList, and a test where it's in the pending list. For a URL to request, to avoid any chance the request will actually complete, I think it's best to use net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(net::URLRequestFailedJob::START, net::ERR_IO_PENDING) as the URL. To use the mocks, you'll need to add code in the test fixture's SetUpOnMainThread() method to post a task to the IOThread to call net::URLRequestFailedJob::AddUrlHandler(); Happy to talk more about the details of setting up the test.
On 2016/01/25 20:08:31, mmenke wrote: > On 2016/01/21 17:34:33, gzobqq wrote: > > On 2016/01/21 16:32:34, mmenke wrote: > > > On 2016/01/20 18:57:48, gzobqq wrote: > > > > On 2016/01/20 15:41:44, mmenke wrote: > > > > > On 2016/01/20 15:38:23, mmenke wrote: > > > > > > On 2016/01/20 05:51:01, gzobqq wrote: > > > > > > > On 2016/01/19 20:30:31, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > > > File content/browser/loader/resource_dispatcher_host_impl.cc > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: void > > > > > > > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > > > > > > > BUG: This changes the id of a request, so needs to update the set > > as > > > > > well. > > > > > > > I'm > > > > > > > > not a huge fan of the information duplicating approach because it > > > > > inevitably > > > > > > > > leads to bug like this. Wonder if we can come up with something > > > > better... > > > > > > > > > > > > > > Well darn. Can't believe I forgot about this. Thanks for catching. > But > > > > yeah, > > > > > > > let's go with checking pending_loaders_ and blocked_loaders_map_ > > instead > > > > > then. > > > > > > > > > > > > Great. Also, how unit/browser-testable are > > > bad_message::ReceivedBadMessage > > > > > > calls? I generally feel that every fix should come with a test. I'm > > less > > > > > sure > > > > > > about issues like this, but then, perhaps it should be considered more > > > > > important > > > > > > to have tests because it's considered a security issue. > > > > > > > > > > Ahh, you're not using a http://chromium.org account, sorry, I didn't > > notice. > > > > > > > I'll look > > > > > into the testing situation myself. > > > > > > > > Here is the second attempt which checks the maps. About testing, perhaps > > > > overriding ResourceMessageFilter::ShutdownForBadMessage() in > > > > content/browser/loader/resource_dispatcher_host_unittest.cc would do it. > > > There's > > > > something similar in > > > > content/browser/service_worker/service_worker_handle_unittest.cc. My gmail > > > > account is really not an excuse for not writing a test. But I'm very much > > > > willing to leave this to you if you're up for it. If you're busy though, I > > can > > > > take it. Might take me some time, I'm slow paced. > > > > > > I'll do the legwork to figure out what would be involved in writing a test - > I > > > think it's reasonable to ask for a test, just since you're not working on > > Chrome > > > full time, I don't feel it's reasonable to expect you to figure out a > > reasonable > > > approach (And if I have time, I may also be willing to write it, we'll see). > > > > Sounds good, thank you. > > Sorry for slowness. content/browser/security_exploit_browsertest.cc looks to be > the place you want to add a test. See InvalidOriginHeaders, in particular. I > think it's safe to use IpcSecurityTestUtil::PwnMessageReceived for both the > initial request, and the followup with the duplicate id. > > Ideally we'd have a test where the initial request is in the BlockedLoadersList, > and a test where it's in the pending list. > > For a URL to request, to avoid any chance the request will actually complete, I > think it's best to use > net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(net::URLRequestFailedJob::START, > net::ERR_IO_PENDING) as the URL. To use the mocks, you'll need to add code in > the test fixture's SetUpOnMainThread() method to post a task to the IOThread to > call net::URLRequestFailedJob::AddUrlHandler(); > > Happy to talk more about the details of setting up the test. Thanks for figuring this out. I am now working on it.
On 2016/01/26 10:07:46, gzobqq wrote: > On 2016/01/25 20:08:31, mmenke wrote: > > On 2016/01/21 17:34:33, gzobqq wrote: > > > On 2016/01/21 16:32:34, mmenke wrote: > > > > On 2016/01/20 18:57:48, gzobqq wrote: > > > > > On 2016/01/20 15:41:44, mmenke wrote: > > > > > > On 2016/01/20 15:38:23, mmenke wrote: > > > > > > > On 2016/01/20 05:51:01, gzobqq wrote: > > > > > > > > On 2016/01/19 20:30:31, mmenke wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > > > > File content/browser/loader/resource_dispatcher_host_impl.cc > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1608573002/diff/20001/content/browser/loader/... > > > > > > > > > content/browser/loader/resource_dispatcher_host_impl.cc:1177: > void > > > > > > > > > ResourceDispatcherHostImpl::UpdateRequestForTransfer( > > > > > > > > > BUG: This changes the id of a request, so needs to update the > set > > > as > > > > > > well. > > > > > > > > I'm > > > > > > > > > not a huge fan of the information duplicating approach because > it > > > > > > inevitably > > > > > > > > > leads to bug like this. Wonder if we can come up with something > > > > > better... > > > > > > > > > > > > > > > > Well darn. Can't believe I forgot about this. Thanks for catching. > > But > > > > > yeah, > > > > > > > > let's go with checking pending_loaders_ and blocked_loaders_map_ > > > instead > > > > > > then. > > > > > > > > > > > > > > Great. Also, how unit/browser-testable are > > > > bad_message::ReceivedBadMessage > > > > > > > calls? I generally feel that every fix should come with a test. > I'm > > > less > > > > > > sure > > > > > > > about issues like this, but then, perhaps it should be considered > more > > > > > > important > > > > > > > to have tests because it's considered a security issue. > > > > > > > > > > > > Ahh, you're not using a http://chromium.org account, sorry, I didn't > > > notice. > > > > > > > > > I'll look > > > > > > into the testing situation myself. > > > > > > > > > > Here is the second attempt which checks the maps. About testing, perhaps > > > > > overriding ResourceMessageFilter::ShutdownForBadMessage() in > > > > > content/browser/loader/resource_dispatcher_host_unittest.cc would do it. > > > > There's > > > > > something similar in > > > > > content/browser/service_worker/service_worker_handle_unittest.cc. My > gmail > > > > > account is really not an excuse for not writing a test. But I'm very > much > > > > > willing to leave this to you if you're up for it. If you're busy though, > I > > > can > > > > > take it. Might take me some time, I'm slow paced. > > > > > > > > I'll do the legwork to figure out what would be involved in writing a test > - > > I > > > > think it's reasonable to ask for a test, just since you're not working on > > > Chrome > > > > full time, I don't feel it's reasonable to expect you to figure out a > > > reasonable > > > > approach (And if I have time, I may also be willing to write it, we'll > see). > > > > > > Sounds good, thank you. > > > > Sorry for slowness. content/browser/security_exploit_browsertest.cc looks to > be > > the place you want to add a test. See InvalidOriginHeaders, in particular. I > > think it's safe to use IpcSecurityTestUtil::PwnMessageReceived for both the > > initial request, and the followup with the duplicate id. > > > > Ideally we'd have a test where the initial request is in the > BlockedLoadersList, > > and a test where it's in the pending list. > > > > For a URL to request, to avoid any chance the request will actually complete, > I > > think it's best to use > > > net::URLRequestFailedJob::GetMockHttpUrlWithFailurePhase(net::URLRequestFailedJob::START, > > net::ERR_IO_PENDING) as the URL. To use the mocks, you'll need to add code in > > the test fixture's SetUpOnMainThread() method to post a task to the IOThread > to > > call net::URLRequestFailedJob::AddUrlHandler(); > > > > Happy to talk more about the details of setting up the test. > > Thanks for figuring this out. I am now working on it. PTAL
Thanks for the test! The looks good, just a few minor things. To reduce round-trips, suggest you add an owner for the security tests (tsepez or creis) once you've responded to comments. Since creis owns bad_messages as well, suggest you go with him. You also need a histograms.xml owner - anyone in the histograms owners file should be fine. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... File content/browser/security_exploit_browsertest.cc (right): https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:103: ResourceHostMsg_Request CreateDefaultRequest() { Optional: Suggest making this take in a url, and setting the request.url to it. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:144: child_id, web_rfh->GetRoutingID()); This needs to be run on the IO thread, and we're currently on the UI thread. You can just post a task to run this to the IO thread - no need to wait for it to complete, since everything else this needs to run before will also happen on the IO thread. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:149: // blocks indefinitely, which is good because the request stays alive and we nit (x2): Avoid "we" in comments, due to ambiguity (Does it mean "we" the chromium authors, "we" the object, "we" the code, etc). Prefer passive voice. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:437: ResourceHostMsg_RequestResource(web_rfh->GetRoutingID(), 1, These tests make me nervous now - we're depending on the request for the navigation either not to use ID 1, or to be completely cleaned up before this message reaches the IO thread. Otherwise, the new check will cause these tests to pass, even if what they're testing breaks. Could we just declare a constant "const int kRequestIdNotPreviouslyUsed = 10000" and use that for all of these (It's not a problem using the same ID in each test, because in each case the old RFH is destroyed, so the new one has a new ID). May want to do that for your new test as well, though it shouldn't really matter, there, since even if we match the ID of the original request, and it's not cleaned up yet, we'll still crash the process the right way.
gzobqq@gmail.com changed reviewers: + creis@chromium.org, mpearson@chromium.org
mmenke, PTAL creis, PTAL at security_exploit_browsertest.cc and bad_message.h mpearson, PTAL at histograms.xml Thanks. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... File content/browser/security_exploit_browsertest.cc (right): https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:103: ResourceHostMsg_Request CreateDefaultRequest() { On 2016/01/28 16:20:08, mmenke wrote: > Optional: Suggest making this take in a url, and setting the request.url to it. Done. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:144: child_id, web_rfh->GetRoutingID()); On 2016/01/28 16:20:08, mmenke wrote: > This needs to be run on the IO thread, and we're currently on the UI thread. > You can just post a task to run this to the IO thread - no need to wait for it > to complete, since everything else this needs to run before will also happen on > the IO thread. Good catch, done. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:149: // blocks indefinitely, which is good because the request stays alive and we On 2016/01/28 16:20:08, mmenke wrote: > nit (x2): Avoid "we" in comments, due to ambiguity (Does it mean "we" the > chromium authors, "we" the object, "we" the code, etc). Prefer passive voice. Done. https://codereview.chromium.org/1608573002/diff/80001/content/browser/securit... content/browser/security_exploit_browsertest.cc:437: ResourceHostMsg_RequestResource(web_rfh->GetRoutingID(), 1, On 2016/01/28 16:20:08, mmenke wrote: > These tests make me nervous now - we're depending on the request for the > navigation either not to use ID 1, or to be completely cleaned up before this > message reaches the IO thread. Otherwise, the new check will cause these tests > to pass, even if what they're testing breaks. > > Could we just declare a constant "const int kRequestIdNotPreviouslyUsed = 10000" > and use that for all of these (It's not a problem using the same ID in each > test, because in each case the old RFH is destroyed, so the new one has a new > ID). > > May want to do that for your new test as well, though it shouldn't really > matter, there, since even if we match the ID of the original request, and it's > not cleaned up yet, we'll still crash the process the right way. Done.
histograms.xml lgtm
LGTM, just two nits. Thanks again both for catching this, and for the fix! https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... File content/browser/security_exploit_browsertest.cc (right): https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:49: const int kRequestIdNotPreviouslyUsed = 10000; nit: Should have a comment about this. https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:126: ResourceHostMsg_Request request = CreateXHRRequest("http://bar.com/simple_page.html"); nit: Split onto two lines, so no line is over 80 characters.
Thanks! content/ LGTM with nits. https://codereview.chromium.org/1608573002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1608573002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1177: for (BlockedLoadersMap::const_iterator iter = blocked_loaders_map_.begin(); nit: Maybe use C++11 range-based for loop here, similar to ResourceDispatcherHostImpl::OnShutdown? Would be a little more concise. (For reference, the allowed C++11 uses are here: https://chromium-cpp.appspot.com/.) https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... File content/browser/security_exploit_browsertest.cc (right): https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:126: ResourceHostMsg_Request request = CreateXHRRequest("http://bar.com/simple_page.html"); On 2016/01/28 21:12:33, mmenke wrote: > nit: Split onto two lines, so no line is over 80 characters. Tip: Running "git cl format" should take care of this automatically. :) https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:134: RenderFrameHost* web_rfh = shell->web_contents()->GetMainFrame(); nit: s/web_rfh/rfh/ (The web_ prefix was useful in the InvalidOriginHeaders test because the logic was specific to RFHs in web processes, as opposed to chrome:// or other privileged processes. This test applies to all renderer processes.) Same below with web_process_killed. https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:156: // Use the same request id twice. nit: Move comment above the RenderProcessHostWatcher declaration, since it's a good description of the whole block.
https://codereview.chromium.org/1608573002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1608573002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1177: for (BlockedLoadersMap::const_iterator iter = blocked_loaders_map_.begin(); On 2016/01/28 22:05:16, Charlie Reis wrote: > nit: Maybe use C++11 range-based for loop here, similar to > ResourceDispatcherHostImpl::OnShutdown? Would be a little more concise. > > (For reference, the allowed C++11 uses are here: > https://chromium-cpp.appspot.com/.) Done. https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... File content/browser/security_exploit_browsertest.cc (right): https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:49: const int kRequestIdNotPreviouslyUsed = 10000; On 2016/01/28 21:12:33, mmenke wrote: > nit: Should have a comment about this. Done. https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:126: ResourceHostMsg_Request request = CreateXHRRequest("http://bar.com/simple_page.html"); On 2016/01/28 21:12:33, mmenke wrote: > nit: Split onto two lines, so no line is over 80 characters. Done. https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:126: ResourceHostMsg_Request request = CreateXHRRequest("http://bar.com/simple_page.html"); On 2016/01/28 22:05:16, Charlie Reis wrote: > On 2016/01/28 21:12:33, mmenke wrote: > > nit: Split onto two lines, so no line is over 80 characters. > > Tip: Running "git cl format" should take care of this automatically. :) That helps, thanks. https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:134: RenderFrameHost* web_rfh = shell->web_contents()->GetMainFrame(); On 2016/01/28 22:05:16, Charlie Reis wrote: > nit: s/web_rfh/rfh/ > > (The web_ prefix was useful in the InvalidOriginHeaders test because the logic > was specific to RFHs in web processes, as opposed to chrome:// or other > privileged processes. This test applies to all renderer processes.) > > Same below with web_process_killed. Done. https://codereview.chromium.org/1608573002/diff/100001/content/browser/securi... content/browser/security_exploit_browsertest.cc:156: // Use the same request id twice. On 2016/01/28 22:05:16, Charlie Reis wrote: > nit: Move comment above the RenderProcessHostWatcher declaration, since it's a > good description of the whole block. Done.
The CQ bit was checked by gzobqq@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1608573002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1608573002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
bad_message.h is moving faster than I can manage to rebase and recompile :)
The CQ bit was checked by gzobqq@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1608573002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1608573002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gzobqq@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, mpearson@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1608573002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1608573002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1608573002/160001
Message was sent while issue was closed.
Description was changed from ========== Block a compromised renderer from reusing request ids. BUG=578882 ========== to ========== Block a compromised renderer from reusing request ids. BUG=578882 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Block a compromised renderer from reusing request ids. BUG=578882 ========== to ========== Block a compromised renderer from reusing request ids. BUG=578882 Committed: https://crrev.com/1af4fada49c4f3890f16daac31d38379a9d782b2 Cr-Commit-Position: refs/heads/master@{#372547} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1af4fada49c4f3890f16daac31d38379a9d782b2 Cr-Commit-Position: refs/heads/master@{#372547} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
