The default bug view has changed. See this FAQ.
Bug 325403 (zdi-can-015)

CSS Letter-Spacing Heap Overflow Vulnerability (ZDI-06-010)

RESOLVED FIXED

Status

()

Core
Layout: Text
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dveditz, Assigned: rbs)

Tracking

({verified1.7.13, verified1.8.0.2, verified1.8.1})

Trunk
verified1.7.13, verified1.8.0.2, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8.1 +
blocking1.8.0.2 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][rft-dl])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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
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...
(Assignee)

Comment 2

11 years ago
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.
> 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).
(Reporter)

Comment 4

11 years ago
(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.
(Assignee)

Comment 5

11 years ago
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.
(Reporter)

Comment 6

11 years ago
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.
(Reporter)

Comment 7

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

I mean ==, not !=
(Assignee)

Comment 8

11 years ago
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   }
(Assignee)

Comment 9

11 years ago
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.
(Reporter)

Comment 10

11 years ago
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.
Flags: blocking1.8.1+
Flags: blocking1.8.0.2+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical]
(Reporter)

Comment 11

11 years ago
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.
(Reporter)

Comment 12

11 years ago
Created attachment 210450 [details]
linux PoC
(Reporter)

Comment 13

11 years ago
Created attachment 210451 [details]
windows PoC
(Assignee)

Comment 14

11 years ago
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.)
(Assignee)

Comment 15

11 years ago
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.
Attachment #210467 - Flags: superreview?(bzbarsky)
Attachment #210467 - Flags: review?(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
Attachment #210467 - Flags: superreview?(bzbarsky)
Attachment #210467 - Flags: superreview+
Attachment #210467 - Flags: review?(bzbarsky)
Attachment #210467 - Flags: review+
(Assignee)

Comment 17

11 years ago
Created attachment 210957 [details] [diff] [review]
updated patch
Assignee: nobody → rbs
Status: NEW → ASSIGNED
(Assignee)

Comment 18

11 years ago
Checked in the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 19

11 years ago
dveditz agree to check this one in for rds
(Reporter)

Comment 20

11 years ago
Created attachment 211757 [details] [diff] [review]
"updated patch" merged to 1.7 branch
Attachment #210467 - Attachment is obsolete: true
(Reporter)

Comment 21

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

carrying over r/sr=bz, seeking a=
Attachment #211757 - Flags: superreview+
Attachment #211757 - Flags: review+
Attachment #211757 - Flags: approval1.7.13?
Attachment #211757 - Flags: approval-aviary1.0.8?
(Reporter)

Comment 22

11 years ago
Comment on attachment 210957 [details] [diff] [review]
updated patch

This is the one for the 1.8 branches
Attachment #210957 - Flags: approval1.8.0.2?
Attachment #210957 - Flags: approval-branch-1.8.1?(bzbarsky)
Attachment #210957 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+

Comment 23

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

a=timr for drivers.
Attachment #211757 - Flags: approval1.7.13?
Attachment #211757 - Flags: approval1.7.13+
Attachment #211757 - Flags: approval-aviary1.0.8?
Attachment #211757 - Flags: approval-aviary1.0.8+
(Reporter)

Comment 24

11 years ago
attachment 211757 [details] [diff] [review] checked into aviary101/moz17 branches
Keywords: fixed-aviary1.0.8, fixed1.7.13
(Reporter)

Comment 25

11 years ago
Attachment 210957 [details] [diff] checked into the 1.8.0 and 1.8 branches
Keywords: fixed1.8, fixed1.8.0.2
(Reporter)

Comment 26

11 years ago
Comment on attachment 210957 [details] [diff] [review]
updated patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210957 - Flags: approval1.8.0.2? → approval1.8.0.2+
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.
Keywords: fixed-aviary1.0.8, fixed1.7.13 → verified-aviary1.0.8, verified1.7.13

Updated

11 years ago
Flags: testcase+

Updated

11 years ago
Whiteboard: [sg:critical] → [sg:critical][rft-dl]

Comment 28

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

Comment 29

11 years ago
(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

11 years ago
(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

11 years ago
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?
Keywords: fixed1.8.0.2 → verified1.8.0.2
(Reporter)

Updated

11 years ago
Alias: zdi-can-015
Summary: CSS Letter-Spacing Heap Overflow Vulnerability (ZDI-CAN-015) → CSS Letter-Spacing Heap Overflow Vulnerability (ZDI-06-010)
(Reporter)

Updated

11 years ago
Keywords: fixed1.8 → fixed1.8.1
(Reporter)

Updated

11 years ago
Group: security

Comment 32

11 years ago
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

Keywords: fixed1.8.1 → verified1.8.1

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.