Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1678233003:
Don't focus the location bar in a phishy situation. (Closed)
|
Created:
4 years, 10 months ago by palmer Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't focus the location bar in a phishy situation.
There is logic to focus the location bar for editing if the URL is about:blank.
However, if the page transition type is PAGE_TRANSITION_LINK, bypass that logic;
it's not really a blank page. This avoids a phishy edge case with window.open.
BUG=567445
Committed: https://crrev.com/c70cb1fe9303df33b11187d0f5f60d22938e6e63
Cr-Commit-Position: refs/heads/master@{#379396}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Take creis' suggestion; useless sketch of a test. #Patch Set 3 : Test in chrome/. #
Total comments: 4
Patch Set 4 : Make it a uitest. #
Total comments: 6
Patch Set 5 : Wait for load in the WC returned by GetActiveWebContents #
Total comments: 12
Patch Set 6 : Respond to comments. Thanks! #
Total comments: 1
Messages
Total messages: 30 (8 generated)
palmer@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org
nasko or creis, could you please take a look?
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678233003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678233003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Glad you're narrowing down the cause for this! That said, this doesn't look like the right fix to me. We shouldn't be depending on PageTransition for security decisions in general, and this seems a few steps removed from the bug. Let's try to understand a bit more of how the attack works, since I suspect it involves cross-process navigations. I'll see if I can take another look tomorrow.
New suggestion below, after investigating. Is it possible to add a test for this? The navigation part would be easy (run a script in a page that does w=window.open(); w.opener=null; w.location.href=url). The interesting part is checking whether the omnibox has focus afterward. Not sure how easy that is. https://codereview.chromium.org/1678233003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1678233003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:2920: entry->GetURL() == GURL(url::kAboutBlankURL)) { Here's what I would recommend (if we decide to keep the focus-about-blank logic for startup): // If we are starting at about:blank, give the omnibox focus to let the user easily edit it. We intentionally check the pending entry rather than the visible one, since we don't want to be tricked by non-visible pending URLs. See https://crbug.com/567445. NavigationEntryImpl* entry = controller_.GetPendingEntry(); if (controller_.IsInitialNavigation() && entry && !entry->is_renderer_initiated() && entry->GetURL() == GURL(url::kAboutBlankURL)) { ...
https://codereview.chromium.org/1678233003/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1678233003/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:2920: entry->GetURL() == GURL(url::kAboutBlankURL)) { On 2016/02/10 05:47:48, Charlie Reis wrote: > Here's what I would recommend (if we decide to keep the focus-about-blank logic > for startup): > > // If we are starting at about:blank, give the omnibox focus to let the user > easily edit it. We intentionally check the pending entry rather than the > visible one, since we don't want to be tricked by non-visible pending URLs. See > https://crbug.com/567445. > NavigationEntryImpl* entry = controller_.GetPendingEntry(); > if (controller_.IsInitialNavigation() && > entry && !entry->is_renderer_initiated() && > entry->GetURL() == GURL(url::kAboutBlankURL)) { > ... OK, I did that, and added a sketch of a test that tests nothing. :) Do you have any clues about how I can make a test that does do something? Do I need to migrate it to chrome browsertests, or is there some vestigial LocationBar in content/ (doesn't look like it)?
creis@chromium.org changed reviewers: + pkasting@chromium.org
Good question about the test. It will need to be in the chrome/ layer to be meaningful, since the bug involves going out to OpenURLFromTab to get to UpdateUIForNavigationInTab. Maybe something like browser_navigator_browsertest.cc? We'll also want to check whether the location bar was given focus. @pkasting, can you recommend how to do that?
OK, here's a test in chrome/. If ui_test_utils::IsViewFocused worked, I'd be more sure that the test was testing the right thing. https://codereview.chromium.org/1678233003/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_navigator_browsertest.cc (right): https://codereview.chromium.org/1678233003/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_navigator_browsertest.cc:1450: ASSERT_FALSE(omnibox->IsEditingOrEmpty()); This test passes, but I'm not 100% sure it's testing the right thing. Any clues? https://codereview.chromium.org/1678233003/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_navigator_browsertest.cc:1451: // ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX)); This line causes the link to fail: No implementation available for the function ui_test_utils::IsViewFocused. I'm not sure why. Any clues?
https://codereview.chromium.org/1678233003/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_navigator_browsertest.cc (right): https://codereview.chromium.org/1678233003/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_navigator_browsertest.cc:1450: ASSERT_FALSE(omnibox->IsEditingOrEmpty()); On 2016/02/12 01:15:54, palmer wrote: > This test passes, but I'm not 100% sure it's testing the right thing. Any clues? It's not. Just focusing the omnibox does not set the state to editing. https://codereview.chromium.org/1678233003/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_navigator_browsertest.cc:1451: // ASSERT_TRUE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_OMNIBOX)); On 2016/02/12 01:15:54, palmer wrote: > This line causes the link to fail: No implementation available for the function > ui_test_utils::IsViewFocused. I'm not sure why. Any clues? That's linked into the interactive_ui_tests target, whereas this is linked into browser_tests.
> > This line causes the link to fail: No implementation available for the > function > > ui_test_utils::IsViewFocused. I'm not sure why. Any clues? > > That's linked into the interactive_ui_tests target, whereas this is linked into > browser_tests. Thank you. Fixed that. I think this works; the test fails without the change and passes with it.
On 2016/02/12 23:00:17, palmer wrote: > > > This line causes the link to fail: No implementation available for the > > function > > > ui_test_utils::IsViewFocused. I'm not sure why. Any clues? > > > > That's linked into the interactive_ui_tests target, whereas this is linked > into > > browser_tests. > > Thank you. Fixed that. I think this works; the test fails without the change and > passes with it. Hmm, I'm curious which way the test is failing without the fix, since it looks like the test is looking at the wrong tab. (Glad you're finding a way forward!) I'm in a meeting so I can't try it out myself right now. https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:759: EXPECT_FALSE(web_contents->GetController().GetPendingEntry()); I don't think you don't need this line. Technically there will be a pending entry during the popup's navigation. https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:761: web_contents->GetVisibleURL()); Hmm, this isn't the WebContents where the spoof happens. There's a separate popup WebContents that we have to wait for (and wait for it to finish navigating). https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:762: EXPECT_FALSE(IsViewFocused(VIEW_ID_OMNIBOX)); Which window is this checking? Is the test opening the popup in the same window or a different one? (I'm not sure if you need the popup to be in a new window for the attack to succeed. If so, you might need to specify the size in the window.open call.)
> Hmm, I'm curious which way the test is failing without the fix, since it looks
> like the test is looking at the wrong tab. (Glad you're finding a way
forward!)
[11452:11516:0216/173324:WARNING:embedded_test_server.cc(167)] Request not
handled. Returning 404: /favicon.ico
../../chrome/browser/ui/browser_focus_uitest.cc:762: Failure
Value of: IsViewFocused(VIEW_ID_OMNIBOX)
Actual: true
Expected: false
[ FAILED ] BrowserFocusTest.AboutBlankNavigationLocationTest, where TypeParam
= and GetParam() = (3373 ms)
[----------] 1 test from BrowserFocusTest (3373 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (3374 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] BrowserFocusTest.AboutBlankNavigationLocationTest, where TypeParam
= and GetParam() =
1 FAILED TEST
[0216/173324:ERROR:kill_posix.cc(82)] Unable to terminate process group 11452:
No such process
[1/1] BrowserFocusTest.AboutBlankNavigationLocationTest (4195 ms)
1 test failed:
BrowserFocusTest.AboutBlankNavigationLocationTest
(../../chrome/browser/ui/browser_focus_uitest.cc:742)
(I will address the other comments in a subsequent message)
https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:759: EXPECT_FALSE(web_contents->GetController().GetPendingEntry()); On 2016/02/12 23:43:47, Charlie Reis (catching up) wrote: > I don't think you don't need this line. Technically there will be a pending > entry during the popup's navigation. Done. https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:761: web_contents->GetVisibleURL()); On 2016/02/12 23:43:47, Charlie Reis (slow til Mar 7) wrote: > Hmm, this isn't the WebContents where the spoof happens. There's a separate > popup WebContents that we have to wait for (and wait for it to finish > navigating). Following the example of other code in this file, I added: ASSERT_NO_FATAL_FAILURE(content::WaitForLoadStop( browser()->tab_strip_model()->GetActiveWebContents())); https://codereview.chromium.org/1678233003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:762: EXPECT_FALSE(IsViewFocused(VIEW_ID_OMNIBOX)); On 2016/02/12 23:43:47, Charlie Reis (slow til Mar 7) wrote: > Which window is this checking? Is the test opening the popup in the same window > or a different one? (I'm not sure if you need the popup to be in a new window > for the attack to succeed. If so, you might need to specify the size in the > window.open call.) I think the core problem is that the Omnibox is focused for editing, not so much that it's a particular size. The size arguments in the example spoof seem to be just for tuning the attack to get the right text scrolled to the leading-most edge. I could be wrong though.
> Following the example of other code in this file, I added: > > ASSERT_NO_FATAL_FAILURE(content::WaitForLoadStop( > browser()->tab_strip_model()->GetActiveWebContents())); I should add that I'm not entirely certain that's sufficient. (But the test does continue to pass with it, and it seems to make sense to me.)
Sorry for the delay! (I've been at an event all week.) LGTM. The fix looks right, and the updated test seems to be doing the right thing, to the extent I understand it. https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:763: EXPECT_FALSE(IsViewFocused(VIEW_ID_OMNIBOX)); Sure, these lines look plausible to me, assuming the popup opens in the same window. (That's probably enough to repro the bug.)
pkasting, ping. :)
LGTM https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:741: // Ensure that crbug.com/567445 does not regress. Nit: Explain here what this is testing, e.g. "Ensure the omnibox does not get focused when loading about:blank in a case where it's not the startup URL, e.g. when a page opens a popup to about:blank and then navigates it. This is a potential security issue; see comments in WebContentsImpl::FocusLocationBarByDefault()." https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:744: embedded_test_server()->GetURL("/title1.html")); Nit: Store URL in a temp since you use it twice: const std::string url1(embedded_test_server()->GetURL("/title1.html")); ui_test_utils::NavigateToURL(browser(), url1); ... EXPECT_EQ(url1, web_contents->GetVisibleURL()); https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:747: "var w = window.open('about:blank');" Nit: Could go on previous line (and next two lines get indented even) But even better would be this, which is shorter and simpler overall: const std::string url2(embedded_test_server()->GetURL("/title2.html").spec()); const std::string spoof("var w = window.open('about:blank'); w.opener = null;" "w.document.location = '" + url2 + "';"); ... ASSERT_TRUE(content::ExecuteScript(web_contents, spoof)); (|url2| here is so named to parallel the suggestion above) https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:762: browser()->tab_strip_model()->GetActiveWebContents())); Is this different than |web_contents|? If so I'd add a comment about why, since this seems subtle. https://codereview.chromium.org/1678233003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1678233003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2938: // See https://crbug.com/567445. Nit: I'd prefer to explain sufficiently inline that we don't need the bug reference. This way a reader doesn't need to go anywhere else to understand this sufficiently. Maybe something like this: When the browser is started with about:blank as the startup URL, focus the location bar (which will also select its contents) so users can simply begin typing to navigate elsewhere. We need to be careful not to trigger this for anything other than the startup navigation. In particular, if we allow an attacker to open a popup to about:blank, then navigate, focusing the omnibox will cause the end of the new URL to be scrolled into view instead of the start, allowing the attacker to spoof other URLs. The conditions checked here are all aimed at ensuring no such attacker-controlled navigation can trigger this. Note that we check the pending entry instead of the visible one; for the startup URL case these are the same, but for the attacker-controlled navigation case the visible entry is the committed "about:blank" URL and the pending entry is the problematic navigation elsewhere.
https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_focus_uitest.cc (right): https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:741: // Ensure that crbug.com/567445 does not regress. On 2016/03/03 21:02:18, Peter Kasting wrote: > Nit: Explain here what this is testing, e.g. "Ensure the omnibox does not get > focused when loading about:blank in a case where it's not the startup URL, e.g. > when a page opens a popup to about:blank and then navigates it. This is a > potential security issue; see comments in > WebContentsImpl::FocusLocationBarByDefault()." Done. https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:744: embedded_test_server()->GetURL("/title1.html")); On 2016/03/03 21:02:18, Peter Kasting wrote: > Nit: Store URL in a temp since you use it twice: > > const std::string url1(embedded_test_server()->GetURL("/title1.html")); > ui_test_utils::NavigateToURL(browser(), url1); > ... > EXPECT_EQ(url1, web_contents->GetVisibleURL()); Done. https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:747: "var w = window.open('about:blank');" On 2016/03/03 21:02:18, Peter Kasting wrote: > Nit: Could go on previous line (and next two lines get indented even) > > But even better would be this, which is shorter and simpler overall: > > const std::string url2(embedded_test_server()->GetURL("/title2.html").spec()); > const std::string spoof("var w = window.open('about:blank'); w.opener = null;" > "w.document.location = '" + url2 + "';"); > ... > ASSERT_TRUE(content::ExecuteScript(web_contents, spoof)); > > (|url2| here is so named to parallel the suggestion above) Done. https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:762: browser()->tab_strip_model()->GetActiveWebContents())); On 2016/03/03 21:02:18, Peter Kasting wrote: > Is this different than |web_contents|? If so I'd add a comment about why, since > this seems subtle. Yes, they are different; it's the newly-opened contents that has the phishy-looking URL. I added a comment. https://codereview.chromium.org/1678233003/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_focus_uitest.cc:763: EXPECT_FALSE(IsViewFocused(VIEW_ID_OMNIBOX)); On 2016/02/25 00:48:40, Charlie Reis (OOO til Mar 7) wrote: > Sure, these lines look plausible to me, assuming the popup opens in the same > window. (That's probably enough to repro the bug.) Acknowledged. https://codereview.chromium.org/1678233003/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1678233003/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2938: // See https://crbug.com/567445. On 2016/03/03 21:02:19, Peter Kasting wrote: > Nit: I'd prefer to explain sufficiently inline that we don't need the bug > reference. This way a reader doesn't need to go anywhere else to understand > this sufficiently. > > Maybe something like this: > > When the browser is started with about:blank as the startup URL, focus the > location bar (which will also select its contents) so users can simply begin > typing to navigate elsewhere. > > We need to be careful not to trigger this for anything other than the startup > navigation. In particular, if we allow an attacker to open a popup to > about:blank, then navigate, focusing the omnibox will cause the end of the new > URL to be scrolled into view instead of the start, allowing the attacker to > spoof other URLs. The conditions checked here are all aimed at ensuring no such > attacker-controlled navigation can trigger this. > > Note that we check the pending entry instead of the visible one; for the startup > URL case these are the same, but for the attacker-controlled navigation case the > visible entry is the committed "about:blank" URL and the pending entry is the > problematic navigation elsewhere. Done.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1678233003/#ps100001 (title: "Respond to comments. Thanks!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1678233003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1678233003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Don't focus the location bar in a phishy situation. There is logic to focus the location bar for editing if the URL is about:blank. However, if the page transition type is PAGE_TRANSITION_LINK, bypass that logic; it's not really a blank page. This avoids a phishy edge case with window.open. BUG=567445 ========== to ========== Don't focus the location bar in a phishy situation. There is logic to focus the location bar for editing if the URL is about:blank. However, if the page transition type is PAGE_TRANSITION_LINK, bypass that logic; it's not really a blank page. This avoids a phishy edge case with window.open. BUG=567445 Committed: https://crrev.com/c70cb1fe9303df33b11187d0f5f60d22938e6e63 Cr-Commit-Position: refs/heads/master@{#379396} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c70cb1fe9303df33b11187d0f5f60d22938e6e63 Cr-Commit-Position: refs/heads/master@{#379396}
Message was sent while issue was closed.
groby@chromium.org changed reviewers: + groby@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1678233003/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1678233003/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2952: // the pending entry is the problematic navigation elsewhere. Thank you, thank you, thank you! Future Rachel is immensely grateful for this comment! (Because 6 months hence, I'll have no idea why this is here otherwise :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
