Prevent bindings escalation on an existing NavigationEntry.

Prevent bindings escalation on an existing NavigationEntry.

BUG=173672
TEST=See bug for repro steps.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180607

[Charlie Reis, 2013-02-01 18:07:29]

Nasko and Chris, can you take a look?  I'll run it past Brett if it looks good
to you.

[nasko, 2013-02-01 21:52:13]

lgtm

[Charlie Reis, 2013-02-01 22:00:40]

Brett, can you review as well?  I've CC'd you on the bug for context.

I'm holding off on making bindings_ persistent to make this CL easier to merge,
and I'll follow up with a CL to do the rest.

[Chris Evans, 2013-02-01 22:47:47]

Couple of little comments.

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:891:
controller.GetLastCommittedEntry())->bindings());
Is it possible to actually do a history.back() and check the "current state" ?

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_entry_impl.h:98: bindings_ = bindings;
Should this ever be called more than one? Assert?

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_entry_impl.h:99: }
Nit: add newline?

[Charlie Reis, 2013-02-04 06:03:40]

Fixed comments and caught another place that might have escalated bindings. 
PTAL.

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_controller_impl_unittest.cc:891:
controller.GetLastCommittedEntry())->bindings());
On 2013/02/01 22:47:47, Chris Evans wrote:
> Is it possible to actually do a history.back() and check the "current state" ?

In the first patchset, there's only one entry and you can't go back.  I've
changed it to do a RVH swap and then go back, to verify the privileges aren't
leaked.

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
File content/browser/web_contents/navigation_entry_impl.h (right):

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_entry_impl.h:98: bindings_ = bindings;
On 2013/02/01 22:47:47, Chris Evans wrote:
> Should this ever be called more than one? Assert?

It's set every time you commit, but we can ensure that it's always the same
value after the first time.  I changed it to initialize to -1 and ensure that it
always gets set to a real value and stays there.  That should avoid any cases
where an attacker tricks us into escalating the bindings later.

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
content/browser/web_contents/navigation_entry_impl.h:99: }
On 2013/02/01 22:47:47, Chris Evans wrote:
> Nit: add newline?

This is consistent with the rest of the file.

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
content/browser/web_contents/render_view_host_manager.cc:897:
delegate_->CreateWebUIForRenderManager(entry.GetURL()));
Turns out we missed this case in the first patchset.  I've added SetPendingWebUI
to check uniformly.

[Chris Evans, 2013-02-04 19:13:33]

On 2013/02/04 06:03:40, creis wrote:
> Fixed comments and caught another place that might have escalated bindings. 
> PTAL.
> 
>
https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/navigation_controller_impl_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:891:
> controller.GetLastCommittedEntry())->bindings());
> On 2013/02/01 22:47:47, Chris Evans wrote:
> > Is it possible to actually do a history.back() and check the "current state"
?
> 
> In the first patchset, there's only one entry and you can't go back.  I've
> changed it to do a RVH swap and then go back, to verify the privileges aren't
> leaked.
> 
>
https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/navigation_entry_impl.h (right):
> 
>
https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
> content/browser/web_contents/navigation_entry_impl.h:98: bindings_ = bindings;
> On 2013/02/01 22:47:47, Chris Evans wrote:
> > Should this ever be called more than one? Assert?
> 
> It's set every time you commit, but we can ensure that it's always the same
> value after the first time.  I changed it to initialize to -1 and ensure that
it
> always gets set to a real value and stays there.  That should avoid any cases
> where an attacker tricks us into escalating the bindings later.
> 
>
https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
> content/browser/web_contents/navigation_entry_impl.h:99: }
> On 2013/02/01 22:47:47, Chris Evans wrote:
> > Nit: add newline?
> 
> This is consistent with the rest of the file.
> 
>
https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/render_view_host_manager.cc (right):
> 
>
https://codereview.chromium.org/12086109/diff/1/content/browser/web_contents/...
> content/browser/web_contents/render_view_host_manager.cc:897:
> delegate_->CreateWebUIForRenderManager(entry.GetURL()));
> Turns out we missed this case in the first patchset.  I've added
SetPendingWebUI
> to check uniformly.

LGTM

[Charlie Reis, 2013-02-04 19:15:23]

Thanks.  Darin, can you take a look for sanity check?  Brett's on vacation, so
I'm moving him to CC.

[darin (slow to review), 2013-02-05 00:24:26]

LGTM

[commit-bot: I haz the power, 2013-02-05 00:53:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/12086109/17001

[commit-bot: I haz the power, 2013-02-05 02:11:20]

Change committed as 180607