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

Issue 155711 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment

Security: forced oom in browser process due to indefinitely growing buffer in chunked decoder

Reported by szasza.c...@gmail.com, Oct 13 2012

Issue description

23.0.1244.0 dev
22.0.1229.94 stable
(+indefinite growing partially affects mozilla
since the code was derived from its codebase)

testing platform: winxp sp2/sp3, 32bit win7

relevant code in: net/http/http_chunked_decoder.cc
                  net/http/http_stream_parser.cc

description: HttpChunkedDecoder is an object meant to filter incoming chunked data in HttpStreamParser in such a way that it works correctly no matter where the previous tcp transmission delimited. Therefore it maintains an std::string, line_buf_ to handle situations where the \n of a chunk header hasn't arrived yet, it is done with a simple append, the size of line_buf_ is never checked against any constraint. 

An attacker can transmit chunked transfer encoded data containing no newline, leading the accessing browser to run out of memory and crash.

test case: [1] run poc
           [2] navigate to localhost
           [3] observe that do_malloc returns 0, resulting in the
           different route (*)  of execution in the patched malloc
           of chromium, eventually calling the new handler nh*
           pointing to an int3 (**)
(*)
dev build: 03506F82   JNZ SHORT chrome_1.03506FE6
release: 01C31688   JNZ SHORT chrome_1.01C316D5)

(**)
allocator_shim.cc line 92: (nh*)();
dev build: 03506FE1  CALL ESI
release: 01C316D1   CALL ESI

observations: obviously line_buf_ could easily be limited to
some small number, chunked headers are meant to store an integer sized number in ascii hex and only some small additional stuff. 
talking about integers, the casting of buf_len to int from size_t in chunked_decoder is pretty lame; its passed to StringPiece which takes
size_t, and it could also be possible (but i havent tested it) that
on a largeaddressaware build probably running on 64bit win.
an integer overflow would be possible at
 chunked_decoder.cc/ line 114
using the above line_buf_ growing approach to get a minus value in buf_len.

 
chromehax.py
1.3 KB View Download

Comment 1 by jsc...@chromium.org, Oct 15 2012

Cc: eroman@chromium.org rvargas@chromium.org
Labels: -Pri-0 -Area-Undefined Area-Internals Internals-Network
Would one of the CCs mind taking a look?
Cc: wtc@chromium.org
Adding Wan-Teh too.

The suggestion of a possible integer overflow is worrysome.
Cc: mmenke@chromium.org willchan@chromium.org
Labels: -Internals-Network Internals-Network-HTTP
Ew. Matt, if you're around, you've probably taken a look at this more recently than me and should take a look. I'm going to dive in once I get a moment.

Comment 4 by mmenke@chromium.org, Oct 15 2012

Owner: mmenke@chromium.org
Status: Assigned
Looking over the code, looks like the bug report is accurate.

I'm unsure exactly what would happen on the integer overflow - we'd end up with a negative buffer length and make a StringPiece with it, which would cast it back to a size_t, I believe.  Looks like this could be pretty bad.

Comment 5 by mmenke@chromium.org, Oct 15 2012

Oh, and I'm assigning it to myself and plan to just add some sane limit at which point we return an error.  If anyone else wants to tackle it, or has a better idea, feel free to claim the bug.
Yes, this is a bad bug. It looks very easy to trigger an OOM. That said, in my quick skim, I don't see how someone can use this exploit to take over Chromium, even with the integer overflow, so hopefully it's not possible :) I ought to think about it more, but I believe this bug only has crash potential and does not pose a significant security risk. It also can cause bad performance since the std::string requires large chunks of contiguous memory.

Overall, the code is suboptimal and we should fix it. Adding a sane limit is a good first step. Preferably not a CHECK though, since the main problem AFAICT is a server-triggerable crash, so we should gracefully handle it instead.

Comment 7 by mmenke@chromium.org, Oct 15 2012

Actually, I think all we'd end up doing is reading through random memory looking for a semicolon until we crash or find one.  Suppose there may be some clever way to figure out if we have semi-colons near a 2^32 byte-boundary relative to the start of the string, but not sure how useful that'd be.  If we don't find a semi-colon, or it's too far after a 2^32 byte boundary, we'll be unable to parse the string as a chunk length.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 18 2012

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

------------------------------------------------------------------------
r162756 | mmenke@chromium.org | 2012-10-18T19:18:00.916065Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_chunked_decoder_unittest.cc?r1=162756&r2=162755&pathrev=162756
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_chunked_decoder.cc?r1=162756&r2=162755&pathrev=162756
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/http/http_chunked_decoder.h?r1=162756&r2=162755&pathrev=162756

Fix a crash when a line containing the length of an HTTP
chunk-encoded response is too long.

R=willchan@chromium.org
BUG= 155711 

Review URL: https://chromiumcodereview.appspot.com/11191003
------------------------------------------------------------------------
btw, should i notify mozilla? (or it would violate responsible disclosure?)
their (the original; http://mxr.mozilla.org/mozilla-release/source/netwerk/protocol/http/nsHttpChunkedDecoder.cpp ) 
version has the same logic regarding this header-buffer growing, the only difference is that they use a custom string
 implementation nsCString, which quite remarkably ignores OOM and just doesnt append in that case. No crash
is observable, nevertheless firefox obviously starts to malfunction 
at this point along with a bad performance throughout the system. 
I think it's a great idea to contact Mozilla about their version of the bug.  However, I think it would be best not to say much (Or anything) about the Chrome bug, just out of general paranoia.  I'm not a security person, and it looks like the worst you can do with this bug is basically a crash, but prefer to err on the side of caution.
Do we consider this bug fixed now? I'd like to close this one out if possible.

Some nit-picky thoughts:

* In HttpChunkedDecoder::ScanForChunkRemaining(const char* buf, int buf_len), buf_len and bytes_consumed should be size_t (get rid of those static_cast<int>s). Casting to different signedness and possibly different width is trouble.

* In theory, the expression line_buf_.length() + buf_len > kMaxLineBufLen could itself be subject to integer overflow, causing the check to falsely pass. But I *think* that would only happen if we had already used up a huge amount of memory and DoS'd ourselves, since buf_len results from StringPiece::find (i.e. it's a count of bytes already allocated). Are you guys sure? I always get a bit anxious when I see overflowish-looking expressions, so I thought I'd mention it.
Labels: OS-All Pri-2 SecImpacts-Stable SecImpacts-Beta SecSeverity-Low Merge-Requested Mstone-23
The patch does stop the attack from working (I get net::ERR_INVALID_CHUNKED_ENCODING as expected).

We should merge this patch into 23.

Fixing flags.
I think you're right to be concerned about mixing size_t's and ints.

That having been said, I consider it fixed - rightly or wrongly, our network stack uses negative return codes to indicate an error, so we can't use size_t's across our entire network stack, and therefore, I think we should minimize their scope.  It's relatively easy to reason about their interactions if size_t's are generally only used in a single file or function.

line_buf_.length() is at most kMaxLineBufLen.  buf_len is at most MAX_INT (And since it comes from a single socket read, it's actually much, much smaller, in general).  Adding them is therefore less than MAX_INT + kMaxLineBufLen, which fits well within the unsigned size_t you get from adding the two.

We could ditch the use of a string, and just use a GrowableIOBuffer instead (Or a scoped_ptr_malloc<char>, along with an int length, which is what GrowableIOBuffer does).  It's a bit more complicated, but avoids any use of size_t's at all.  Of course, then I suppose you could theoretically have overflow in that length check.
Status: Fixed
Ok, I understand. Thank you!

I'll mark this Fixed then, and we'll get it into 23.

Comment 15 by kareng@google.com, Nov 5 2012

i assume this can wait until stable 2? we've passed the train for stable 1.
Yes, I think so. Thank you, Karen.
Labels: -Restrict-View-SecurityTeam -Merge-Requested Restrict-View-SecurityNotify Merge-Approved
Status: FixUnreleased
"notifier: Merge Approved Improperly(cr): 155711".  I assume it just doesn't think much of scarybeasts setting Merge-Approved.  I'll plan to go ahead merge to M23 this evening, assuming I don't hear otherwise.
I'll do it for you now. Not sure why the bot got upset. I set the flag all the time.
Ok, thanks!

Maybe it's just that no one actually pays attention to the notifier.  :)
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 12 2012

Labels: -Merge-Approved merge-merged-1271
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=167233

------------------------------------------------------------------------
r167233 | cevans@chromium.org | 2012-11-12T20:54:06.667838Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/net/http/http_chunked_decoder_unittest.cc?r1=167233&r2=167232&pathrev=167233
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/net/http/http_chunked_decoder.cc?r1=167233&r2=167232&pathrev=167233
   M http://src.chromium.org/viewvc/chrome/branches/1271/src/net/http/http_chunked_decoder.h?r1=167233&r2=167232&pathrev=167233

Merge 162756 - Fix a crash when a line containing the length of an HTTP
chunk-encoded response is too long.

R=willchan@chromium.org
BUG= 155711 

Review URL: https://chromiumcodereview.appspot.com/11191003

TBR=mmenke@chromium.org
Review URL: https://codereview.chromium.org/11362212
------------------------------------------------------------------------
@szasza.contact: would you like us to credit you by name? If so, what name?

Also, as you can see, we've gone for "Low" severity. We'd be happy to revise upwards if there's any way to demo the integer issues causing a bad write before hitting OOM.
@scarybeasts: Yeah, it would be great, thanks. My name is Attila Szász.

I'd also be happy too, if i could ;) but the mention of the integer overflow only as a sidenote had good reason; I couldn't find anything harmful building on it.
This is quite a buggy looking code mainly due to its Cish sorta state machinery;
achieves a good performance at the expense of being subject to some first doubts
about its possible execution paths,
nevertheless i believe these size_t castbacks, the check on parsed_number, bytes_consumed, and the fact that the mainloop's buf_len is seperated from the affected parsing
make it impossible to do nasty things around memmove. 

The worst thing you can do imo, and the one that hasnt been mentioned yet is
a very weak kind of outofbounds read at  
   185 while (len && start[len - 1] == ' ') 
buf_len this time is implicitly int (and also minus when the position of ';' is still after the 2gig boundary - correct me if im wrong) so i guess
nothing stops you from figuring out the number of consecutive spaces somewhere before your string at this point. hurray! :DD

Status: Fixed
Project Member

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

Labels: -Type-Security -Area-Internals -Internals-Network-HTTP -SecImpacts-Stable -SecImpacts-Beta -SecSeverity-Low -Mstone-23 Security-Severity-Low Security-Impact-Stable Security-Impact-Beta Cr-Internals-Network-HTTP Cr-Internals M-23 Type-Bug-Security
Labels: -Restrict-View-SecurityNotify
Project Member

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

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

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

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

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

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

Comment 30 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 31 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 32 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
Components: Internals>Network
Components: -Internals>Network>HTTP

Sign in to add a comment