Closed
Bug 325403
(zdi-can-015)
Opened 19 years ago
Closed 19 years ago
CSS Letter-Spacing Heap Overflow Vulnerability (ZDI-06-010)
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
People
(Reporter: dveditz, Assigned: rbs)
Details
(Keywords: verified1.7.13, verified1.8.0.2, verified1.8.1, Whiteboard: [sg:critical][rft-dl])
Attachments
(4 files, 1 obsolete file)
1.21 KB,
text/html
|
Details | |
163 bytes,
text/html
|
Details | |
8.57 KB,
patch
|
bzbarsky
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2+
|
Details | Diff | Splinter Review |
8.57 KB,
patch
|
dveditz
:
review+
dveditz
:
superreview+
timr
:
approval-aviary1.0.8+
timr
:
approval1.7.13+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
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...
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•19 years ago
|
||
> 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•19 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.
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•19 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•19 years ago
|
||
> If I could jigger that testcase around so newWordBuf != aWordBuf
I mean ==, not !=
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 }
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•19 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•19 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•19 years ago
|
||
Reporter | ||
Comment 13•19 years ago
|
||
Assignee | ||
Comment 14•19 years ago
|
||
Play the game with somebody else. (no provision for long words. That will break other things anyway.)
Assignee | ||
Comment 15•19 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 16•19 years ago
|
||
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•19 years ago
|
||
Assignee: nobody → rbs
Status: NEW → ASSIGNED
Assignee | ||
Comment 18•19 years ago
|
||
Checked in the trunk.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 19•19 years ago
|
||
dveditz agree to check this one in for rds
Reporter | ||
Comment 20•19 years ago
|
||
Attachment #210467 -
Attachment is obsolete: true
Reporter | ||
Comment 21•19 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•19 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)
Updated•19 years ago
|
Attachment #210957 -
Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Comment 23•19 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•19 years ago
|
||
attachment 211757 [details] [diff] [review] checked into aviary101/moz17 branches
Keywords: fixed-aviary1.0.8,
fixed1.7.13
Reporter | ||
Comment 25•19 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•19 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+
Comment 27•19 years ago
|
||
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.
Updated•19 years ago
|
Flags: testcase+
Updated•19 years ago
|
Whiteboard: [sg:critical] → [sg:critical][rft-dl]
Comment 28•19 years ago
|
||
I crash with Firefox 1.0.8 linux 2006021 on the windowsPoC after closing the security device password dialog.
Comment 29•19 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•19 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•19 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•19 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•19 years ago
|
Keywords: fixed1.8 → fixed1.8.1
Reporter | ||
Updated•19 years ago
|
Group: security
Comment 32•18 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•18 years ago
|
Flags: in-testsuite+ → in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•