Allow Signin page to open other chrome:// URLs if login content in <webview>

Allow Signin page to open other chrome:// URLs if login content in <webview>

BUG=450125
Committed: https://crrev.com/9626273c576eaea50bcd2ccfa6b372a091f51409
Cr-Commit-Position: refs/heads/master@{#317209}

[Fady Samuel, 2015-01-31 22:32:47]

fsamuel@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2015-02-02 17:29:21]

Yes, this check was in there for the untrusted-content-in-process case.  I'm
very happy to be getting rid of that case, so this seems reasonable.

I don't quite understand how this fixes the bug, though, or how that feature
ever worked.  How can a non-chrome:// renderer process do a window.open to a
chrome:// URL (e.g., when choosing Language Settings from a form input on a web
page)?  Is that not what's normally happening?

https://codereview.chromium.org/890183002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/890183002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1024: // the signin page may
host untrusted web content unless <webview>-based
This comment is a bit hard to follow.  Maybe:

...since the signin page may host untrusted web content when <webview>-based
signin is not enabled.  In the <webview> case, untrusted content is
out-of-process and cannot navigate to chrome:// URLs.

[Fady Samuel, 2015-02-02 19:59:07]

So when the content in <webview> wants to create a new window, the signin page
WebUI discards that new window, and calls window.open from its context instead.
Thus, it was actually the WebUI trying to request the languages page, not the
<webview> content.

https://codereview.chromium.org/890183002/diff/1/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/890183002/diff/1/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1024: // the signin page may
host untrusted web content unless <webview>-based
On 2015/02/02 17:29:21, Charlie Reis wrote:
> This comment is a bit hard to follow.  Maybe:
> 
> ...since the signin page may host untrusted web content when <webview>-based
> signin is not enabled.  In the <webview> case, untrusted content is
> out-of-process and cannot navigate to chrome:// URLs.

Done.

[Fady Samuel, 2015-02-02 19:59:15]

PTAL

[Charlie Reis, 2015-02-02 20:23:55]

On 2015/02/02 19:59:07, Fady Samuel wrote:
> So when the content in <webview> wants to create a new window, the signin page
> WebUI discards that new window, and calls window.open from its context
instead.
> Thus, it was actually the WebUI trying to request the languages page, not the
> <webview> content.

Thanks-- that partly explains the regression with <webview>: whatever approach
we normally use was blocked and window.open was attempted (unsuccessfully)
instead.

I was mainly asking how the feature itself works on normal web pages, though. 
How is a normal renderer process allowed to pop open a tab to a chrome:// URL? 
It seems like we're using that path for both normal tabs and <webview> now, so
I'd like to understand how it's allowed.

[Fady Samuel, 2015-02-02 20:41:49]

On 2015/02/02 20:23:55, Charlie Reis wrote:
> On 2015/02/02 19:59:07, Fady Samuel wrote:
> > So when the content in <webview> wants to create a new window, the signin
page
> > WebUI discards that new window, and calls window.open from its context
> instead.
> > Thus, it was actually the WebUI trying to request the languages page, not
the
> > <webview> content.
> 
> Thanks-- that partly explains the regression with <webview>: whatever approach
> we normally use was blocked and window.open was attempted (unsuccessfully)
> instead.
> 
> I was mainly asking how the feature itself works on normal web pages, though. 
> How is a normal renderer process allowed to pop open a tab to a chrome:// URL?

> It seems like we're using that path for both normal tabs and <webview> now, so
> I'd like to understand how it's allowed.

I just tried window.open('chrome://settings/languages') from google.com with my
patch applied. It still opens about:blank. I'm not sure where that happens
though. I can dig more into that if you're concerned.

[Charlie Reis, 2015-02-02 21:10:57]

On 2015/02/02 20:41:49, Fady Samuel wrote:
> On 2015/02/02 20:23:55, Charlie Reis wrote:
> > On 2015/02/02 19:59:07, Fady Samuel wrote:
> > > So when the content in <webview> wants to create a new window, the signin
> page
> > > WebUI discards that new window, and calls window.open from its context
> > instead.
> > > Thus, it was actually the WebUI trying to request the languages page, not
> the
> > > <webview> content.
> > 
> > Thanks-- that partly explains the regression with <webview>: whatever
approach
> > we normally use was blocked and window.open was attempted (unsuccessfully)
> > instead.
> > 
> > I was mainly asking how the feature itself works on normal web pages,
though. 
> > How is a normal renderer process allowed to pop open a tab to a chrome://
URL?
> 
> > It seems like we're using that path for both normal tabs and <webview> now,
so
> > I'd like to understand how it's allowed.
> 
> I just tried window.open('chrome://settings/languages') from http://google.com
with my
> patch applied. It still opens about:blank. I'm not sure where that happens
> though. I can dig more into that if you're concerned.

No, I'm asking how the "Language settings.." menu works.

[Fady Samuel, 2015-02-02 23:39:43]

On 2015/02/02 21:10:57, Charlie Reis wrote:
> On 2015/02/02 20:41:49, Fady Samuel wrote:
> > On 2015/02/02 20:23:55, Charlie Reis wrote:
> > > On 2015/02/02 19:59:07, Fady Samuel wrote:
> > > > So when the content in <webview> wants to create a new window, the
signin
> > page
> > > > WebUI discards that new window, and calls window.open from its context
> > > instead.
> > > > Thus, it was actually the WebUI trying to request the languages page,
not
> > the
> > > > <webview> content.
> > > 
> > > Thanks-- that partly explains the regression with <webview>: whatever
> approach
> > > we normally use was blocked and window.open was attempted (unsuccessfully)
> > > instead.
> > > 
> > > I was mainly asking how the feature itself works on normal web pages,
> though. 
> > > How is a normal renderer process allowed to pop open a tab to a chrome://
> URL?
> > 
> > > It seems like we're using that path for both normal tabs and <webview>
now,
> so
> > > I'd like to understand how it's allowed.
> > 
> > I just tried window.open('chrome://settings/languages') from
http://google.com
> with my
> > patch applied. It still opens about:blank. I'm not sure where that happens
> > though. I can dig more into that if you're concerned.
> 
> No, I'm asking how the "Language settings.." menu works.

#7 0x7f7c02b4cbcb extensions::GuestViewBase::CompleteInit()
#8 0x7f7c02b579c1 base::internal::RunnableAdapter<>::Run()
#9 0x7f7c02b5785f base::internal::InvokeHelper<>::MakeItSo()
#10 0x7f7c02b5777e base::internal::Invoker<>::Run()
#11 0x7f7c0088ca76 base::Callback<>::Run()
#12 0x7f7c02b6752c extensions::WebViewGuest::CreateWebContents()
#13 0x7f7c02b4ca8f extensions::GuestViewBase::Init()
#14 0x7f7c02b58f86 extensions::GuestViewManager::CreateGuest()
#15 0x7f7c02b69ca8 extensions::WebViewGuest::CreateNewGuestWebViewWindow()
#16 0x7f7c02b6e120 extensions::WebViewGuest::OpenURLFromTab()
#17 0x7f7c02b6e197 extensions::WebViewGuest::OpenURLFromTab()
#18 0x7f7bfb74c076 content::WebContentsImpl::OpenURL()
#19 0x7f7c03746a02 RenderViewContextMenuBase::OpenURL()
#20 0x7f7c03d56441 RenderViewContextMenu::ExecuteCommand()
#21 0x7f7c0230b490 RenderViewContextMenuViews::ExecuteCommand()
#22 0x7f7bf3a67f00 ui::SimpleMenuModel::ActivatedAt()
#23 0x7f7bfe5673c5 views::MenuModelAdapter::ExecuteCommand()
#24 0x7f7bfe56bba9 views::internal::MenuRunnerImpl::MenuDone()
#25 0x7f7bfe56b9f3 views::internal::MenuRunnerImpl::RunMenuAt()
#26 0x7f7bfe56ad3e views::MenuRunner::RunMenuAt()
#27 0x7f7c0374b53e ToolkitDelegateViews::RunMenuAt()
#28 0x7f7c0230ac96 RenderViewContextMenuViews::RunMenuAt()
#29 0x7f7c0230b8d3 RenderViewContextMenuViews::Show()
#30 0x7f7c01f2d3f0 ChromeWebContentsViewDelegateViews::ShowMenu()
#31 0x7f7c01f2d41c ChromeWebContentsViewDelegateViews::ShowMenu()
#32 0x7f7c03cd9f16 extensions::ChromeWebViewGuestDelegate::OnShowContextMenu()
#33 0x7f7c02b6d743 extensions::WebViewGuest::ShowContextMenu()

It looks like the plumbing happens through the WebContentsDelegate. The
WebContentsDelegate of a guest is WebViewGuest.

[Charlie Reis, 2015-02-03 00:14:09]

On 2015/02/02 23:39:43, Fady Samuel wrote:
> On 2015/02/02 21:10:57, Charlie Reis wrote:
> > On 2015/02/02 20:41:49, Fady Samuel wrote:
> > > On 2015/02/02 20:23:55, Charlie Reis wrote:
> > > > On 2015/02/02 19:59:07, Fady Samuel wrote:
> > > > > So when the content in <webview> wants to create a new window, the
> signin
> > > page
> > > > > WebUI discards that new window, and calls window.open from its context
> > > > instead.
> > > > > Thus, it was actually the WebUI trying to request the languages page,
> not
> > > the
> > > > > <webview> content.
> > > > 
> > > > Thanks-- that partly explains the regression with <webview>: whatever
> > approach
> > > > we normally use was blocked and window.open was attempted
(unsuccessfully)
> > > > instead.
> > > > 
> > > > I was mainly asking how the feature itself works on normal web pages,
> > though. 
> > > > How is a normal renderer process allowed to pop open a tab to a
chrome://
> > URL?
> > > 
> > > > It seems like we're using that path for both normal tabs and <webview>
> now,
> > so
> > > > I'd like to understand how it's allowed.
> > > 
> > > I just tried window.open('chrome://settings/languages') from
> http://google.com
> > with my
> > > patch applied. It still opens about:blank. I'm not sure where that happens
> > > though. I can dig more into that if you're concerned.
> > 
> > No, I'm asking how the "Language settings.." menu works.
> 
> #7 0x7f7c02b4cbcb extensions::GuestViewBase::CompleteInit()
> #8 0x7f7c02b579c1 base::internal::RunnableAdapter<>::Run()
> #9 0x7f7c02b5785f base::internal::InvokeHelper<>::MakeItSo()
> #10 0x7f7c02b5777e base::internal::Invoker<>::Run()
> #11 0x7f7c0088ca76 base::Callback<>::Run()
> #12 0x7f7c02b6752c extensions::WebViewGuest::CreateWebContents()
> #13 0x7f7c02b4ca8f extensions::GuestViewBase::Init()
> #14 0x7f7c02b58f86 extensions::GuestViewManager::CreateGuest()
> #15 0x7f7c02b69ca8 extensions::WebViewGuest::CreateNewGuestWebViewWindow()
> #16 0x7f7c02b6e120 extensions::WebViewGuest::OpenURLFromTab()
> #17 0x7f7c02b6e197 extensions::WebViewGuest::OpenURLFromTab()
> #18 0x7f7bfb74c076 content::WebContentsImpl::OpenURL()
> #19 0x7f7c03746a02 RenderViewContextMenuBase::OpenURL()
> #20 0x7f7c03d56441 RenderViewContextMenu::ExecuteCommand()
> #21 0x7f7c0230b490 RenderViewContextMenuViews::ExecuteCommand()
> #22 0x7f7bf3a67f00 ui::SimpleMenuModel::ActivatedAt()
> #23 0x7f7bfe5673c5 views::MenuModelAdapter::ExecuteCommand()
> #24 0x7f7bfe56bba9 views::internal::MenuRunnerImpl::MenuDone()
> #25 0x7f7bfe56b9f3 views::internal::MenuRunnerImpl::RunMenuAt()
> #26 0x7f7bfe56ad3e views::MenuRunner::RunMenuAt()
> #27 0x7f7c0374b53e ToolkitDelegateViews::RunMenuAt()
> #28 0x7f7c0230ac96 RenderViewContextMenuViews::RunMenuAt()
> #29 0x7f7c0230b8d3 RenderViewContextMenuViews::Show()
> #30 0x7f7c01f2d3f0 ChromeWebContentsViewDelegateViews::ShowMenu()
> #31 0x7f7c01f2d41c ChromeWebContentsViewDelegateViews::ShowMenu()
> #32 0x7f7c03cd9f16 extensions::ChromeWebViewGuestDelegate::OnShowContextMenu()
> #33 0x7f7c02b6d743 extensions::WebViewGuest::ShowContextMenu()
> 
> It looks like the plumbing happens through the WebContentsDelegate. The
> WebContentsDelegate of a guest is WebViewGuest.

Thanks for digging that up.

It's sounding to me like this is the wrong change to make.  We have to assume
that there's some untrusted web content inside the <webview>.  If I understand
this correctly, attempts to open a new window from within the guest (e.g.,
target="foo", window.open, or otherwise) get mapped to window.open within the
WebUI embedder page.  If that's correct, then your change will allow the
untrusted content to open chrome:// URLs.

In contrast, the Language Settings menu is attempting to use
WebContentsImpl::OpenURL.  The bug seems like we're mapping that onto
window.open in the embedder, rather than sending it straight to the embedder's
WebContentsImpl::OpenURL.  If we skipped the window.open part, it would never
look like the request came from web content.

Does that sound right?  (Sorry for being picky here, but this is a very
important security boundary to get right.)

[Fady Samuel, 2015-02-03 00:17:43]

On 2015/02/03 00:14:09, Charlie Reis wrote:
> On 2015/02/02 23:39:43, Fady Samuel wrote:
> > On 2015/02/02 21:10:57, Charlie Reis wrote:
> > > On 2015/02/02 20:41:49, Fady Samuel wrote:
> > > > On 2015/02/02 20:23:55, Charlie Reis wrote:
> > > > > On 2015/02/02 19:59:07, Fady Samuel wrote:
> > > > > > So when the content in <webview> wants to create a new window, the
> > signin
> > > > page
> > > > > > WebUI discards that new window, and calls window.open from its
context
> > > > > instead.
> > > > > > Thus, it was actually the WebUI trying to request the languages
page,
> > not
> > > > the
> > > > > > <webview> content.
> > > > > 
> > > > > Thanks-- that partly explains the regression with <webview>: whatever
> > > approach
> > > > > we normally use was blocked and window.open was attempted
> (unsuccessfully)
> > > > > instead.
> > > > > 
> > > > > I was mainly asking how the feature itself works on normal web pages,
> > > though. 
> > > > > How is a normal renderer process allowed to pop open a tab to a
> chrome://
> > > URL?
> > > > 
> > > > > It seems like we're using that path for both normal tabs and <webview>
> > now,
> > > so
> > > > > I'd like to understand how it's allowed.
> > > > 
> > > > I just tried window.open('chrome://settings/languages') from
> > http://google.com
> > > with my
> > > > patch applied. It still opens about:blank. I'm not sure where that
happens
> > > > though. I can dig more into that if you're concerned.
> > > 
> > > No, I'm asking how the "Language settings.." menu works.
> > 
> > #7 0x7f7c02b4cbcb extensions::GuestViewBase::CompleteInit()
> > #8 0x7f7c02b579c1 base::internal::RunnableAdapter<>::Run()
> > #9 0x7f7c02b5785f base::internal::InvokeHelper<>::MakeItSo()
> > #10 0x7f7c02b5777e base::internal::Invoker<>::Run()
> > #11 0x7f7c0088ca76 base::Callback<>::Run()
> > #12 0x7f7c02b6752c extensions::WebViewGuest::CreateWebContents()
> > #13 0x7f7c02b4ca8f extensions::GuestViewBase::Init()
> > #14 0x7f7c02b58f86 extensions::GuestViewManager::CreateGuest()
> > #15 0x7f7c02b69ca8 extensions::WebViewGuest::CreateNewGuestWebViewWindow()
> > #16 0x7f7c02b6e120 extensions::WebViewGuest::OpenURLFromTab()
> > #17 0x7f7c02b6e197 extensions::WebViewGuest::OpenURLFromTab()
> > #18 0x7f7bfb74c076 content::WebContentsImpl::OpenURL()
> > #19 0x7f7c03746a02 RenderViewContextMenuBase::OpenURL()
> > #20 0x7f7c03d56441 RenderViewContextMenu::ExecuteCommand()
> > #21 0x7f7c0230b490 RenderViewContextMenuViews::ExecuteCommand()
> > #22 0x7f7bf3a67f00 ui::SimpleMenuModel::ActivatedAt()
> > #23 0x7f7bfe5673c5 views::MenuModelAdapter::ExecuteCommand()
> > #24 0x7f7bfe56bba9 views::internal::MenuRunnerImpl::MenuDone()
> > #25 0x7f7bfe56b9f3 views::internal::MenuRunnerImpl::RunMenuAt()
> > #26 0x7f7bfe56ad3e views::MenuRunner::RunMenuAt()
> > #27 0x7f7c0374b53e ToolkitDelegateViews::RunMenuAt()
> > #28 0x7f7c0230ac96 RenderViewContextMenuViews::RunMenuAt()
> > #29 0x7f7c0230b8d3 RenderViewContextMenuViews::Show()
> > #30 0x7f7c01f2d3f0 ChromeWebContentsViewDelegateViews::ShowMenu()
> > #31 0x7f7c01f2d41c ChromeWebContentsViewDelegateViews::ShowMenu()
> > #32 0x7f7c03cd9f16
extensions::ChromeWebViewGuestDelegate::OnShowContextMenu()
> > #33 0x7f7c02b6d743 extensions::WebViewGuest::ShowContextMenu()
> > 
> > It looks like the plumbing happens through the WebContentsDelegate. The
> > WebContentsDelegate of a guest is WebViewGuest.
> 
> Thanks for digging that up.
> 
> It's sounding to me like this is the wrong change to make.  We have to assume
> that there's some untrusted web content inside the <webview>.  If I understand
> this correctly, attempts to open a new window from within the guest (e.g.,
> target="foo", window.open, or otherwise) get mapped to window.open within the
> WebUI embedder page.  If that's correct, then your change will allow the
> untrusted content to open chrome:// URLs.
> 
> In contrast, the Language Settings menu is attempting to use
> WebContentsImpl::OpenURL.  The bug seems like we're mapping that onto
> window.open in the embedder, rather than sending it straight to the embedder's
> WebContentsImpl::OpenURL.  If we skipped the window.open part, it would never
> look like the request came from web content.
> 
> Does that sound right?  (Sorry for being picky here, but this is a very
> important security boundary to get right.)



Not quite, this doesn't map directly to the WebUI's window.open. The signin page
listens for the newwindow event:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

It discards the new window from the <webview> and then it calls window.open on
behalf of the webview's guest content. See the link above.

[Charlie Reis, 2015-02-03 00:33:17]

On 2015/02/03 00:17:43, Fady Samuel wrote:
> On 2015/02/03 00:14:09, Charlie Reis wrote:
> > On 2015/02/02 23:39:43, Fady Samuel wrote:
> > > On 2015/02/02 21:10:57, Charlie Reis wrote:
> > > > On 2015/02/02 20:41:49, Fady Samuel wrote:
> > > > > On 2015/02/02 20:23:55, Charlie Reis wrote:
> > > > > > On 2015/02/02 19:59:07, Fady Samuel wrote:
> > > > > > > So when the content in <webview> wants to create a new window, the
> > > signin
> > > > > page
> > > > > > > WebUI discards that new window, and calls window.open from its
> context
> > > > > > instead.
> > > > > > > Thus, it was actually the WebUI trying to request the languages
> page,
> > > not
> > > > > the
> > > > > > > <webview> content.
> > > > > > 
> > > > > > Thanks-- that partly explains the regression with <webview>:
whatever
> > > > approach
> > > > > > we normally use was blocked and window.open was attempted
> > (unsuccessfully)
> > > > > > instead.
> > > > > > 
> > > > > > I was mainly asking how the feature itself works on normal web
pages,
> > > > though. 
> > > > > > How is a normal renderer process allowed to pop open a tab to a
> > chrome://
> > > > URL?
> > > > > 
> > > > > > It seems like we're using that path for both normal tabs and
<webview>
> > > now,
> > > > so
> > > > > > I'd like to understand how it's allowed.
> > > > > 
> > > > > I just tried window.open('chrome://settings/languages') from
> > > http://google.com
> > > > with my
> > > > > patch applied. It still opens about:blank. I'm not sure where that
> happens
> > > > > though. I can dig more into that if you're concerned.
> > > > 
> > > > No, I'm asking how the "Language settings.." menu works.
> > > 
> > > #7 0x7f7c02b4cbcb extensions::GuestViewBase::CompleteInit()
> > > #8 0x7f7c02b579c1 base::internal::RunnableAdapter<>::Run()
> > > #9 0x7f7c02b5785f base::internal::InvokeHelper<>::MakeItSo()
> > > #10 0x7f7c02b5777e base::internal::Invoker<>::Run()
> > > #11 0x7f7c0088ca76 base::Callback<>::Run()
> > > #12 0x7f7c02b6752c extensions::WebViewGuest::CreateWebContents()
> > > #13 0x7f7c02b4ca8f extensions::GuestViewBase::Init()
> > > #14 0x7f7c02b58f86 extensions::GuestViewManager::CreateGuest()
> > > #15 0x7f7c02b69ca8 extensions::WebViewGuest::CreateNewGuestWebViewWindow()
> > > #16 0x7f7c02b6e120 extensions::WebViewGuest::OpenURLFromTab()
> > > #17 0x7f7c02b6e197 extensions::WebViewGuest::OpenURLFromTab()
> > > #18 0x7f7bfb74c076 content::WebContentsImpl::OpenURL()
> > > #19 0x7f7c03746a02 RenderViewContextMenuBase::OpenURL()
> > > #20 0x7f7c03d56441 RenderViewContextMenu::ExecuteCommand()
> > > #21 0x7f7c0230b490 RenderViewContextMenuViews::ExecuteCommand()
> > > #22 0x7f7bf3a67f00 ui::SimpleMenuModel::ActivatedAt()
> > > #23 0x7f7bfe5673c5 views::MenuModelAdapter::ExecuteCommand()
> > > #24 0x7f7bfe56bba9 views::internal::MenuRunnerImpl::MenuDone()
> > > #25 0x7f7bfe56b9f3 views::internal::MenuRunnerImpl::RunMenuAt()
> > > #26 0x7f7bfe56ad3e views::MenuRunner::RunMenuAt()
> > > #27 0x7f7c0374b53e ToolkitDelegateViews::RunMenuAt()
> > > #28 0x7f7c0230ac96 RenderViewContextMenuViews::RunMenuAt()
> > > #29 0x7f7c0230b8d3 RenderViewContextMenuViews::Show()
> > > #30 0x7f7c01f2d3f0 ChromeWebContentsViewDelegateViews::ShowMenu()
> > > #31 0x7f7c01f2d41c ChromeWebContentsViewDelegateViews::ShowMenu()
> > > #32 0x7f7c03cd9f16
> extensions::ChromeWebViewGuestDelegate::OnShowContextMenu()
> > > #33 0x7f7c02b6d743 extensions::WebViewGuest::ShowContextMenu()
> > > 
> > > It looks like the plumbing happens through the WebContentsDelegate. The
> > > WebContentsDelegate of a guest is WebViewGuest.
> > 
> > Thanks for digging that up.
> > 
> > It's sounding to me like this is the wrong change to make.  We have to
assume
> > that there's some untrusted web content inside the <webview>.  If I
understand
> > this correctly, attempts to open a new window from within the guest (e.g.,
> > target="foo", window.open, or otherwise) get mapped to window.open within
the
> > WebUI embedder page.  If that's correct, then your change will allow the
> > untrusted content to open chrome:// URLs.
> > 
> > In contrast, the Language Settings menu is attempting to use
> > WebContentsImpl::OpenURL.  The bug seems like we're mapping that onto
> > window.open in the embedder, rather than sending it straight to the
embedder's
> > WebContentsImpl::OpenURL.  If we skipped the window.open part, it would
never
> > look like the request came from web content.
> > 
> > Does that sound right?  (Sorry for being picky here, but this is a very
> > important security boundary to get right.)
> 
> 
> 
> Not quite, this doesn't map directly to the WebUI's window.open. The signin
page
> listens for the newwindow event:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> It discards the new window from the <webview> and then it calls window.open on
> behalf of the webview's guest content. See the link above.

That's exactly the problem I was referring to.  If a web page within the
<webview> can trigger the newwindow event in any way, then we have a security
hole:
1) Load an untrusted page into the <webview>.  (Possible since HTTP content is
allowed there.)
2) Attacker does window.open("chrome://foo").
3) The code you mention does a window.open("chrome://foo") in the WebUI embedder
instead.
4) The change in this CL would allow the navigation.

It seems to me that the embedder page should be sending the Language Settings
request to WebContents::OpenURL, rather than window.open.  Then we could keep
window.open locked down (possibly more than it is today).

[Fady Samuel, 2015-02-04 08:37:50]

On 2015/02/03 00:33:17, Charlie Reis wrote:
> On 2015/02/03 00:17:43, Fady Samuel wrote:
> > On 2015/02/03 00:14:09, Charlie Reis wrote:
> > > On 2015/02/02 23:39:43, Fady Samuel wrote:
> > > > On 2015/02/02 21:10:57, Charlie Reis wrote:
> > > > > On 2015/02/02 20:41:49, Fady Samuel wrote:
> > > > > > On 2015/02/02 20:23:55, Charlie Reis wrote:
> > > > > > > On 2015/02/02 19:59:07, Fady Samuel wrote:
> > > > > > > > So when the content in <webview> wants to create a new window,
the
> > > > signin
> > > > > > page
> > > > > > > > WebUI discards that new window, and calls window.open from its
> > context
> > > > > > > instead.
> > > > > > > > Thus, it was actually the WebUI trying to request the languages
> > page,
> > > > not
> > > > > > the
> > > > > > > > <webview> content.
> > > > > > > 
> > > > > > > Thanks-- that partly explains the regression with <webview>:
> whatever
> > > > > approach
> > > > > > > we normally use was blocked and window.open was attempted
> > > (unsuccessfully)
> > > > > > > instead.
> > > > > > > 
> > > > > > > I was mainly asking how the feature itself works on normal web
> pages,
> > > > > though. 
> > > > > > > How is a normal renderer process allowed to pop open a tab to a
> > > chrome://
> > > > > URL?
> > > > > > 
> > > > > > > It seems like we're using that path for both normal tabs and
> <webview>
> > > > now,
> > > > > so
> > > > > > > I'd like to understand how it's allowed.
> > > > > > 
> > > > > > I just tried window.open('chrome://settings/languages') from
> > > > http://google.com
> > > > > with my
> > > > > > patch applied. It still opens about:blank. I'm not sure where that
> > happens
> > > > > > though. I can dig more into that if you're concerned.
> > > > > 
> > > > > No, I'm asking how the "Language settings.." menu works.
> > > > 
> > > > #7 0x7f7c02b4cbcb extensions::GuestViewBase::CompleteInit()
> > > > #8 0x7f7c02b579c1 base::internal::RunnableAdapter<>::Run()
> > > > #9 0x7f7c02b5785f base::internal::InvokeHelper<>::MakeItSo()
> > > > #10 0x7f7c02b5777e base::internal::Invoker<>::Run()
> > > > #11 0x7f7c0088ca76 base::Callback<>::Run()
> > > > #12 0x7f7c02b6752c extensions::WebViewGuest::CreateWebContents()
> > > > #13 0x7f7c02b4ca8f extensions::GuestViewBase::Init()
> > > > #14 0x7f7c02b58f86 extensions::GuestViewManager::CreateGuest()
> > > > #15 0x7f7c02b69ca8
extensions::WebViewGuest::CreateNewGuestWebViewWindow()
> > > > #16 0x7f7c02b6e120 extensions::WebViewGuest::OpenURLFromTab()
> > > > #17 0x7f7c02b6e197 extensions::WebViewGuest::OpenURLFromTab()
> > > > #18 0x7f7bfb74c076 content::WebContentsImpl::OpenURL()
> > > > #19 0x7f7c03746a02 RenderViewContextMenuBase::OpenURL()
> > > > #20 0x7f7c03d56441 RenderViewContextMenu::ExecuteCommand()
> > > > #21 0x7f7c0230b490 RenderViewContextMenuViews::ExecuteCommand()
> > > > #22 0x7f7bf3a67f00 ui::SimpleMenuModel::ActivatedAt()
> > > > #23 0x7f7bfe5673c5 views::MenuModelAdapter::ExecuteCommand()
> > > > #24 0x7f7bfe56bba9 views::internal::MenuRunnerImpl::MenuDone()
> > > > #25 0x7f7bfe56b9f3 views::internal::MenuRunnerImpl::RunMenuAt()
> > > > #26 0x7f7bfe56ad3e views::MenuRunner::RunMenuAt()
> > > > #27 0x7f7c0374b53e ToolkitDelegateViews::RunMenuAt()
> > > > #28 0x7f7c0230ac96 RenderViewContextMenuViews::RunMenuAt()
> > > > #29 0x7f7c0230b8d3 RenderViewContextMenuViews::Show()
> > > > #30 0x7f7c01f2d3f0 ChromeWebContentsViewDelegateViews::ShowMenu()
> > > > #31 0x7f7c01f2d41c ChromeWebContentsViewDelegateViews::ShowMenu()
> > > > #32 0x7f7c03cd9f16
> > extensions::ChromeWebViewGuestDelegate::OnShowContextMenu()
> > > > #33 0x7f7c02b6d743 extensions::WebViewGuest::ShowContextMenu()
> > > > 
> > > > It looks like the plumbing happens through the WebContentsDelegate. The
> > > > WebContentsDelegate of a guest is WebViewGuest.
> > > 
> > > Thanks for digging that up.
> > > 
> > > It's sounding to me like this is the wrong change to make.  We have to
> assume
> > > that there's some untrusted web content inside the <webview>.  If I
> understand
> > > this correctly, attempts to open a new window from within the guest (e.g.,
> > > target="foo", window.open, or otherwise) get mapped to window.open within
> the
> > > WebUI embedder page.  If that's correct, then your change will allow the
> > > untrusted content to open chrome:// URLs.
> > > 
> > > In contrast, the Language Settings menu is attempting to use
> > > WebContentsImpl::OpenURL.  The bug seems like we're mapping that onto
> > > window.open in the embedder, rather than sending it straight to the
> embedder's
> > > WebContentsImpl::OpenURL.  If we skipped the window.open part, it would
> never
> > > look like the request came from web content.
> > > 
> > > Does that sound right?  (Sorry for being picky here, but this is a very
> > > important security boundary to get right.)
> > 
> > 
> > 
> > Not quite, this doesn't map directly to the WebUI's window.open. The signin
> page
> > listens for the newwindow event:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > 
> > It discards the new window from the <webview> and then it calls window.open
on
> > behalf of the webview's guest content. See the link above.
> 
> That's exactly the problem I was referring to.  If a web page within the
> <webview> can trigger the newwindow event in any way, then we have a security
> hole:
> 1) Load an untrusted page into the <webview>.  (Possible since HTTP content is
> allowed there.)
> 2) Attacker does window.open("chrome://foo").
> 3) The code you mention does a window.open("chrome://foo") in the WebUI
embedder
> instead.
> 4) The change in this CL would allow the navigation.
> 
> It seems to me that the embedder page should be sending the Language Settings
> request to WebContents::OpenURL, rather than window.open.  Then we could keep
> window.open locked down (possibly more than it is today).

The breaks the ctrl+click use case in <webview>. I'd like to be able to
Ctrl+Click a link in a <webview> and embed that new thing in another <webview>

The functionality you're suggesting would send ctrl+click to the browser
instead. I prefer keeping the existing behavior for Chrome Apps, but doing what
you suggest for WebUI. Thoughts?

[Fady Samuel, 2015-02-04 08:56:52]

PTAL Charlie. How does this middle ground look to you? In Chrome Apps, we still
plumb this out as a newwindow event. Outside of Chrome Apps, this OpenURLFromTab
goes to the embedder.

[Charlie Reis, 2015-02-04 22:02:57]

This doesn't make sense to me yet.  Let's look more closely at the cases:

1) Renderer-initiated nav to chrome:// inside signin webview.
(This includes link clicks, ctrl+click, window.open, etc.)
Must be blocked.
Is this blocked in patch 4?  It goes through the owner's OpenURLFromTab, and I'm
not sure whether it is blocked after that.  It probably looks like it's coming
from the WebUI embedder, which means it might be allowed.

2) Language Settings inside signin webview.
Should work.
Works in patch 4, since it goes through the owner's OpenURLFromTab.

3) Renderer-initiated nav to chrome:// inside app webview.
Must be blocked.  (Webviews in apps should not be able to open chrome:// URLs.)
I *think* this is blocked in WebViewGuest::NavigateGuest, right?

4) Language Settings inside app webview.
I think this should work?  (Do apps have context menus with Language Settings?)
It's not clear to me whether this will work or not, since it goes through
WebViewGuest::NavigateGuest.  Thus it may get blocked.


So, it sounds like (1) may be wrong (insecure), and (4) may be broken.

Alternative: Instead of distinguishing between apps and other cases, would it
make sense to check OpenURLParams::is_renderer_initiated?  The Language Settings
context menu nav should look like a browser-initiated navigation in the guest
WebContents, but any navigations from content inside the webview would be
renderer-initiated.

https://codereview.chromium.org/890183002/diff/60001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/890183002/diff/60001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1059: if
(!switches::IsEnableWebviewBasedSignin() &&
I'm still hesitant to change this.  It sounds like it's not necessary for fixing
the bug (since Language Settings will go straight to OpenURLFromTab and not
through here), while inline_login.js continues to route any newwindow events
from the webview (if any) through here.

Why do we need to skip the check?  And do we still need onNewWindow in
inline_login.js?

[Fady Samuel, 2015-02-04 23:57:32]

Hi Charlie,

PTAL. I believe I've addressed your comments. We still need to listen to the
newwindow event in WebUI because there are links on the signin page.

Thanks,

Fady

[Charlie Reis, 2015-02-05 21:01:33]

Yes, I think this will work.  Thanks for working through it!

Quick sanity check before I approve: is there a way to test either of these
paths?  Ideally we'd have a sign-in test that tried to open a chrome:// URL from
inside the webview (which would have failed earlier patchsets), and a simple
test "simulating" Language Settings with any browser-initiated nav in the
guest's WebContents.

[Fady Samuel, 2015-02-18 22:19:56]

PTAL. I added a test that verifies that the "Language Settings" menu item goes
to the embedder's WebContentsDelegate.

[Charlie Reis, 2015-02-18 22:32:06]

On 2015/02/18 22:19:56, Fady Samuel wrote:
> PTAL. I added a test that verifies that the "Language Settings" menu item goes
> to the embedder's WebContentsDelegate.

Great!  That covers the functional side (case 2 in your comment).  What about
the security side (case 1 in your comment)?  That seems like a more important
test to me, especially since I haven't been able to connect the dots to see how
it works.

https://codereview.chromium.org/890183002/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/890183002/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:1124: //    
WebViewGuest::NavigateGuest.
nit: It's not obvious how we get to NavigateGuest.  Can you mention the line(s)
below that goes there?  

(Is it LoadURLWithParams?  CreateNewGuestWebViewWindow?  Both?  I don't see it
in either, so I'm concerned that we're not actually covered.)

[Fady Samuel, 2015-02-19 00:22:59]

On 2015/02/18 22:32:06, Charlie Reis wrote:
> On 2015/02/18 22:19:56, Fady Samuel wrote:
> > PTAL. I added a test that verifies that the "Language Settings" menu item
goes
> > to the embedder's WebContentsDelegate.
> 
> Great!  That covers the functional side (case 2 in your comment).  What about
> the security side (case 1 in your comment)?  That seems like a more important
> test to me, especially since I haven't been able to connect the dots to see
how
> it works.
> 
>
https://codereview.chromium.org/890183002/diff/100001/extensions/browser/gues...
> File extensions/browser/guest_view/web_view/web_view_guest.cc (right):
> 
>
https://codereview.chromium.org/890183002/diff/100001/extensions/browser/gues...
> extensions/browser/guest_view/web_view/web_view_guest.cc:1124: //    
> WebViewGuest::NavigateGuest.
> nit: It's not obvious how we get to NavigateGuest.  Can you mention the
line(s)
> below that goes there?  
> 
> (Is it LoadURLWithParams?  CreateNewGuestWebViewWindow?  Both?  I don't see it
> in either, so I'm concerned that we're not actually covered.)

Wow! Good catch! You're right! LoadURLWithParams does not do this check! Eeek!
Thanks for that! PTAL!

[Fady Samuel, 2015-02-19 22:27:04]

PTAL Charlie! Phew! Writing those tests was harder than I expected!

[Charlie Reis, 2015-02-19 22:56:29]

Thanks for your diligence on this!  I'm glad we were able to avoid some cases
that might have had security issues.

LGTM with nits.

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:238: bool
open_url_from_tab_;
nit: Make past tense, e.g., did_open_url_from_tab_

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1896:
content::WebContents* new_guest_web_contents=
nit: Comma before =

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1898: // Verify that a
new guest was created.
nit: Move comment above previous statement.

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1906:
IN_PROC_BROWSER_TEST_F(WebViewTest, ContextMenuLanguageSettings) {
nit: Let's add a comment about the test, saying that we expect this case to
succeed, unlike navigations initiated by the webview.

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1921:
mock->WaitForOpenURLFromTab();
Can we verify something that shows the navigation succeeded?  Maybe add a
WebContentsAddedObserver and show that it gets to the language settings URL?

https://codereview.chromium.org/890183002/diff/160001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/simple/main.js (right):

https://codereview.chromium.org/890183002/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/simple/main.js:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
nit: 2015
(Or is this just a move?)

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:1171: //
chrome://settings.
nit: chrome://
(settings isn't part of the scheme)

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:288: void
LoadURLWithParams(const GURL& url,
nit: Let's document what force_navigation means, since it's not obvious.

[Fady Samuel, 2015-02-20 00:32:28]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[Fady Samuel, 2015-02-20 00:33:06]

Addressed comments. CQ'ing! Thanks!

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:238: bool
open_url_from_tab_;
On 2015/02/19 22:56:28, Charlie Reis wrote:
> nit: Make past tense, e.g., did_open_url_from_tab_

Done.

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1896:
content::WebContents* new_guest_web_contents=
On 2015/02/19 22:56:28, Charlie Reis wrote:
> nit: Comma before =

Done.

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1898: // Verify that a
new guest was created.
On 2015/02/19 22:56:28, Charlie Reis wrote:
> nit: Move comment above previous statement.

Done.

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1906:
IN_PROC_BROWSER_TEST_F(WebViewTest, ContextMenuLanguageSettings) {
On 2015/02/19 22:56:28, Charlie Reis wrote:
> nit: Let's add a comment about the test, saying that we expect this case to
> succeed, unlike navigations initiated by the webview.

Done.

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1921:
mock->WaitForOpenURLFromTab();
On 2015/02/19 22:56:28, Charlie Reis wrote:
> Can we verify something that shows the navigation succeeded?  Maybe add a
> WebContentsAddedObserver and show that it gets to the language settings URL?

Done.

https://codereview.chromium.org/890183002/diff/160001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/web_view/simple/main.js (right):

https://codereview.chromium.org/890183002/diff/160001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/simple/main.js:1: //
Copyright 2013 The Chromium Authors. All rights reserved.
On 2015/02/19 22:56:28, Charlie Reis wrote:
> nit: 2015
> (Or is this just a move?)

This is a move. from teardown.

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:1171: //
chrome://settings.
On 2015/02/19 22:56:28, Charlie Reis wrote:
> nit: chrome://
> (settings isn't part of the scheme)

Done.

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.h (right):

https://codereview.chromium.org/890183002/diff/160001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.h:288: void
LoadURLWithParams(const GURL& url,
On 2015/02/19 22:56:28, Charlie Reis wrote:
> nit: Let's document what force_navigation means, since it's not obvious.

Done.

[Fady Samuel, 2015-02-20 00:33:15]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2015-02-20 00:34:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/890183002/180001

[Charlie Reis, 2015-02-20 00:37:59]

Thanks!

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (right):

https://codereview.chromium.org/890183002/diff/160001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_browsertest.cc:1896:
content::WebContents* new_guest_web_contents=
On 2015/02/20 00:33:06, Fady Samuel wrote:
> On 2015/02/19 22:56:28, Charlie Reis wrote:
> > nit: Comma before =
> 
> Done.

Ha!  Yes, I meant space, of course.

[commit-bot: I haz the power, 2015-02-20 01:31:56]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-20 01:32:33]

Patchset 10 (id:??) landed as
https://crrev.com/9626273c576eaea50bcd2ccfa6b372a091f51409
Cr-Commit-Position: refs/heads/master@{#317209}

[wjmaclean, 2015-07-17 01:31:28]

wjmaclean@chromium.org changed reviewers:
+ lazyboy@chromium.org

[wjmaclean, 2015-07-17 01:31:46]

wjmaclean@chromium.org changed reviewers:
+ wjmaclean@chromium.org

[wjmaclean, 2015-07-17 01:31:47]

+ lazyboy@ for OWNERS

[Charlie Reis, 2015-07-17 16:15:09]

On 2015/07/17 01:31:47, wjmaclean wrote:
> + lazyboy@ for OWNERS

I think you posted this on the wrong CL.

[wjmaclean, 2015-07-17 16:36:22]

wjmaclean@chromium.org changed reviewers:
- lazyboy@chromium.org, wjmaclean@chromium.org

[wjmaclean, 2015-07-17 16:37:04]

On 2015/07/17 16:15:09, Charlie Reis wrote:
> On 2015/07/17 01:31:47, wjmaclean wrote:
> > + lazyboy@ for OWNERS
> 
> I think you posted this on the wrong CL.

Yeah, we sorted that out last night, but I forgot to take him off the old issue.
Thanks for spotting it.