284219 - check SetLength+BeginWriting users for potential buffer overruns

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Darin Fisher]

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

Johnny, we need you :-)  Can you audit this stuff and make sure we're safe?

Comment 2

[Darin Fisher]

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]

anyone know if progress has been made on this since last year?

Comment 4

[Darin Fisher]

Not by me.

Comment 5

[Daniel Veditz [:dveditz]]

We really need to do this sooner rather than later.

Comment 6

[chris hofmann]

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]

p43 of the slide deck

Comment 8

[Darin Fisher]

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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

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

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

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

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]

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

Attachment: Chec after SetLength

Comment 16

[Darin Fisher]

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

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

Attachment: v2 (1.8/1.8.0 branch)

Comment 19

[Darin Fisher]

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

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

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

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

Fix checked into trunk and 1.8 branch

Comment 24

[Daniel Veditz [:dveditz]]

Fix checked into 1.8.0 branch

Comment 25

[Alexander Sack]

Attachment: 1.0.x backport