Last Comment Bug 284219 - check SetLength+BeginWriting users for potential buffer overruns
: check SetLength+BeginWriting users for potential buffer overruns
Status: RESOLVED FIXED
[sg:critical?][need testcase] no demo...
: fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: String (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.8beta5
Assigned To: Daniel Veditz [:dveditz]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-03-01 00:40 PST by Darin Fisher
Modified: 2007-06-27 20:17 PDT (History)
10 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
asa: blocking1.8rc1-
dveditz: blocking1.8.0.5+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Chec after SetLength (42.03 KB, patch)
2006-06-20 07:23 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
v2 (1.8/1.8.0 branch) (42.04 KB, patch)
2006-06-21 00:23 PDT, Daniel Veditz [:dveditz]
darin.moz: review+
darin.moz: superreview+
jaymoz: approval1.8.0.5+
mconnor: approval1.8.1+
Details | Diff | Splinter Review
1.0.x backport (16.68 KB, patch)
2006-08-08 08:26 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description Darin Fisher 2005-03-01 00:40:35 PST
There's a fair amount of code out there that does something equivalent to:

  nsCString s;
  s.SetLength(n);
  char *p = s.BeginWriting();
  memcpy(p, ... );

Any of these could be potential buffer overruns if an attacker can set n
sufficiently large to trigger an out of memory condition.  That would then
result in p pointing at the static empty buffer, which lives at a fixed known
address, and could then be exploited by an attacker :(

I think we need to ensure that consumers like this verify the success of
SetLength by testing Length() afterwards.  This is similar to the ReplacePrep
problem that was recently fixed.
Comment 1 Asa Dotzler [:asa] 2005-10-12 11:25:29 PDT
Johnny, we need you :-)  Can you audit this stuff and make sure we're safe?
Comment 2 Darin Fisher 2006-03-09 18:17:39 PST
Besides just auditing code, I think we need to do a couple things to help avoid this coding problem.  One big change that would help is to make SetLength return a boolean indicating whether or not the operation succeeded in allocating memory.  Then we could go through the code and ensure that people are checking for that error condition.

Bug 149646 proposes that we abort the process if SetLength fails.  I think handling the error might be better as it is sometimes the case that webcontent can affect the size passed to SetLength.
Comment 3 chris hofmann 2006-03-14 16:59:00 PST
anyone know if progress has been made on this since last year?
Comment 4 Darin Fisher 2006-03-14 17:01:53 PST
Not by me.
Comment 5 Daniel Veditz [:dveditz] 2006-04-13 15:03:49 PDT
We really need to do this sooner rather than later.
Comment 6 chris hofmann 2006-04-13 15:06:56 PDT
Gael Delalleeau talks more about the ReplacePrep problem in this slide deck preso

http://cansecwest.com/core05/memory_vulns_delalleau.pdf
Comment 7 chris hofmann 2006-04-13 15:07:27 PDT
p43 of the slide deck
Comment 8 Darin Fisher 2006-04-13 15:50:18 PDT
The simple solution is to add checks like this:

  http://lxr.mozilla.org/mozilla/source/xpcom/io/nsWinAPIs.cpp#68

But, it might be nicer to make SetLength return PRBool.  It would also be nice to change BeginWriting to take a length parameter, and then have BeginWriting return NULL if the allocation failed.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-04-13 17:16:43 PDT
(In reply to comment #8)
> But, it might be nicer to make SetLength return PRBool.

Agreed.

> It would also be nice
> to change BeginWriting to take a length parameter, and then have BeginWriting
> return NULL if the allocation failed.

I see why you'd want it to return null (which the callers would have to check), but why take a length parameter?
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-04-13 17:17:50 PDT
(In reply to comment #9)
> I see why you'd want it to return null (which the callers would have to check),
> but why take a length parameter?

Actually, I'm not so sure.  Should callers be required to SetLength / SetCapacity before BeginWriting, which will force copy-on-write?
Comment 11 Darin Fisher 2006-04-14 08:40:04 PDT
> Actually, I'm not so sure.  Should callers be required to SetLength /
> SetCapacity before BeginWriting, which will force copy-on-write?

The idea is that typically callers want to set the length of the string before mutating it because often they aren't sure if the length of the string is already what they need it to be.  So, the following pattern becomes very common:

  str.SetLength(num);
  if (str.Length() != num)
    return NS_ERROR_OUT_OF_MEMORY;
  char *ptr = str.BeginWriting();
  for (PRUint32 i = 0; i < num; ++i) {
    ptr[i] = ...
  }

I think it would make sense to allow people to have one function that they call that resizes the internal buffer (if necessary) and returns a mutable pointer to it.  Perhaps that means that we could just extend BeginWriting to take an optional length parameter.  If not specified, then the current length of the string would be used.

The optional parameter to BeginWriting is not really necessary, but it just seems like it would be a nice optimization and make the API easier to use:

  char *ptr = str.BeginWriting(num);
  if (!ptr)
    return NS_ERROR_OUT_OF_MEMORY;
  for (PRUint32 i = 0; i < num; ++i) {
    ptr[i] = ...
  }
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-20 02:44:56 PDT
people don't always want to change the length. consider callers who want to use strtok.
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-20 02:47:27 PDT
that said, making BeginWriting take a length parameter would make some code simpler, and, I believe, also resolve the XXX comment at http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTAString.h#351
Comment 14 Darin Fisher 2006-04-29 16:51:36 PDT
Because this bug is a security bug, I'd prefer to move the actually API changing work into a public bug.  See bug 335957.  I didn't make it a dependency for this bug because we should try to fix this bug on the stable branches without introducing new string APIs.
Comment 15 Daniel Veditz [:dveditz] 2006-06-20 07:23:39 PDT
Created attachment 226333 [details] [diff] [review]
Chec after SetLength
Comment 16 Darin Fisher 2006-06-20 10:57:14 PDT
Comment on attachment 226333 [details] [diff] [review]
Chec after SetLength

>Index: mozilla/xpcom/string/public/nsReadableUtils.h

>+inline PRBool StringSetLength(nsAString &aStr, PRUint32 aLen)
>+{
>+    aStr.SetLength(aLen);
>+    if (aStr.Length() != aLen)
>+        return PR_FALSE;
>+    return PR_TRUE;

nit:  return aStr.Length() == aLen;


>+inline PRBool StringSetLength(nsACString &aStr, PRUint32 aLen)
>+{
>+    aStr.SetLength(aLen);
>+    if (aStr.Length() != aLen)
>+        return PR_FALSE;
>+    return PR_TRUE;

Same here.


Perhaps it would be better to use the verb "ensure" instead of "set" ...

   StringEnsureLength or EnsureStringLength or EnsureLength

Might be nice to have something different than SetLength since that's
already a method of nsAString.

I haven't reviewed further.
Comment 17 Daniel Veditz [:dveditz] 2006-06-20 13:21:44 PDT
> Might be nice to have something different than SetLength since that's
> already a method of nsAString.

I somehow assumed your proposed API changes would include a change to the SetLength signature, and then we could replace all the StringSetLength() back to SetLength. But it looks like you're attacking the other end with a GetWritableBuffer signature change, and in that case EnsureStringLength() sounds better.
Comment 18 Daniel Veditz [:dveditz] 2006-06-21 00:23:40 PDT
Created attachment 226473 [details] [diff] [review]
v2 (1.8/1.8.0 branch)
Comment 19 Darin Fisher 2006-06-21 00:30:57 PDT
Comment on attachment 226473 [details] [diff] [review]
v2 (1.8/1.8.0 branch)

Versions of EnsureStringLength that take nsString and nsCString might be nice since .Length() is inlined nicely on those types.


+    // FIXME: need way to return error
+    if (!EnsureStringLength(aDest, aSource.Length())) {
+      aDest.Truncate();
+      return; // out of memory
+    }
     aDest.SetLength(aSource.Length());

I think you can remove the aDest.SetLength(...) call, right?


r+sr=darin
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2006-06-21 09:19:14 PDT
(In reply to comment #19)
> (From update of attachment 226473 [details] [diff] [review] [edit])
> Versions of EnsureStringLength that take nsString and nsCString might be nice
> since .Length() is inlined nicely on those types.

How about:
template<class T>
PRBool EnsureStringLength(T& aStr, PRUint32 aLength)
{
    aStr.SetLength(aLen);
    return (aStr.Length() == aLen);
}
Comment 21 Daniel Veditz [:dveditz] 2006-06-21 13:50:15 PDT
Comment on attachment 226473 [details] [diff] [review]
v2 (1.8/1.8.0 branch)

This is the 1.8 branch version, there were a few merges required for the trunk (xslt moved, for one).
Comment 22 Jay Patel [:jay] 2006-06-21 14:37:27 PDT
Comment on attachment 226473 [details] [diff] [review]
v2 (1.8/1.8.0 branch)

approved for 1.8.0 branch, a=jay for drivers.
Comment 23 Daniel Veditz [:dveditz] 2006-06-22 12:14:18 PDT
Fix checked into trunk and 1.8 branch
Comment 24 Daniel Veditz [:dveditz] 2006-06-23 00:58:14 PDT
Fix checked into 1.8.0 branch
Comment 25 Alexander Sack 2006-08-08 08:26:27 PDT
Created attachment 232736 [details] [diff] [review]
1.0.x backport

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