Last Comment Bug 325403 - (zdi-can-015) CSS Letter-Spacing Heap Overflow Vulnerability (ZDI-06-010)
(zdi-can-015)
: CSS Letter-Spacing Heap Overflow Vulnerability (ZDI-06-010)
Status: RESOLVED FIXED
[sg:critical][rft-dl]
: verified1.7.13, verified1.8.0.2, verified1.8.1
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: rbs
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-31 19:46 PST by Daniel Veditz [:dveditz]
Modified: 2007-04-01 15:35 PDT (History)
8 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
linux PoC (1.21 KB, text/html)
2006-02-01 23:31 PST, Daniel Veditz [:dveditz]
no flags Details
windows PoC (163 bytes, text/html)
2006-02-01 23:32 PST, Daniel Veditz [:dveditz]
no flags Details
possible patch (6.15 KB, patch)
2006-02-02 04:27 PST, rbs
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
updated patch (8.57 KB, patch)
2006-02-06 17:39 PST, rbs
bzbarsky: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
"updated patch" merged to 1.7 branch (8.57 KB, patch)
2006-02-13 12:36 PST, Daniel Veditz [:dveditz]
dveditz: review+
dveditz: superreview+
timr: approval‑aviary1.0.8+
timr: approval1.7.13+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2006-01-31 19:46:38 PST
ZDI-CAN-015: Mozilla Firefox CSS Letter-Spacing Heap Overflow Vulnerability

-- ABSTRACT ------------------------------------------------------------

3Com has identified a vulnerability affecting the following products:

Firefox 1.5.x and below
Mozilla 1.7.x and below

-- VULNERABILITY DETAILS -----------------------------------------------

A remotely exploitable heap overflow exists in the latest versions of
the Firefox web browser.

The flaw is due to incorrect handling of the CSS "letter-spacing"
element. By specifying a large number, an attacker can overflow an
integer used during memory allocation. This buffer is then later used
to store user supplied data.

The vulnerable function, ComputeTotalWordDimensions(), can be found
in:

    ./layout/html/base/src/nsTextFrame.cpp

On line 5649* the memory is allocated by

    newWordBuf = \
        (PRUnichar*)nsMemory::Alloc(sizeof(PRUnichar)*newWordBufSize);

And 3 lines later used by memcpy causing the overflow 

    memcpy((void*)newWordBuf, aWordBuf, \
        sizeof(PRUnichar)*(newWordBufSize-moreSize));

The values of newWordBufSize and moreSize are influenced by the
function ComputeWordFragmentDimensions() which again uses the value
provided with the letter-spacing attribute. on line 5810* in
nsTextFrame.cpp.

    dimensions.width += wordLen*ts.mLetterSpacing;

This situation leads to heap corruption and can be controlled to
execute arbitrary code supplied by a malicious attacker.

* source lines differ between version.  Listed are for firefox 1.0.7.

-- CREDIT --------------------------------------------------------------

This vulnerability was discovered by:

    Anonymous
Comment 1 Boris Zbarsky [:bz] (TPAC) 2006-01-31 22:09:59 PST
So the issue is when we do (on current trunk):

6295         newWordBufSize += moreSize;

and this causes newWordBufSize to overflow a PRUint32, right?

Sounds like we should change that to something like (with better variable naming):

  PRUint32 increasedBufSize = newWordBufSize + moreSize;
  if (increasedBufSize < newWordBufSize) {
    // Integer overflow... Not much we can do here
    // Bail out in some safe way or something
  }
  newWordBufSize = newNewWordBufSize;

I'm not quite sure what a safe way to bail would be.  rbs, any ideas?

But even then, it seems like this lets things allocate large chunks of memory in one go if they want...
Comment 2 rbs 2006-01-31 22:56:29 PST
http://lxr.mozilla.org/mozilla/source/layout/generic/nsTextFrame.cpp#6300
I see that the code (trunk) has a null-check if the alloc fails (in which case, the text is treated as empty -- with moreDimensions.Clear() further down). Isn't it enough?

Also, the "VULNERABILITY DETAILS" description is not entirely true.

What ComputeTotalWordDimensions() does is to compute the non-breaking width across multiple spans, e.g., as in "text1<b>text2</b> where the total text shoudn't be broken. (The letter spacing is incidental.) The code (trunk) seems safe to me, unless I am missing something.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2006-01-31 23:02:22 PST
> I see that the code (trunk) has a null-check if the alloc fails

No.  The problem is that if, say, our original newWordBufSize is 2 and moreSize is (PR_UINT32_MAX - 1) then we'd end up allocating a 1-PRUnichar buffer (which succeeds, probably) and memcopying 2 PRUnichars into it (which is our heap overflow).
Comment 4 Daniel Veditz [:dveditz] 2006-01-31 23:26:31 PST
(In reply to comment #2)
> (The letter spacing is incidental.)

The letter spacing is an essential multiplier, otherwise you'd need an insanely long word to overflow.
Comment 5 rbs 2006-01-31 23:58:19 PST
bz, I see. In that case, iterating on your snippet above should do the trick:

  PRUint32 totalBufSize = newWordBufSize + moreSize;
  if (totalBufSize < newWordBufSize) {
    // Integer overflow... Not much we can do here. Just bail out now
    goto done;
  }
  newWordBufSize = totalBufSize;

---
re: Comment #4
> you'd need an insanely long word to overflow.

Exactly. 

What is happening in ComputeTotalWordDimensions() is that its return witdh variable is overloaded (like in GDI return codes). When it returns a negative value, it is the length of the buffer needed for the function to work -- not the actual width with letter-spacing, word-spacing and such, which it can't/hasn't yet computed without the needed buffer.
Comment 6 Daniel Veditz [:dveditz] 2006-02-01 00:09:47 PST
I can get an overflow, calling the nsMemory::Realloc() with 44 using
data:text/html,<div>this is a div</div><div style="letter-spacing:1127282">bo00000000000000000000000000000000000000000000000000000000000000000000000000b<b>AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA</b>whoaaaaaaaaazzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzzz</div>

If I could jigger that testcase around so newWordBuf != aWordBuf I'd fall into the ::Alloc case and then memcpy would overwrite the heap.
Comment 7 Daniel Veditz [:dveditz] 2006-02-01 00:18:59 PST
> If I could jigger that testcase around so newWordBuf != aWordBuf 

I mean ==, not !=
Comment 8 rbs 2006-02-01 04:29:51 PST
That's getting interesting. Why is it related to letter-spacing? Here is the early return in ComputeTotalWordDimensions() when it wants a bigger buffer. The letter-spacing is not used at that point (and there is no suspicious += in sight yet).

6395   // We need to adjust the length by look at the two pieces together
6396   // but if we have to grow aWordBuf, ask caller do it by return a negative value of size
6397   if ((wordLen + aRunningWordLen) > aWordBufSize) {
6398     dimensions.width = aWordBufSize - wordLen - aRunningWordLen; 
6399     return dimensions;
6400   }
Comment 9 rbs 2006-02-01 04:51:44 PST
What if... it is dimensions.width that overflows in the _normal_ codepath of ComputeTotalWordDimensions() due to that huge letter-spacing, and turning negative and hence being misinterpreted as a request to grow the buffer by the caller.
Comment 10 Daniel Veditz [:dveditz] 2006-02-01 08:36:49 PST
Yes, that's exactly what this bug has been describing. That routine uses a single return value for two completely different purposes and one is bleeding into the other. There's no way to indicate any other kind of error from ComputeWordFragmentDimensions.

Similar letterspacing*wordlen math shows up in MeasureText()

The spec says "there may be implementation-specific limits" on letter-spacing, but that simply means an attacker uses a correspondingly longer word. Our internal units seem to be 15x the pixel spacing set in CSS.
Comment 11 Daniel Veditz [:dveditz] 2006-02-01 08:56:38 PST
I also see width go negative in the for loop in nsTextFrame::RenderString but couldn't get it to be used (I think it's only use is to be passed into PaintTextDecorations() inside that loop?).

letter-spacing can also be negative which also seems to be leading to internally suspicious results, but I haven't gotten any crashes.
Comment 12 Daniel Veditz [:dveditz] 2006-02-01 23:31:53 PST
Created attachment 210450 [details]
linux PoC
Comment 13 Daniel Veditz [:dveditz] 2006-02-01 23:32:21 PST
Created attachment 210451 [details]
windows PoC
Comment 14 rbs 2006-02-02 04:27:22 PST
Created attachment 210467 [details] [diff] [review]
possible patch

Play the game with somebody else. (no provision for long words. That will break other things anyway.)
Comment 15 rbs 2006-02-02 12:09:14 PST
Comment on attachment 210467 [details] [diff] [review]
possible patch

bz, care to take a look before nsTextFrame changes again? The patch shouldn't make things any worse.
Comment 16 Boris Zbarsky [:bz] (TPAC) 2006-02-06 08:45:38 PST
Comment on attachment 210467 [details] [diff] [review]
possible patch

>Index: nsTextFrame.cpp
>@@ -508,7 +508,7 @@ public:
>-                                                 PRBool* aStop,
>+                                                 PRInt32* aMoreSize,

Please add documentation about what this arg means?  In fact, if you can document all the args to the function that would be great.  ;)

>@@ -6278,19 +6278,18 @@ nsTextFrame::ComputeTotalWordDimensions(

>+      PRBool moreSize = 0;

PRInt32, right?

>@@ -6308,13 +6307,13 @@ nsTextFrame::ComputeTotalWordDimensions(

>+          NS_ASSERTION((moreSize <= 0),
>+                       "ComputeWordFragmentWidth is asking more buffer");

s/Width/Dimensions/ here?

>@@ -6388,17 +6388,18 @@ nsTextFrame::ComputeWordFragmentDimensio

>+    *aMoreSize = -1; // flag that we shoud stop now

"should"

r+sr=bzbarsky
Comment 17 rbs 2006-02-06 17:39:59 PST
Created attachment 210957 [details] [diff] [review]
updated patch
Comment 18 rbs 2006-02-06 17:59:47 PST
Checked in the trunk.
Comment 19 Tim Riley [:timr] 2006-02-10 12:02:51 PST
dveditz agree to check this one in for rds
Comment 20 Daniel Veditz [:dveditz] 2006-02-13 12:36:05 PST
Created attachment 211757 [details] [diff] [review]
"updated patch" merged to 1.7 branch
Comment 21 Daniel Veditz [:dveditz] 2006-02-13 14:03:56 PST
Comment on attachment 211757 [details] [diff] [review]
"updated patch" merged to 1.7 branch

carrying over r/sr=bz, seeking a=
Comment 22 Daniel Veditz [:dveditz] 2006-02-13 14:04:30 PST
Comment on attachment 210957 [details] [diff] [review]
updated patch

This is the one for the 1.8 branches
Comment 23 Tim Riley [:timr] 2006-02-13 16:12:12 PST
Comment on attachment 211757 [details] [diff] [review]
"updated patch" merged to 1.7 branch

a=timr for drivers.
Comment 24 Daniel Veditz [:dveditz] 2006-02-13 17:02:38 PST
attachment 211757 [details] [diff] [review] checked into aviary101/moz17 branches
Comment 25 Daniel Veditz [:dveditz] 2006-02-13 18:23:55 PST
Attachment 210957 [details] [diff] checked into the 1.8.0 and 1.8 branches
Comment 26 Daniel Veditz [:dveditz] 2006-02-14 15:49:40 PST
Comment on attachment 210957 [details] [diff] [review]
updated patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 27 Marcia Knous [:marcia - use ni] 2006-02-17 13:53:14 PST
verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060217 Firefox/1.0.8. Adding keyword. according to dveditz, a crash would be bad, but I get no crash running the Windows PoC.

also verified Mozilla 1.7.13 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060217. Adding keyword.
Comment 28 Bob Clary [:bc:] 2006-02-23 04:06:33 PST
I crash with Firefox 1.0.8 linux 2006021 on the windowsPoC after closing the security device password dialog.
Comment 29 Bob Clary [:bc:] 2006-02-23 04:12:33 PST
(In reply to comment #28)
> I crash with Firefox 1.0.8 linux 2006021 on the windowsPoC after closing the
> security device password dialog.
> 

sorry, wrong bug. 
Comment 30 Bob Clary [:bc:] 2006-02-23 04:18:48 PST
(In reply to comment #29)

Actually this was the right bug. 7 hours of repetitive testing has taken its tool.

Crash in Firefox 1.0.8/linux/20060221 on the WindowsPoC. 
Comment 31 Jay Patel [:jay] 2006-02-24 14:01:12 PST
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, no crashes with either PoC.

dveditz - Re: Bob's 1.0.8 crash on Linux with the Windows PoC, is that something we should be worried about?
Comment 32 Bob Clary [:bc:] 2006-08-22 14:29:34 PDT
https://bugzilla.mozilla.org/attachment.cgi?id=210450&action=view
ff2b2 debug/nightly linux, windows no crash 

linux
###!!! ASSERTION: Computed overflow area must contain frame bounds: 'aNewSize.width == 0 || aNewSize.height == 0 || aOverflowArea->Contains(nsRect(nsPoint(0, 0), aNewSize))', file /work/mozilla/builds/ff/2.0/mozilla/layout/generic/nsFrame.cpp, line 4410
Break: at file /work/mozilla/builds/ff/2.0/mozilla/layout/generic/nsFrame.cpp, line 4410
###!!! ASSERTION: bad width: 'metrics.width>=0', file /work/mozilla/builds/ff/2.0/mozilla/layout/generic/nsLineLayout.cpp, line 1068
Break: at file /work/mozilla/builds/ff/2.0/mozilla/layout/generic/nsLineLayout.cpp, line 1068

https://bugzilla.mozilla.org/attachment.cgi?id=210451
ff2b2 debug/nightly linux, windows no crash 
###!!! ASSERTION: bad width: 'metrics.width>=0', file /work/mozilla/builds/ff/2.0/mozilla/layout/generic/nsLineLayout.cpp, line 1068
Break: at file /work/mozilla/builds/ff/2.0/mozilla/layout/generic/nsLineLayout.cpp, line 1068


Note You need to log in before you can comment on or make changes to this bug.