307537 - Crash on cursor motion in text entry due to overrun of clusterBuffer

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Ian Jackson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050513 Debian/1.7.8-1
Build Identifier: firefox (1.0.6-1ubuntu10) breezy and others

Bug was originally reported to ubuntu in our patched 1.0.6, as
http://bugzilla.ubuntu.com/buglist.cgi?cmdtype=runnamed&namedcmd=Todo1

I have checked the source from Debian's archive and the bug is also in
mozilla-firefox_1.0.99+deerpark-alpha2.orig.tar.gz which I assume is the same as
your original upstream tarball.

Reproducible: Always

Steps to Reproduce:
1. Visit the provided URL.
2. Left-click somewhere in the middle of the text in the final text box.
3. Press the left arrow key.

Actual Results:  
Firefox takes a segmentation fault.

Expected Results:  
Moved the cursor left.

This patch fixes the problem, and I believe it to be correct:

--- orig/firefox-1.0.6/layout/html/base/src/nsTextFrame.cpp     2005-09-08
19:11:43.000000000 +0100
+++ firefox-1.0.6/layout/html/base/src/nsTextFrame.cpp  2005-09-08
18:07:18.000000000 +0100
@@ -4008,12 +4008,6 @@
   }
   PRInt32* ip = indexBuffer.mBuffer;
 
-  nsAutoIndexBuffer clusterBuffer;
-  rv = clusterBuffer.GrowTo(mContentLength + 1);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
   PRInt32 textLength;
   nsresult result(NS_ERROR_FAILURE);
   aPos->mResultContent = mContent;//do this right off
@@ -4055,6 +4049,12 @@
       nsTextTransformer tx(doc->GetLineBreaker(), nsnull, aPresContext);
       PrepareUnicodeText(tx, &indexBuffer, &paintBuffer, &textLength);
 
+      nsAutoIndexBuffer clusterBuffer;
+      rv = clusterBuffer.GrowTo(textLength + 1);
+      if (NS_FAILED(rv)) {
+       return rv;
+      }
+
       nsIFrame *frameUsed = nsnull;
       PRInt32 start;
       PRBool found = PR_TRUE;

Comment 1

[Adam Guthrie]

Could you provide a talkback ID for the crash? We might already have another bug
opened for this. Also, WFM in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1)
Gecko/20050907 Firefox/1.6a1 ID:2005090711

Comment 2

[Mats Palmgren (inactive)]

Reproduced in a trunk debug build on Linux. Patch coming up...

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

1. make FillClusterBuffer() grow the result buffer as needed
2. fix OOM crasher on CreateRenderingContext() and propagate the result code
   from GetClusterInfo()
3. remove 4 unused variables that the compiler warns about

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 195591 [details] [diff] [review]
Patch rev. 1

This seems to be for pango-enabled builds only, right, since they're the only
ones that implement GetClusterInfo.

>+  nsresult rv = aClusterBuffer.GrowTo(aLength + 1);

It looks (from nsIRenderingContext and the pango GetClusterInfo implementation)
like this only needs to be |aLength|.  Do double-check that, though. ;-)

>-    shell->CreateRenderingContext(this, getter_AddRefs(acx));
>+    rv = shell->CreateRenderingContext(this, getter_AddRefs(acx));
>+    NS_ENSURE_SUCCESS(rv, rv);
> 
>     // Find the font metrics for this text
>     SetFontFromStyle(acx, mStyleContext);
> 
>-    if (acx)
>-      acx->GetHints(clusterHint);
>+    acx->GetHints(clusterHint);

It might be worth double-checking that CreateRenderingContext implementations
don't return NS_OK and a null pointer.

r+sr=dbaron

Comment 5

[Mats Palmgren (inactive)]

(In reply to comment #4)
> (From update of attachment 195591 [details] [diff] [review] [edit])
> This seems to be for pango-enabled builds only, right, since they're
> the only ones that implement GetClusterInfo.

The crash is for all builds. The potential exploit is probably only for
platforms that implement GetClusterInfo() since otherwise we will
overwrite with 1's (one or more) which seems more difficult to use...
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextFrame.cpp&rev=1.520&root=/cvsroot&mark=1673#1650

> >+  nsresult rv = aClusterBuffer.GrowTo(aLength + 1);
> 
> It looks ... like this only needs to be |aLength|.

Agreed.

BTW, looking at nsFontMetricsPango.cpp it doesn't seem to cope
with embedded NULs, this looks like an UMR on attrs[] for example:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/gtk/nsFontMetricsPango.cpp&rev=1.18&root=/cvsroot&mark=904,916,923,929#893


> >     // Find the font metrics for this text
> >     SetFontFromStyle(acx, mStyleContext);
> > 
> >-    if (acx)
> >-      acx->GetHints(clusterHint);
> >+    acx->GetHints(clusterHint);
> 
> It might be worth double-checking that CreateRenderingContext implementations
> don't return NS_OK and a null pointer.

We wouldn't reach that line anyway because SetFontFromStyle() would have
already crashed nicely for us ;-)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrame.cpp&rev=3.587&root=/cvsroot&mark=434#427

I checked all CreateRenderingContext() and they seemed ok.

Comment 6

[Mats Palmgren (inactive)]

Removed the '+ 1' and checked in to trunk at 2005-09-11 18:05 PDT

Comment 7

[Mats Palmgren (inactive)]

Attachment: Final patch for 1.8 branch

This patch includes the '+ 1' removal and the "unused variable" compile warning

cleanups is not included in this version since those did not exist on branch.

Checked in to MOZILLA_1_8_BRANCH at 2005-09-13 21:21 PDT

-> FIXED

Comment 8

[Ian Jackson]

Can this bug now not be made public now ?  The Debian firefox maintainer would like to see it ...

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 196005 [details] [diff] [review]
Final patch for 1.8 branch

a=dveditz for drivers, please add fixed-aviary1.0.8 and fixed1.7.13 keywords when checked in.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 196005 [details] [diff] [review]
Final patch for 1.8 branch

This patch is not needed on the older branches.
I was about to apply the patch to check it in but discovered
that the FillClusterBuffer method does not exist.
It was first added by bug 260663 so I don't think 1.7.13/1.0.8
is affected by this bug. I did a build from the MOZILLA_1_7_BRANCH
and verified I didn't crash on the URL.

Comment 11

[Daniel Veditz [:dveditz]]

Removing blocking flags from the old branches