oops, I forgot to attach the report and to set a summary ...

Summary should be: "Google Chrome OOB Array Indexing Bug"

VendorNotification.txt 8.0 KB   View   Download

able to reproduce browser crash on chrome trunk version 5.0.359.0. looking more into it.

Thanks Tobias for this bug. I have prepared the patch and just testing it.

I've not tried reproducing. A few questions:
1) Is this indeed a browser process crash (chrome dies completely) or a renderer 
process crash (sad tab only)? If so, why are we not parsing inside the renderer?
2) What does "line.erase(-1);" do? Could an attacker do more than crash the process?

Depending on the answers to these questions, this could range from Sec-Critical to Sec-
None...

hey BJ, here are the answers :)

1) it is a browser process crash as i get the "Whoa ! Chrome has crashed" crash. as i
understand the network layer processing occurs in browser process and not in our
renderer sandbox, but i might be wrong here..
2) the crash occurs because of out of array read at previous line[line.length() - 1].
i dont see the program ever processing the next next line.erase(-1).

Inferno -- how do production builds of Linux and Mac behave, out of curiousity?

Oh, and in terms of severity of current production builds -- it's luckily low because 
Windows does runtime checking of STL indexes even in optimized builds.

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

------------------------------------------------------------------------
r42204 | inferno@chromium.org | 2010-03-21 17:36:31 -0700 (Sun, 21 Mar 2010) | 4 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/ftp/ftp_network_transaction.cc?r1=42204&r2=42203

Fix the out-of-bounds array read in the ftp response.
BUG=38845
TEST=Pass a ftp server response having two double quotes with no character in between and verify no browser crash happens.
Review URL: http://codereview.chromium.org/1082008
------------------------------------------------------------------------

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

------------------------------------------------------------------------
r42205 | inferno@chromium.org | 2010-03-21 17:59:31 -0700 (Sun, 21 Mar 2010) | 7 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/249/src/net/ftp/ftp_network_transaction.cc?r1=42205&r2=42204

Merge 42204 - Fix the outofbounds array read in the ftp response.
BUG=38845
TEST=Pass a ftp server response having two double quotes with no character in between and verify no browser crash happens.
Review URL: http://codereview.chromium.org/1082008

TBR=inferno@chromium.org
Review URL: http://codereview.chromium.org/1133007
------------------------------------------------------------------------

Fix merged to 4.1.

Results for Mac and Linux builds

Chrome 5.0.307.11 beta on Mac OS X - No Crash
Chrome 5.0.353.0 on ubuntu linux - No Crash

Re: comment #5
1) Is there a need for this to happen in the browser? Can we move it to the renderer as a mitigation?
2) Can you prove that it will never hit that line? Assuming that the memory immediately in front of 
the "line" variable can contain any value and that this value may be attacker controlled or at least 
influenced, I'd like to know what happens when you execute "line.erase(1)" on an empty line. We have 
to either rule out exploitability or assume it can be exploited... I'll see if I can debug this now.

Nevermind: I did not read comment #7 :)

So, this ALWAYS causes a browser crash because the index (-1) is invalid, which 
triggers an int 3. It's a DoS.

I'd still like to know why we are parsing this in the browser - is there any reason why 
we cannot do this parsing in the renderer?

This is a network stack question. We're parsing the directory listing in the renderer because it's just downloaded 
by the network stack, and we can process it later, after closing the connection to the server.

However, parsing the control socket responses directly influences the next actions taken by the network 
transaction. Doing that in the renderer doesn't sound like a good idea, because we don't trust the renderer.

On the other hand, there was an idea to run the network stack out-of-process. This may be applicable here.

Ok, that makes sense. It's good to keep the renderer away from the network layer.

It's also a good idea to keep the processing of network data outside the browser process. Is there an open 
feature request for moving HTTP/FTP/other network response processing out-of-process?

I think we don't have such bug, but maybe I just failed to find it. Darin or Wan-Teh should know for sure.

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

------------------------------------------------------------------------
r42238 | inferno@chromium.org | 2010-03-22 11:27:02 -0700 (Mon, 22 Mar 2010) | 5 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/ftp/ftp_network_transaction_unittest.cc?r1=42238&r2=42237

Add unit test to check for zero length dir in FTP PWD response. 

BUG=38845
TEST=FtpNetworkTransactionTest.ZeroLengthDirInPWD
Review URL: http://codereview.chromium.org/1166001
------------------------------------------------------------------------

Hi Tobias -- thanks for the report! We will release the fix shortly and credit you as 
"Tobias Klein" unless we hear otherwise.

Hi, could you please use "Tobias Klein (www.trapkit.de)"? Thanks!

(No comment was entered for this change.)

Verified with Google Chrome 4.1.249.1045 (Official Build 42898) and it doesn't crash.

(No comment was entered for this change.)

Verified fixed on Google Chrome	5.0.365.0 (Official Build 43016) unknown
WebKit	533.3
V8	2.2.0

http://googlechromereleases.blogspot.com/2010/03/stable-update-disable-translate.html

Releasing... adding credit to 
http://sites.google.com/a/chromium.org/dev/Home/chromium-security

Thanks again Tobias!

Sorry to bug people, hope this isn't a stupid question: Why don't the Mac and Linux 
versions crash? What do they do differently?

@ljseltzer: check out comment 7. windows does runtime stl index checking, so it
crashes when it detects that. mac and linux happily keep processing.

(No comment was entered for this change.)

Batch update.