Closed
Bug 615147
Opened 14 years ago
Closed 14 years ago
Integer overflow leads to incorrect buffer size in nsTSubstring_CharT::MutatePrep
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla2.0b10
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
Details
(Keywords: testcase, Whiteboard: [sg:critical?][hardblocker] [qa-examined-192])
Attachments
(3 files, 2 obsolete files)
|
7.99 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
|
8.83 KB,
patch
|
dveditz
:
approval1.9.2.14-
dveditz
:
approval1.9.2.17+
|
Details | Diff | Splinter Review |
|
4.31 KB,
patch
|
dbaron
:
review+
dveditz
:
approval1.9.1.17-
dveditz
:
approval1.9.1.19+
|
Details | Diff | Splinter Review |
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
| Assignee | ||
Comment 1•14 years ago
|
||
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)
| Assignee | ||
Comment 2•14 years ago
|
||
Actually, TestStrings and many more unit tests are disabled in libxul
builds... I filed bug 615163 on that.
blocking1.9.2: --- → ?
status1.9.1:
--- → ?
| Assignee | ||
Updated•14 years ago
|
blocking1.9.2: ? → ---
status1.9.2:
--- → ?
| Assignee | ||
Comment 3•14 years ago
|
||
> 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.
Updated•14 years ago
|
blocking2.0: --- → ?
| Assignee | ||
Comment 4•14 years ago
|
||
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.
| Assignee | ||
Comment 5•14 years ago
|
||
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)
Updated•14 years ago
|
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-
| Assignee | ||
Comment 7•14 years ago
|
||
> 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.
| Assignee | ||
Comment 8•14 years ago
|
||
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)
Updated•14 years ago
|
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+
Updated•14 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][hardblocker]
| Assignee | ||
Comment 10•14 years ago
|
||
With review comments addressed:
http://hg.mozilla.org/mozilla-central/rev/5efe5f98ac13
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
| Assignee | ||
Comment 11•14 years ago
|
||
This is also good for 1.9.2 branch.
Attachment #503855 -
Flags: approval1.9.2.14?
| Assignee | ||
Comment 12•14 years ago
|
||
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?
Updated•14 years ago
|
Updated•14 years ago
|
blocking1.9.1: ? → .18+
blocking1.9.2: ? → .15+
Comment 13•14 years ago
|
||
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-
Updated•14 years ago
|
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 15•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #503855 -
Flags: approval1.9.2.15? → approval1.9.2.15+
| Assignee | ||
Comment 16•14 years ago
|
||
Comment 17•14 years ago
|
||
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.
Comment 18•14 years ago
|
||
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.
Updated•14 years ago
|
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker] [qa-examined-192]
| Assignee | ||
Comment 19•14 years ago
|
||
(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.
Updated•14 years ago
|
Group: core-security
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•