The default bug view has changed. See this FAQ.

check SetLength+BeginWriting users for potential buffer overruns

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
String
P1
critical
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Darin Fisher, Assigned: dveditz)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
mozilla1.8beta5
fixed1.8.0.5, fixed1.8.1
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8rc1 -
blocking1.8.0.5 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][need testcase] no demonstrated cases)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta2
(Assignee)

Updated

12 years ago
Whiteboard: [sg:investigation]
(Reporter)

Updated

12 years ago
Priority: -- → P1
(Reporter)

Updated

12 years ago
Target Milestone: mozilla1.8beta2 → mozilla1.8beta5
(Reporter)

Updated

12 years ago
Flags: blocking1.8rc1?

Comment 1

12 years ago
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-
(Reporter)

Comment 2

11 years ago
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

11 years ago
anyone know if progress has been made on this since last year?
(Reporter)

Comment 4

11 years ago
Not by me.
(Assignee)

Comment 5

11 years ago
We really need to do this sooner rather than later.
Assignee: jst → dveditz
Flags: blocking1.8.0.3?

Comment 6

11 years ago
Gael Delalleeau talks more about the ReplacePrep problem in this slide deck preso

http://cansecwest.com/core05/memory_vulns_delalleau.pdf

Comment 7

11 years ago
p43 of the slide deck
(Reporter)

Comment 8

11 years ago
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?
(Reporter)

Comment 11

11 years ago
> 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] = ...
  }
(Assignee)

Updated

11 years ago
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
(Reporter)

Comment 14

11 years ago
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.
(Assignee)

Updated

11 years ago
Flags: blocking1.8.0.4+ → blocking1.8.0.5+
(Assignee)

Comment 15

11 years ago
Created attachment 226333 [details] [diff] [review]
Chec after SetLength
Attachment #226333 - Flags: superreview?(darin)
Attachment #226333 - Flags: review?(darin)
Attachment #226333 - Flags: approval1.8.1?
(Reporter)

Comment 16

11 years ago
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.
(Assignee)

Comment 17

11 years ago
> 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.
(Assignee)

Updated

11 years ago
Attachment #226333 - Attachment is obsolete: true
Attachment #226333 - Flags: superreview?(darin)
Attachment #226333 - Flags: review?(darin)
Attachment #226333 - Flags: approval1.8.1?
(Assignee)

Comment 18

11 years ago
Created attachment 226473 [details] [diff] [review]
v2 (1.8/1.8.0 branch)
Attachment #226333 - Attachment is obsolete: true
Attachment #226473 - Flags: superreview?
Attachment #226473 - Flags: review?(darin)
Attachment #226473 - Flags: approval1.8.1?
(Reporter)

Comment 19

11 years ago
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);
}
(Assignee)

Comment 21

11 years ago
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 22

11 years ago
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+

Updated

11 years ago
Attachment #226473 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 23

11 years ago
Fix checked into trunk and 1.8 branch
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(Assignee)

Comment 24

11 years ago
Fix checked into 1.8.0 branch
Keywords: fixed1.8.0.5

Updated

11 years ago
Whiteboard: [sg:investigate] → [sg:investigate][need testcase]
(Assignee)

Updated

11 years ago
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
(Assignee)

Updated

11 years ago
Whiteboard: [sg:investigate][need testcase] → [sg:critical?][need testcase] no demonstrated cases

Comment 25

11 years ago
Created attachment 232736 [details] [diff] [review]
1.0.x backport

Updated

11 years ago
Flags: in-testsuite-
(Assignee)

Updated

10 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.