1318766 - Write beyond bounds caused by nsTSubstringTuple_CharT::Length()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[q1]

nsTSubstringTuple_CharT (xpcom\string\nsTSubstringTuple.h/.cpp), which represents binary trees of string fragments, can experience an integer overflow in nsTSubstringTuple_CharT::Length(), which computes the total length of a tree of string fragments. If Length() overflows, its callers can fail to allocate sufficient space to contain the concatenation of the string fragments, causing a write beyond bounds. I have found two susceptible callers: nsTSubstring_CharT::Assign(const substring_tuple_type &, fallible_t) and nsTSubstring_CharT::Replace(..., const substring_tuple_type &).

I have not yet found a caller of Assign() or Replace() that definitely can call one of those functions with a nsTSubstringTuple_CharT whose total length is long enough to cause the overflow, but nsTSubstringTuple_CharT is widely used, and I expect that such a caller exists. (BTW, the maximum-allowed length of |nsCString|s and |nsAString|s is 0x7ffffff5 characters and 0x3ffffff9 characters, respectively [1], so an overflow can be caused by only 3 |nsCString| fragments of maximum length) or 5 |nsAString| fragments of maximum length).


Details
-------

The bug is in Length(), which computes the total length of the tree's fragments into |len| without checking for overflow; presumably it should OOM_CRASH on overflow:

12: nsTSubstringTuple_CharT::size_type
13: nsTSubstringTuple_CharT::Length() const
14: {
15:   uint32_t len;
16:   if (mHead) {
17:     len = mHead->Length();
18:   } else {
19:     len = TO_SUBSTRING(mFragA).Length();
20:   }
21: 
22:   return len + TO_SUBSTRING(mFragB).Length();
23: }

The bug then propagates into, e.g., nsTSubstring_CharT::Assign(const substring_tuple_type &, fallible_t) (xpcom\string\nsTSubstring.h/.cpp), which uses Length() to compute the total length of the string that would result from concatenating all of the fragments in the source nsTSubstringTuple_CharT (line 460), and MutatePrep() (line 465) to allocate that amount of space:

451: bool
452: nsTSubstring_CharT::Assign(const substring_tuple_type& aTuple,
453:                            const fallible_t& aFallible)
454: {
455:   if (aTuple.IsDependentOn(mData, mData + mLength)) {
456:     // take advantage of sharing here...
457:     return Assign(string_type(aTuple), aFallible);
458:   }
459: 
460:   size_type length = aTuple.Length();
461: 
462:   // don't use ReplacePrep here because it changes the length
463:   char_type* oldData;
464:   uint32_t oldFlags;
465:   if (!MutatePrep(length, &oldData, &oldFlags)) {
466:     return false;
467:   }
468: 
469:   if (oldData) {
470:     ::ReleaseData(oldData, oldFlags);
471:   }
...

Assign() then uses nsTSubstringTuple_CharT::WriteTo() to write the data into the buffer |mData|:

472: 
473:   aTuple.WriteTo(mData, length);
474:   mData[length] = 0;
475:   mLength = length;
476:   return true;
477: }

But nsTSubstringTuple_CharT::WriteTo() does not check (except in debug builds) whether |mData| is long enough to contain the data, so }headLen| on line 38 can underflow, and |a.Length()| (line 45) and |b.Length()| (line 48) are used to copy fragments into the buffer without checking whether it is large enough to contain them:

32: void
33: nsTSubstringTuple_CharT::WriteTo(char_type* aBuf, uint32_t aBufLen) const
34: {
35:   const substring_type& b = TO_SUBSTRING(mFragB);
36: 
37:   NS_ASSERTION(aBufLen >= b.Length(), "buffer too small");
38:   uint32_t headLen = aBufLen - b.Length();
39:   if (mHead) {
40:     mHead->WriteTo(aBuf, headLen);
41:   } else {
42:     const substring_type& a = TO_SUBSTRING(mFragA);
43: 
44:     NS_ASSERTION(a.Length() == headLen, "buffer incorrectly sized");
45:     char_traits::copy(aBuf, a.Data(), a.Length());
46:   }
47: 
48:   char_traits::copy(aBuf + headLen, b.Data(), b.Length());
49: 
50: #if 0
...
69: #endif
70: }

[1] Both limits are enforced by nsTSubstring_CharT::MutatePrep().

Comment 1

[Eric Rahm [:erahm]]

Looks like the main problem here is not release asserting the buffer size in |WriteTo|. The overflow in |Length| is also an issue, but if we fix |WriteTo| we'd just end up with a truncated (but still null terminated) string.

Exploitablity seems feasible anywhere we're concatenating strings from user content -- I'm reasonably sure nsTStringTuple is only ever used for concatenating strings, ie: |nsCString foo = bar + baz + qux| would end up constructing |foo| with a tuple of (bar,baz,qux).

As an aside: calculating |b.Length()| 3 times in |WriteTo| seems pretty excessive.

Comment 2

[q1]

(In reply to Eric Rahm [:erahm] from comment #1)
> Looks like the main problem here is not release asserting the buffer size in
> |WriteTo|. The overflow in |Length| is also an issue, but if we fix
> |WriteTo| we'd just end up with a truncated (but still null terminated)
> string.

It seems that that truncated string might sometimes cause FF to do something insecure, such as loading a resource from a domain or filename that is a truncated form of the intended domain or filename.

Comment 3

[Eric Rahm [:erahm]]

So the easiest solution is to add a release assert to WriteTo/Length. A more elaborate solution would be to propagate errors from |Length| and |WriteTo|, I'll take a look at how involved that would be, it might make sense to make that a follow up.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Validate length in SubstringTuple

This is the minimal fix which I'd feel good about uplifting.

MozReview-Commit-ID: JuwQS8jpKcX

Comment 5

[q1]

(In reply to Eric Rahm [:erahm] from comment #4)
> Created attachment 8815434 [details] [diff] [review]
> Validate length in SubstringTuple
> 
> This is the minimal fix which I'd feel good about uplifting.
> 
> MozReview-Commit-ID: JuwQS8jpKcX

I think the assertion on line 44 also should become a release assert.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Validate length in SubstringTuple

This is the simple fix with an additional assert.

MozReview-Commit-ID: JuwQS8jpKcX

Comment 7

[Eric Rahm [:erahm]]

Attachment: Validate length in SubstringTuple

This is a more elaborate fix that allows for fallibility.

Comment 8

[q1]

(In reply to Eric Rahm [:erahm] from comment #7)
> Created attachment 8815512 [details] [diff] [review]
> Validate length in SubstringTuple
> 
> This is a more elaborate fix that allows for fallibility.

I have two comments:

1. Check the status from the call to WriteTo() in nsTSubstring_CharT::Assign(const substring_tuple_type&, const fallible_t&);

2. Check or purposely ignore (e.g., cast to void) the status from the call to WriteTo() in nsTSubstring_CharT::Replace(index_type, size_type, const substring_tuple_type&);

Comment 9

[Eric Rahm [:erahm]]

FWIW I prefer the simple case, I'd rather do a larger rewrite in a follow up.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8815511 [details] [diff] [review]
Validate length in SubstringTuple

Review of attachment 8815511 [details] [diff] [review]:
-----------------------------------------------------------------

I'm inclined to land this one.  We can land the more invasive fix, possibly including q1's comments, later.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8815512 [details] [diff] [review]
Validate length in SubstringTuple

Review of attachment 8815512 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think there's anything wrong with this patch, but r+'ing two patches to do the same thing in the same bug is confusing. ;)

::: xpcom/string/nsTSubstring.cpp
@@ +475,5 @@
>  
> +  const auto length = aTuple.Length();
> +  if (!length.isValid()) {
> +    return false;
> +  }

Almost wonder if it'd be better to:

const auto checkedLength = aTuple.Length();
// check validity
const auto length = checkedLength.value();

and change less code below.

Comment 12

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d35117ca7509b057b8fc57e210f147617a13a493
Bug 1318766 - Validate length in SubstringTuple. r=froydnj

Comment 13

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> Comment on attachment 8815512 [details] [diff] [review]
> Validate length in SubstringTuple
> 
> Review of attachment 8815512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think there's anything wrong with this patch, but r+'ing two patches
> to do the same thing in the same bug is confusing. ;)

Yeah makes sense, I just wanted to show what it would look like. Just changing nsTSubstringTuple is going to be much easier to uplift.

Comment 14

[Nathan Froyd [:froydnj]]

Almost positive that everywhere is affected by this one.

[Tracking Requested - why for this release]: security bugs should be uplifted.

Comment 15

[Andrew McCreight [:mccr8]]

I'm going to mark this sec-moderate because the threat of an overflow from content feels a little speculative, but maybe it should be higher.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d35117ca7509

Comment 17

[Eric Rahm [:erahm]]

Comment on attachment 8815511 [details] [diff] [review]
Validate length in SubstringTuple

Approval Request Comment
[Feature/Bug causing the regression]:

N/A, been around for a decade.

[User impact if declined]:

Possible OOB write triggered by large user content. We haven't seen this in practice, currently it's theoretical.

[Is this code covered by automated tests?]:

This involves string code which is widely used and tested.

[Has the fix been verified in Nightly?]:

Again the exploit is theoretical. Has baked a few days.

[Needs manual test from QE? If yes, steps to reproduce]: 

No.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

It just adds release asserts.

[String changes made/needed]:

None.

Comment 18

[Gerry Chang [:gchang]]

Track 51+ as sec-moderate.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8815511 [details] [diff] [review]
Validate length in SubstringTuple

Fix sec-moderate. Beta51+ and Aurora52+. Should be in 51 beta 7.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2258b6da167f

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/084cde31d06d

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Too late for 50 and doesn't match the esr criteria.