Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707133003:
Fix SSL certificate being wrong in the intended_as_new_entry fase of NAVIGATION_TYPE_EXISTING_PAGE. (Closed)
DescriptionFix SSL certificate being wrong in the intended_as_new_entry fase of NAVIGATION_TYPE_EXISTING_PAGE.
It turns out that in some rare race conditions, this section of the code could get reached with a different origin for the committed page and the NavigationEntry that was reused. In that case, make sure to use the SSL certificate from the NavigationHandle instead of resusing the old one.
BUG=688425
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2707133003
Cr-Commit-Position: refs/heads/master@{#452106}
Committed: https://chromium.googlesource.com/chromium/src/+/a78746ec1a1bfa668f5bcb01d2b2665d2c514369
Patch Set 1 #
Total comments: 3
Patch Set 2 : merge #Patch Set 3 : update #Messages
Total messages: 21 (13 generated)
Description was changed from ========== Fix SSL certificate being wrong in the intended_as_new_entry fase of NAVIGATION_TYPE_EXISTING_PAGE. It turns out that in some rare race conditions, this section of the code could get reached with a different origin for the committed page and the NavigationEntry that was reused. In that case, make sure to use the SSL certificate from the NavigationHandle instead of resusing the old one. BUG=688425 ========== to ========== Fix SSL certificate being wrong in the intended_as_new_entry fase of NAVIGATION_TYPE_EXISTING_PAGE. It turns out that in some rare race conditions, this section of the code could get reached with a different origin for the committed page and the NavigationEntry that was reused. In that case, make sure to use the SSL certificate from the NavigationHandle instead of resusing the old one. BUG=688425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
jam@chromium.org changed reviewers: + estark@chromium.org
The CQ bit was checked by jam@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for tracking this down. Since a repro can't be reached yet to hit the that section of the code in a browser test, let's separate the addition of the browser test from the fix (which is needed to merge to beta).
lgtm; as mentioned in chat, we think there could be another bug lurking in here somewhere but this is a quick fix for the immediate issue of the wrong cert info showing up. https://codereview.chromium.org/2707133003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2707133003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1138: CHECK_EQ(new_entry->GetURL().GetOrigin(), params.url.GetOrigin()); optional nit: if we think the only way this can happen is a misbehaving renderer, it might be nicer to kill the renderer with ReceivedBadMessage https://codereview.chromium.org/2707133003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1255: CHECK(!is_in_page); ditto about ReceivedBadMessage
https://codereview.chromium.org/2707133003/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2707133003/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:1138: CHECK_EQ(new_entry->GetURL().GetOrigin(), params.url.GetOrigin()); On 2017/02/21 19:39:41, estark (slow thru Feb 22) wrote: > optional nit: if we think the only way this can happen is a misbehaving > renderer, it might be nicer to kill the renderer with ReceivedBadMessage I added this because I don't see a way for this to happen, and none of the crash reports you added are sending this back (although admittedly the volume isn't huge). But I figured since my previous strategy of assuming a cert can be reused from an entry has caused a bunch of bugs, I want to be explicit in CHECKing this assumption everywhere else. If it fires, then we can drill more into whether it's a bug or a bad renderer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2017/02/21 20:22:45, jam (OOO) wrote: > https://codereview.chromium.org/2707133003/diff/1/content/browser/frame_host/... > File content/browser/frame_host/navigation_controller_impl.cc (right): > > https://codereview.chromium.org/2707133003/diff/1/content/browser/frame_host/... > content/browser/frame_host/navigation_controller_impl.cc:1138: > CHECK_EQ(new_entry->GetURL().GetOrigin(), params.url.GetOrigin()); > On 2017/02/21 19:39:41, estark (slow thru Feb 22) wrote: > > optional nit: if we think the only way this can happen is a misbehaving > > renderer, it might be nicer to kill the renderer with ReceivedBadMessage > > I added this because I don't see a way for this to happen, and none of the crash > reports you added are sending this back (although admittedly the volume isn't > huge). But I figured since my previous strategy of assuming a cert can be reused > from an entry has caused a bunch of bugs, I want to be explicit in CHECKing this > assumption everywhere else. If it fires, then we can drill more into whether > it's a bug or a bad renderer. Gotcha, still lgtm.
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2707133003/#ps20001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jam@chromium.org
The CQ bit was checked by jam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2707133003/#ps40001 (title: "update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487779891014670,
"parent_rev": "1c19bd4c6f5128f467954364964288c697e4c306", "commit_rev":
"a78746ec1a1bfa668f5bcb01d2b2665d2c514369"}
Message was sent while issue was closed.
Description was changed from ========== Fix SSL certificate being wrong in the intended_as_new_entry fase of NAVIGATION_TYPE_EXISTING_PAGE. It turns out that in some rare race conditions, this section of the code could get reached with a different origin for the committed page and the NavigationEntry that was reused. In that case, make sure to use the SSL certificate from the NavigationHandle instead of resusing the old one. BUG=688425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix SSL certificate being wrong in the intended_as_new_entry fase of NAVIGATION_TYPE_EXISTING_PAGE. It turns out that in some rare race conditions, this section of the code could get reached with a different origin for the committed page and the NavigationEntry that was reused. In that case, make sure to use the SSL certificate from the NavigationHandle instead of resusing the old one. BUG=688425 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2707133003 Cr-Commit-Position: refs/heads/master@{#452106} Committed: https://chromium.googlesource.com/chromium/src/+/a78746ec1a1bfa668f5bcb01d2b2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/a78746ec1a1bfa668f5bcb01d2b2... |
