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

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2011
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Security: Location Bar Spoofing using very long string on a web address in the location bar

Reported by jconsult...@gmail.com, Jul 11 2011

Issue description

VULNERABILITY DETAILS
When a webpage try to go on a website that contain very long string in the address location , location bar is spoofed (view screenshot).

VERSION
Chrome Version: [14.0.794.0] + [stable, beta, or dev]
Operating System: [Windows 7]

REPRODUCTION CASE
view Testcase1 and Tescase2

/Jordi Chancel

 
GOOGLECHROME14SPOOF.png
45.7 KB View Download
TESTCASE1.htm
1004 KB View Download
TESTCASE2.html
329 bytes View Download
Labels: -Pri-0 -Area-Undefined Pri-1 Area-UI SecSeverity-Medium Mstone-13 OS-All
Owner: creis@chromium.org
Status: Assigned
Charlie, can you add this to your url bar spoofing bugs :)

Jordi, do you have any repro that does not need drag interaction. That will increase the secseverity.
I will try to code a perfect testcase soon.
I think that it's possible to automate this without drag interaction but i haven't found a way for that (except TESTCASE2).

SecSeverity-Medium is probably appropriate but not sure.

Can you try to find a way for this ? 

Thanks.
someone can help me to found a repro without drag interaction? thanks
@creis , I don't found a way for automate this, except TestCase2.html but I think that it's possible. 

Assigned for Google Chrome 13.x.x.x?
Cc: eroman@chromium.org
@creis : Why nobody works on this vulnerability?

Comment 8 by cdn@chromium.org, Sep 28 2011

Labels: -Mstone-13 Mstone-14

Comment 9 by creis@chromium.org, Sep 28 2011

Sorry, I've been out of town for a bit.  I'll take a look at this one tomorrow to see if I can figure it out.

Comment 10 by creis@chromium.org, Sep 29 2011

Cc: brettw@chromium.org darin@chromium.org supersat@chromium.org
Status: Started
Karl and I are taking a look.  The dragging of the long text to the URL bar initiates a search, and the fact that it's really long is breaking something in Chrome so that the spoof occurs.

Interesting observations:
1) If you copy a smaller portion of the text from the test page, Chrome will take you to Google but you'll see a 414 error page because the search string is too long.
2) If you change your search provider, the spoof will affect the new search engine instead of www.google.com.

There seem to be two issues involved.  First, the renderer is unable to parse the ViewMsg_Navigate IPC message with the long URL, so it drops it on the floor.  This is why the old content stays in the tab.  (Interestingly, in a debug build we actually end up with a sad tab, but release builds just ignore the IPC message.)

Second, the browser is confusingly showing "www.google.com" in the Omnibox rather than the full search string (something like "http://www.google.com/search?gcx=w&sourceid=chrome&ie=UTF-8&q=....").  We'll have to figure out why that's the case.

I think we should fix both issues-- the browser shouldn't be sending the renderer a message it can't decode, and we shouldn't be showing the wrong search URL while waiting for the search to complete.

Comment 11 by creis@chromium.org, Sep 29 2011

Cc: cevans@chromium.org
Chris, it appears that the second issue I described in comment 10 is intentional from your change here:
http://codereview.chromium.org/1569011

We're apparently cropping overly-long URLs to just the origin, rather than eliding them.  I don't see much of a better option, so perhaps we should just keep that behavior as is?

If so, dragging a long URL into the omnibox may end up taking you to a search results page (or a 414 error page) where the URL bar just shows www.google.com (or whatever your search provider's origin is).  I suppose that's ok, from a URL spoof perspective.

We'll look at the failed IPC message parsing next.  I'm guessing the browser will have to catch this case before sending the message and abort the navigation instead, so that we don't show it in the URL bar.

Comment 12 by creis@chromium.org, Sep 29 2011

Interestingly, the IPC message parse is failing due to a similar check that Chris added to protect the browser from OOM ( issue 20233 ):
http://codereview.chromium.org/523088

That CL causes us to avoid parsing any URL over size kMaxURLChars.  If the user types or drags a longer URL to the location bar (as in this bug), the renderer rejects it, either ignoring it or crashing in a debug build.

Conversely, if a longer URL shows up as a link in a web page and the renderer tries to put it in a message to the browser (e.g., by hovering over the link or clicking it), the browser process crashes in debug builds (yikes!).  In release builds, the browser process kills the renderer with a "He's dead, Jim," presumably because of the bad message.

That's no good.  We could easily add a check in TabContents::NavigateToEntry to avoid trying to navigate to a really long URL, but that's only part of the problem.  There's still plenty of ways URLs can end up in IPC messages, and we don't have guards against long URLs for any of them.  As a result, a page that gets long URLs sent to the browser process can easily cause crashes.  (Case in point, see  issue 71691 .)

Chris, I'm wondering if we should revisit how the kMaxURLChars check is done...  Should we have a check on the send side so that we don't send messages from the browser that the renderer can't receive?  Should we have guards on all the places that try to send messages with URLs so that they can handle the failure?  (There could be a lot of those...)  Any other ideas for avoiding these crashes?
Yes, we can have the check on the send side if we wish. It's an explicit non-goal to defend the browser process against DoS from a compromised renderer.

The concern leading to the "kill process and tab" implementation was: what to do if we want to reject a web page's attempt to set e.g. document.location? A lot of security decisions key off document.location so we were a little nervous on how to reject changes without leading to an inconsistent state.

Comment 14 by creis@chromium.org, Sep 30 2011

Here's what I'm thinking.  Perhaps we should allow the browser to keep killing renderers if they send messages with long URLs, since it's not that different from a renderer dying from OOM.  (It seems impractical to add guards for every way the renderer could send a URL to the browser, anyway.)

However, we should probably be careful not to send IPC messages from the browser to the renderer that we know will fail to parse.  We'll start by adding a guard on the Navigate message to fix this spoof, but we should look at other ways the browser sends URLs to the renderer to see if we can add guards there as well.  Otherwise we knowingly put ourselves into an odd state.
I've gone through content/common/view_messages.h, and these are the other ViewMsgs that can send URLs to the renderer:

ViewMsg_SetZoomLevelForLoadingURL
ViewMsg_SetZoomLevelForCurrentURL
ViewMsg_SetAltErrorPageURL
ViewMsg_GetAllSavableResourceLinksForCurrentPage
ViewMsg_GetSerializedHtmlDataForCurrentPageWithLocalLinks

These either seem safe to drop, pass constants, or send the URL that the browser is already at (meaning that if you can't navigate to an excessively-long URL, these will never send an excessively-long URL). 
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 3 2011

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=103720

------------------------------------------------------------------------
r103720 | creis@chromium.org | Mon Oct 03 09:19:40 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/tab_contents/tab_contents.cc?r1=103720&r2=103719&pathrev=103720
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/web_contents_unittest.cc?r1=103720&r2=103719&pathrev=103720

Add a check to NavigateToEntry to make sure the browser never attempts to navigate to a URL longer than content::kMaxURLChars

Patch from Karl Koscher <supersat@chromium.org>
BUG= 88949 

Review URL: http://codereview.chromium.org/8102002
Patch from Karl Koscher <supersat@chromium.org>.
------------------------------------------------------------------------
Status: Fixed
Fixed in r103720.  Chris, shall we merge this to the release branches?
Labels: Merge-Approved
Status: FixUnreleased
Labels: -Restrict-View-SecurityTeam -Mstone-14 Restrict-View-SecurityNotify Mstone-15 reward-topanel
Heya Charlie, quick question for the curious: any reason the renderer doesn't kill itself if it fails to deserialize a message? Seems like we're pretty much in an awful state if that happens, so sad-tabbing is the safest thing to do. And indeed would have saved us this bug (at least the security aspect)?

In terms of merging, there's just M15 open. We'll merge it for you if you like.
That's not a bad idea-- Karl listed the other ways that an unreadable message might get sent to the renderer in comment 15, and those mostly seem like things that would be prevented by not being able to navigate to an excessively long URL.  Having the renderer kill itself might be a good second line of defense.

Do you know the right approach to do that from within the renderer process?  In the browser process we use BrowserRenderProcessHost::ReceivedBadMessage with a UMA stat so that we can track it, though this doesn't generate a crash dump.  Would be nice to be able to track if this sort of kill happens frequently (or at all).
Also, I just confirmed that the fix does prevent the spoof on the Mac canary, 16.0.900.0.  Feel free to merge it to M15!
Wouldn't the best solution be to just use a hard CHECK in the renderer? That way we'd have a crash report to work from in tracking these down?
Project Member

Comment 23 by bugdroid1@chromium.org, Oct 4 2011

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=104010

------------------------------------------------------------------------
r104010 | creis@chromium.org | Tue Oct 04 15:55:40 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/tab_contents/tab_contents.cc?r1=104010&r2=104009&pathrev=104010
 M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view.cc?r1=104010&r2=104009&pathrev=104010

Add a second line of defense for receiving a bad message in the renderer.

BUG= 88949 
TEST=none

Review URL: http://codereview.chromium.org/8142009
------------------------------------------------------------------------
Labels: SecImpacts-Stable
Batch update: Guessing based on search criteria that this security bug impacted a stable release.
Charlie, can you please merge this to 874 branch.
Sure, will do.
Drat, the patches fail to compile on the 874 branch, since the test relies on a newer version of NavigationController::LoadURL.  Once the builders go green, I'll try to merge again, omitting the test.
Merged to the 874 branch in r104552 and r104554 (minus the test).
Labels: -Merge-Approved Merge-Merged merge-merged-874
Thanks a lot Charlie.
chrome 15.0.874.92 BETA fixes this vulnerability(TESTCASE1.HTM).
Labels: -reward-topanel
A drag interaction leading to an URL spoof seems like a fairly minor risk.
Labels: CVE-2011-3875
@scarybeast : can i write a blog post about this vulnerability now ?
@scarybeast : I go write an post blog about this vulnerability without PoC...
Let me know if you want the deletion of this post !
@jordi: if you just wait a day or two, you can blog about it _and_ link to the fixed stable build. That is preferable all round.
Keep an eye out on http://googlechromereleases.blogspot.com/ over the next couple of days :D

Comment 36 by cdn@chromium.org, May 15 2012

Status: Fixed
Marking old security bugs Fixed..
Project Member

Comment 37 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 38 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-UI -SecSeverity-Medium -Mstone-15 -SecImpacts-Stable M-15 Security-Severity-Medium Cr-UI Security-Impact-Stable Type-Bug-Security
Project Member

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

Labels: Restrict-View-EditIssue
Project Member

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

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

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

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

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

Labels: -Security-Severity-Medium Security_Severity-Medium
Cc: -supersat@chromium.org
Labels: -Security_Severity-Medium Security_Severity-None
This was incorrectly rated. Fixing severity. Drag interaction on location bar is a significant user interaction and we don't expect users to fall through this trap.
Project Member

Comment 45 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 46 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
Labels: CVE_description-submitted

Sign in to add a comment