325403 - (zdi-can-015) CSS Letter-Spacing Heap Overflow Vulnerability (ZDI-06-010)

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Daniel Veditz [:dveditz]]

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 [:bzbarsky]]

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]

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 [:bzbarsky]]

> 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]]

(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]

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]]

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]]

> If I could jigger that testcase around so newWordBuf != aWordBuf 

I mean ==, not !=

Comment 8

[rbs]

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]

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]]

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]]

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]]

Attachment: linux PoC

Comment 13

[Daniel Veditz [:dveditz]]

Attachment: windows PoC

Comment 14

[rbs]

Attachment: possible patch

Play the game with somebody else. (no provision for long words. That will break other things anyway.)

Comment 15

[rbs]

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 [:bzbarsky]]

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]

Attachment: updated patch

Comment 18

[rbs]

Checked in the trunk.

Comment 19

[Tim Riley [:timr]]

dveditz agree to check this one in for rds

Comment 20

[Daniel Veditz [:dveditz]]

Attachment: "updated patch" merged to 1.7 branch

Comment 21

[Daniel Veditz [:dveditz]]

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]]

Comment on attachment 210957 [details] [diff] [review]
updated patch

This is the one for the 1.8 branches

Comment 23

[Tim Riley [:timr]]

Comment on attachment 211757 [details] [diff] [review]
"updated patch" merged to 1.7 branch

a=timr for drivers.

Comment 24

[Daniel Veditz [:dveditz]]

attachment 211757 [details] [diff] [review] checked into aviary101/moz17 branches

Comment 25

[Daniel Veditz [:dveditz]]

Attachment 210957 [details] [diff] checked into the 1.8.0 and 1.8 branches

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 210957 [details] [diff] [review]
updated patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 27

[Marcia Knous [:marcia]]

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] (inactive)]

I crash with Firefox 1.0.8 linux 2006021 on the windowsPoC after closing the security device password dialog.

Comment 29

[Bob Clary [:bc] (inactive)]

(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] (inactive)]

(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]]

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] (inactive)]

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