Write beyond bounds caused by nsTSubstringTuple_CharT::Length()

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: q1, Assigned: erahm)

Tracking

({csectype-intoverflow, sec-moderate})

49 Branch
mozilla53
Points:
---
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(firefox-esr45- wontfix, firefox50+ wontfix, firefox51+ fixed, firefox52+ fixed, firefox-esr52 fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

3 years ago
Component: XPCOM → String
Group: core-security → dom-core-security
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.
(Reporter)

Comment 2

2 years ago
(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.
Flags: sec-bounty?
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.
This is the minimal fix which I'd feel good about uplifting.

MozReview-Commit-ID: JuwQS8jpKcX
Attachment #8815434 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
(Reporter)

Comment 5

2 years ago
(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.
This is the simple fix with an additional assert.

MozReview-Commit-ID: JuwQS8jpKcX
Attachment #8815511 - Flags: review?(nfroyd)
This is a more elaborate fix that allows for fallibility.
Attachment #8815512 - Flags: review?(nfroyd)
Attachment #8815434 - Attachment is obsolete: true
Attachment #8815434 - Flags: review?(nfroyd)
(Reporter)

Comment 8

2 years ago
(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&);
FWIW I prefer the simple case, I'd rather do a larger rewrite in a follow up.
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.
Attachment #8815511 - Flags: review?(nfroyd) → review+
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.
Attachment #8815512 - Flags: review?(nfroyd)
(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.
Almost positive that everywhere is affected by this one.

[Tracking Requested - why for this release]: security bugs should be uplifted.
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.
Keywords: sec-moderate
https://hg.mozilla.org/mozilla-central/rev/d35117ca7509
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Attachment #8815512 - Attachment is obsolete: true
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.
Attachment #8815511 - Flags: approval-mozilla-beta?
Attachment #8815511 - Flags: approval-mozilla-aurora?
Track 51+ as sec-moderate.
Comment on attachment 8815511 [details] [diff] [review]
Validate length in SubstringTuple

Fix sec-moderate. Beta51+ and Aurora52+. Should be in 51 beta 7.
Attachment #8815511 - Flags: approval-mozilla-beta?
Attachment #8815511 - Flags: approval-mozilla-beta+
Attachment #8815511 - Flags: approval-mozilla-aurora?
Attachment #8815511 - Flags: approval-mozilla-aurora+
No longer blocks: 1320391
Group: dom-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Too late for 50 and doesn't match the esr criteria.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.