Closed Bug 615147 Opened 9 years ago Closed 9 years ago

Integer overflow leads to incorrect buffer size in nsTSubstring_CharT::MutatePrep

Categories

(Core :: String, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- .17+
status1.9.2 --- .17-fixed
blocking1.9.1 --- .19+
status1.9.1 --- .19-fixed

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: testcase, Whiteboard: [sg:critical?][hardblocker] [qa-examined-192])

Attachments

(3 files, 2 obsolete files)

As neil@parkwaycc.co.uk noted in bug 615112 comment 5:
>If the above is a problem then this line from MutatePrep is too:
>size_type storageSize = (capacity + 1) * sizeof(char_type);

This is indeed a bug...
Assignee: matspal → nobody
Component: XPCOM → String
QA Contact: xpcom → string
Assignee: nobody → matspal
Attached patch Patch rev. 1 (obsolete) — Splinter Review
There are two cases of integer overflow in MutatePrep:
http://hg.mozilla.org/mozilla-central/annotate/c194928bb4a9/xpcom/string/src/nsTSubstring.cpp#l73
1. the current capacity is zero and the requested capacity is size_type(-1)/2
2. the current capacity is in the range 1G .. 2G and the requested capacity
   is larger

For 1. the test on line 84 is false since it doesn't test equality and
we fall through to line 125 which overflows.
For 2. we double the current capacity on line 108, which makes 'capacity'
larger than 2G

There's also an integer overflow in nsStringBuffer::Realloc:
http://hg.mozilla.org/mozilla-central/annotate/c194928bb4a9/xpcom/string/src/nsSubstring.cpp#l219
3. when the requested capacity is near 2G, resulting in storageSize near 4G,
   this leads to overflow of nsStringBuffer::mStorageSize (PRUint32) when
   sizeof(nsStringBuffer) + size > 4G (the realloc is ok since it uses size_t,
   but mStorageSize will be something small)

I've added tests that triggers the above cases in TestStrings.cpp
(1. and 2. crashes the test without the fix)

I also removed the redundant "if (curCapacity > 0)" -- it's
inside a "if (curCapacity != 0)" and curCapacity's type is unsigned.
Attachment #493635 - Flags: review?(dbaron)
Actually, TestStrings and many more unit tests are disabled in libxul
builds...  I filed bug 615163 on that.
blocking1.9.2: --- → ?
status1.9.1: --- → ?
blocking1.9.2: ? → ---
status1.9.2: --- → ?
> the realloc is ok since it uses size_t

Well, at least on 64-bit and 32-bit Linux, which I tested has
sizeof(size_t) == 8.
blocking2.0: --- → ?
The last comment is incorrect, sizeof(size_t) == 4 on 32-bit Linux.
So, the addition in nsStringBuffer::Realloc can potentially overflow.
It seems MutatePrep is the only caller though, and that should now be safe.
Attached patch Patch rev. 2 (obsolete) — Splinter Review
Same fix as before, just updated the test to ignore allocation failures
on 32-bit platforms (since it'll likely exhaust the address space).
It's mostly a crash/assertion test anyway.
Attachment #493635 - Attachment is obsolete: true
Attachment #493889 - Flags: review?(dbaron)
Attachment #493635 - Flags: review?(dbaron)
Whiteboard: [sg:critical?]
Comment on attachment 493889 [details] [diff] [review]
Patch rev. 2

>diff --git a/xpcom/string/src/nsSubstring.cpp b/xpcom/string/src/nsSubstring.cpp
>+    NS_ASSERTION(size_t(PRUint32(-1)) >= size_t(sizeof(nsStringBuffer) + size),
>+                 "mStorageSize will truncate");

This would be clearer with the arguments in the other order (and <= instead of >=).  Also, there's no need to cast sizeof(nsStringBuffer) + size to size_t.


>diff --git a/xpcom/string/src/nsTSubstring.cpp b/xpcom/string/src/nsTSubstring.cpp
>+    const size_type kMaxCapacity =
>+      size_type(-1)/2 - (sizeof(nsStringBuffer)/sizeof(char_type) + 1) - 1;

It's not clear to me what the double -1 is for.

You:
 * want to account for the null terminator (should be -sizeof(char_type))
 * want to be less than 2GB, not equal to (should be -1)
 * maybe you want to handle sizeof(nsStringBuffer) not being a multiple of sizeof(char_type)

But you're also not accounting for storageSize below being computed as:
  storageSize = (capacity + 1) * sizeof(char_type)

which means that this will allow nsString buffers to be up to 4GB, but nsCString buffers up to 2GB, which doesn't make much sense to me.  Is that what you intended?

>+        capacity = temp;
>+        if (capacity > kMaxCapacity) {
>+          // We know the requested capacity cannot have been larger than this
>+          // given the early return at the top.
>+          capacity = kMaxCapacity;
>+        }

I think this would be clearer as:

NS_ASSERTION(NS_MIN(temp, kMaxCapacity) >= capacity,
             "should have hit the early return at the top");
capacity = NS_MIN(temp, kMaxCapacity);
Attachment #493889 - Flags: review?(dbaron) → review-
> It's not clear to me what the double -1 is for.
>  * maybe you want to handle sizeof(nsStringBuffer) not being a multiple of
> sizeof(char_type)

Yes, that.

> which means that this will allow nsString buffers to be up to 4GB, but
> nsCString buffers up to 2GB, which doesn't make much sense to me.
> Is that what you intended?

Yes, I intended to fix integer overflow bugs only, not change any other
behavior such as the one you mention.  I agree it doesn't make sense though.
Ok, so let's fix that too.
Attached patch Patch rev. 3Splinter Review
Make the max size 2GB minus meta data and null terminator.
I included your other suggestions too.

FYI, the "NS_LogInit()" in the test is to avoid ~250 lines of:
WARNING: XPCOM objects created/destroyed from static ctor/dtor ...
Attachment #493889 - Attachment is obsolete: true
Attachment #495165 - Flags: review?(dbaron)
blocking2.0: ? → final+
Comment on attachment 495165 [details] [diff] [review]
Patch rev. 3

>diff --git a/xpcom/string/src/nsSubstring.cpp b/xpcom/string/src/nsSubstring.cpp

> nsStringBuffer*
> nsStringBuffer::Alloc(size_t size)
>   {
>     NS_ASSERTION(size != 0, "zero capacity allocation not allowed");
>+    NS_ASSERTION(sizeof(nsStringBuffer) + size <= size_t(PRUint32(-1)),
>+                 "mStorageSize will truncate");

You should actually && this with:

  sizeof(nsStringBuffer) + size > size


> nsStringBuffer*
> nsStringBuffer::Realloc(nsStringBuffer* hdr, size_t size)

same here

>diff --git a/xpcom/string/src/nsTSubstring.cpp b/xpcom/string/src/nsTSubstring.cpp
> nsTSubstring_CharT::MutatePrep( size_type capacity, char_type** oldData, PRUint32* oldFlags )
>   {
>     // initialize to no old data
>     *oldData = nsnull;
>     *oldFlags = 0;
> 
>     size_type curCapacity = Capacity();
> 
>-    // If |capacity > size_type(-1)/2|, then our doubling algorithm may not be
>+    // If |capacity > kMaxCapacity|, then our doubling algorithm may not be
>     // able to allocate it.  Just bail out in cases like that.  We don't want
>     // to be allocating 2GB+ strings anyway.
>-    if (capacity > size_type(-1)/2) {
>+    PR_STATIC_ASSERT((sizeof(nsStringBuffer) & 0x1) == 0);
>+    const size_type kMaxCapacity =
>+      (size_type(-1)/2 - sizeof(nsStringBuffer)) / sizeof(char_type) - 1;


If you change this to -2 rather than -1, then I think we're good.  (I think we need to account for the null terminator and set the maximum *under* 2^31.)


r=dbaron with that.  Sorry for the delay in re-reviewing.
Attachment #495165 - Flags: review?(dbaron) → review+
Whiteboard: [sg:critical?] → [sg:critical?][hardblocker]
With review comments addressed:
http://hg.mozilla.org/mozilla-central/rev/5efe5f98ac13
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
This is also good for 1.9.2 branch.
Attachment #503855 - Flags: approval1.9.2.14?
Attached patch for 1.9.1Splinter Review
This is for 1.9.1, on this branch we need to keep the "curCapacity > 0" check.
Compare the (current) 1.9.2 vs 1.9.1 code:
http://mxr.mozilla.org/mozilla1.9.2/source/xpcom/string/src/nsTSubstring.cpp#114
http://mxr.mozilla.org/mozilla1.9.1/source/xpcom/string/src/nsTSubstring.cpp#114

Also, I'm skipping the unit test on this branch because SetCapacity()
is void (and Capacity() is protected) so it's not possible to know
if it succeeded or not.
Attachment #503860 - Flags: review?(dbaron)
Attachment #503860 - Flags: approval1.9.1.17?
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking1.9.1: ? → .18+
blocking1.9.2: ? → .15+
Comment on attachment 503855 [details] [diff] [review]
Patch rev. 4 (landed on mozilla-central)

moving approvals to next cycle, already in end-game for 1.9.2.14 and 1.9.1.17
Attachment #503855 - Flags: approval1.9.2.15?
Attachment #503855 - Flags: approval1.9.2.14?
Attachment #503855 - Flags: approval1.9.2.14-
Attachment #503860 - Flags: approval1.9.1.18?
Attachment #503860 - Flags: approval1.9.1.17?
Attachment #503860 - Flags: approval1.9.1.17-
Comment on attachment 503860 [details] [diff] [review]
for 1.9.1

r=dbaron
Attachment #503860 - Flags: review?(dbaron) → review+
Comment on attachment 503860 [details] [diff] [review]
for 1.9.1

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers
Attachment #503860 - Flags: approval1.9.1.18? → approval1.9.1.18+
Attachment #503855 - Flags: approval1.9.2.15? → approval1.9.2.15+
The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.
Is there a reason that tests were checked in for 1.9.2? Normally, we don't check these in until after release for security bugs.
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker] [qa-examined-192]
(In reply to comment #18)
> Is there a reason that tests were checked in for 1.9.2? Normally, we don't
> check these in until after release for security bugs.

They we're pushed to m-c before that.  The reason is that they don't reveal
anything beyond what that the code changes themselves already reveal.
Group: core-security
You need to log in before you can comment on or make changes to this bug.