Closed Bug 284219 Opened 16 years ago Closed 14 years ago

check SetLength+BeginWriting users for potential buffer overruns

Categories

(Core :: String, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta5

People

(Reporter: darin.moz, Assigned: dveditz)

Details

(Keywords: fixed1.8.0.5, fixed1.8.1, Whiteboard: [sg:critical?][need testcase] no demonstrated cases)

Attachments

(2 files, 1 obsolete file)

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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta2
Whiteboard: [sg:investigation]
Priority: -- → P1
Target Milestone: mozilla1.8beta2 → mozilla1.8beta5
Flags: blocking1.8rc1?
Johnny, we need you :-)  Can you audit this stuff and make sure we're safe?
Assignee: darin → jst
Status: ASSIGNED → NEW
Flags: blocking1.8rc1? → blocking1.8rc1-
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.
anyone know if progress has been made on this since last year?
Not by me.
We really need to do this sooner rather than later.
Assignee: jst → dveditz
Flags: blocking1.8.0.3?
Gael Delalleeau talks more about the ReplacePrep problem in this slide deck preso

http://cansecwest.com/core05/memory_vulns_delalleau.pdf
p43 of the slide deck
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.
(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?
(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?
> 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] = ...
  }
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Whiteboard: [sg:investigation] → [sg:investigate]
people don't always want to change the length. consider callers who want to use strtok.
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
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.
Flags: blocking1.8.0.4+ → blocking1.8.0.5+
Attached patch Chec after SetLength (obsolete) — Splinter Review
Attachment #226333 - Flags: superreview?(darin)
Attachment #226333 - Flags: review?(darin)
Attachment #226333 - Flags: approval1.8.1?
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.
> 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.
Attachment #226333 - Attachment is obsolete: true
Attachment #226333 - Flags: superreview?(darin)
Attachment #226333 - Flags: review?(darin)
Attachment #226333 - Flags: approval1.8.1?
Attachment #226333 - Attachment is obsolete: true
Attachment #226473 - Flags: superreview?
Attachment #226473 - Flags: review?(darin)
Attachment #226473 - Flags: approval1.8.1?
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
Attachment #226473 - Flags: superreview?
Attachment #226473 - Flags: superreview+
Attachment #226473 - Flags: review?(darin)
Attachment #226473 - Flags: review+
(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 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).
Attachment #226473 - Attachment description: v2 → v2 (1.8/1.8.0 branch)
Attachment #226473 - Flags: approval1.8.0.5?
Comment on attachment 226473 [details] [diff] [review]
v2 (1.8/1.8.0 branch)

approved for 1.8.0 branch, a=jay for drivers.
Attachment #226473 - Flags: approval1.8.0.5? → approval1.8.0.5+
Attachment #226473 - Flags: approval1.8.1? → approval1.8.1+
Fix checked into trunk and 1.8 branch
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Fix checked into 1.8.0 branch
Keywords: fixed1.8.0.5
Whiteboard: [sg:investigate] → [sg:investigate][need testcase]
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Whiteboard: [sg:investigate][need testcase] → [sg:critical?][need testcase] no demonstrated cases
Attached patch 1.0.x backportSplinter Review
Flags: in-testsuite-
Group: security
You need to log in before you can comment on or make changes to this bug.