Closed Bug 1349719 Opened 3 years ago Closed 3 years ago

Probable write beyond bounds due to nsTSubstring_CharT::Adopt()

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: q1, Assigned: erahm)

Details

(Keywords: csectype-intoverflow, sec-moderate, Whiteboard: [adv-main53+][adv-esr52.1+])

Attachments

(3 files, 2 obsolete files)

nsTSubstring_CharT::Adopt() (xpcom/string/nsTSubstring.cpp) permits its callers to form an ns*String that can be longer than the maximum length ordinarily allowed by nsTSubstring_CharT::MutatePrep(). For single-byte strings, this length is 0x7ffffff5 characters, and for 2-byte strings, it is 0x3ffffff9 characters. These limits guarantee that adding the lengths of 2 ns*Strings -- which is common in the codebase -- will never overflow a uint32_t.

If, however, Adopt() is used to form a string >= 0x80000000 characters, code of the following form can overflow, causing writes beyond bounds:

   nsCString s1;
   s.Adopt (someLongByteString, someLongByteStringLength);
   ...
   nsCString s2;
   ...
   uint32_t newStringLen = s1.Length() + s2.Length();  // potential overflow...
   nsCString s3;
   s3.SetLength(newStringLen);                         // ...and allocation of too-small buffer...

   // ...and write beyond bounds.

   memcpy (s3.BeginWriting(), s1.BeginReading(), s1.Length());
   memcpy (s3.BeginWriting() + s1.Length(), s2.BeginReading(), s2.Length());
   

The following is from trunk:

501: void
502: nsTSubstring_CharT::Adopt(char_type* aData, size_type aLength)
503: {
504:   if (aData) {
505:     ::ReleaseData(mData, mFlags);
506: 
507:     if (aLength == size_type(-1)) {
508:       aLength = char_traits::length(aData);
509:     }
510: 
511:     mData = aData;
512:     mLength = aLength;
513:     SetDataFlags(F_TERMINATED | F_OWNED);
514: 
515:     STRING_STAT_INCREMENT(Adopt);
516:     // Treat this as construction of a "StringAdopt" object for leak
517:     // tracking purposes.
518:     MOZ_LOG_CTOR(mData, "StringAdopt", 1);
519:   } else {
520:     SetIsVoid(true);
521:   }
522: }

I do not yet have a POC.
Flags: sec-bounty?
I'm assuming the max length is coming from |kMaxCapacity| in |MutatePrep| [1]. We could probably pull that constant out to a separate function and check it in |Adopt|.

[1] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/xpcom/string/nsTSubstring.cpp#57-58
(In reply to Eric Rahm [:erahm] from comment #1)
> I'm assuming the max length is coming from |kMaxCapacity| in |MutatePrep|
> [1]. We could probably pull that constant out to a separate function and
> check it in |Adopt|.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 9af9b1c7946ebef6056d2ee4c368d496395cf1c8/xpcom/string/nsTSubstring.cpp#57-58

Yes, that's it.
This refactors the max capacity logic so that both |MutatePrep| and |Adopt| can
share it.

MozReview-Commit-ID: CMn4kiAoWub
Attachment #8850248 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8850248 [details] [diff] [review]
Share max capacity logic in nsTString

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

r+ with some comments we should discuss.

::: xpcom/string/nsTSubstring.cpp
@@ +33,5 @@
>  }
>  
> +constexpr nsTSubstring_CharT::size_type
> +nsTSubstring_CharT::MaxCapacity() {
> +  return (size_type(-1) / 2 - sizeof(nsStringBuffer)) / sizeof(char_type) - 2;

I like using an actual static variable for this a little better, but I guess you used a constexpr function because writing things out with a static variable would be very unwieldly?

@@ +508,5 @@
>        aLength = char_traits::length(aData);
>      }
>  
> +    if (!CheckCapacity(aLength)) {
> +      ::NS_ABORT_OOM(aLength);

WDYT about making this:

MOZ_RELEASE_ASSERT(CheckCapacity(aLength), "adopting a too-long string");

Release asserting would make it a little easier to search for in crash stats and would be a bit more accurate (we're not exactly running out of memory here).  Or did you choose to not do this to avoid bullseye-painting?

::: xpcom/string/nsTSubstring.h
@@ +1117,5 @@
> +
> +  /**
> +   * Checks if the given capacity is valid for this string type.
> +   */
> +  MOZ_MUST_USE bool CheckCapacity(size_type aCapacity) {

We should either make this a static member function or a const member function.
Attachment #8850248 - Flags: review?(nfroyd) → review+
FWIW

   size_type(-1)

technically produces UB. It would seem you could use

   std::numeric_limits <size_type>::max()

instead.
(In reply to q1 from comment #5)
> FWIW
> 
>    size_type(-1)
> 
> technically produces UB.

What is UB about this?  signed-to-unsigned integer conversion is well-defined [conv.integral].
(In reply to Nathan Froyd [:froydnj] from comment #4)
> Comment on attachment 8850248 [details] [diff] [review]
> Share max capacity logic in nsTString
> 
> Review of attachment 8850248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with some comments we should discuss.
> 
> ::: xpcom/string/nsTSubstring.cpp
> @@ +33,5 @@
> >  }
> >  
> > +constexpr nsTSubstring_CharT::size_type
> > +nsTSubstring_CharT::MaxCapacity() {
> > +  return (size_type(-1) / 2 - sizeof(nsStringBuffer)) / sizeof(char_type) - 2;
> 
> I like using an actual static variable for this a little better, but I guess
> you used a constexpr function because writing things out with a static
> variable would be very unwieldly?

I'll see if I can get a static working.

> @@ +508,5 @@
> >        aLength = char_traits::length(aData);
> >      }
> >  
> > +    if (!CheckCapacity(aLength)) {
> > +      ::NS_ABORT_OOM(aLength);
> 
> WDYT about making this:
> 
> MOZ_RELEASE_ASSERT(CheckCapacity(aLength), "adopting a too-long string");
> 
> Release asserting would make it a little easier to search for in crash stats
> and would be a bit more accurate (we're not exactly running out of memory
> here).  Or did you choose to not do this to avoid bullseye-painting?

MOZ_RELEASE_ASSERT probably isn't too obvious, I'll change it.

> ::: xpcom/string/nsTSubstring.h
> @@ +1117,5 @@
> > +
> > +  /**
> > +   * Checks if the given capacity is valid for this string type.
> > +   */
> > +  MOZ_MUST_USE bool CheckCapacity(size_type aCapacity) {
> 
> We should either make this a static member function or a const member
> function.

I'll make it static.
I'm going to mark this sec-moderate because it isn't clear if this can actually be triggered.
(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to q1 from comment #5)
> > FWIW
> > 
> >    size_type(-1)
> > 
> > technically produces UB.
> 
> What is UB about this?  signed-to-unsigned integer conversion is
> well-defined [conv.integral].

Hmm, I am unable to find the text that I believed made this assertion. Perhaps I was thinking of narrowing conversions (s.5.17(9) of n3242), which refers to s.8.5.4, for which the following example (s.8.5.4(6)) was given:

   unsigned int ui1 = {-1}; // error: narrows

I still feel doubtful about the cast, and suggest the std::numeric_limits<T>::max() method.
Updated per review comments.
Attachment #8850248 - Attachment is obsolete: true
Comment on attachment 8850643 [details] [diff] [review]
Share max capacity logic in nsTString

Carrying forward r+
Attachment #8850643 - Flags: review+
(In reply to q1 from comment #9)
> (In reply to Nathan Froyd [:froydnj] from comment #6)
> > (In reply to q1 from comment #5)
> > > FWIW
> > > 
> > >    size_type(-1)
> > > 
> > > technically produces UB.
> > 
> > What is UB about this?  signed-to-unsigned integer conversion is
> > well-defined [conv.integral].
> 
> Hmm, I am unable to find the text that I believed made this assertion.
> Perhaps I was thinking of narrowing conversions (s.5.17(9) of n3242), which
> refers to s.8.5.4, for which the following example (s.8.5.4(6)) was given:
> 
>    unsigned int ui1 = {-1}; // error: narrows
> 
> I still feel doubtful about the cast, and suggest the
> std::numeric_limits<T>::max() method.

We extensively use size_type(-1) in the string code, including as default value for size arguments. I assume prior to c++11 switching |max()| to constexpr this would have been a no-go. Regardless, this would be a rather large change and is outside the scope of this bug. Feel free to file a follow up if you feel strongly about it (I don't think it needs to be sec-sensitive).
(In reply to Eric Rahm [:erahm] from comment #12)
> (In reply to q1 from comment #9)
> > (In reply to Nathan Froyd [:froydnj] from comment #6)
> > > (In reply to q1 from comment #5)
> > > > FWIW
> > > > 
> > > >    size_type(-1)
> > > > 
> > > > technically produces UB.
> > > 
> > > What is UB about this?  signed-to-unsigned integer conversion is
> > > well-defined [conv.integral].
> > 
> > Hmm, I am unable to find the text that I believed made this assertion.
> > Perhaps I was thinking of narrowing conversions (s.5.17(9) of n3242), which
> > refers to s.8.5.4, for which the following example (s.8.5.4(6)) was given:
> > 
> >    unsigned int ui1 = {-1}; // error: narrows
> > 
> > I still feel doubtful about the cast, and suggest the
> > std::numeric_limits<T>::max() method.
> 
> We extensively use size_type(-1) in the string code, including as default
> value for size arguments. I assume prior to c++11 switching |max()| to
> constexpr this would have been a no-go. Regardless, this would be a rather
> large change and is outside the scope of this bug. Feel free to file a
> follow up if you feel strongly about it (I don't think it needs to be
> sec-sensitive).

Thanks for the followup. If I find the UB reference I'll file another bug.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Given where we are in the various lifecycles, this doesn't seem worth backporting to ESR45, but Aurora/Beta/ESR52 seem reasonable?
I agree.
Flags: sec-bounty? → sec-bounty+
Group: core-security → core-security-release
Patch for aurora.
Patch for beta and esr52.
Comment on attachment 8854557 [details] [diff] [review]
Share max capacity logic in nsTString for aurora

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

None, this is old code.

[User impact if declined]:

Possible OOB write if |nsTSubstring_CharT::Adopt| is misused.

[Is this code covered by automated tests?]:

Yes.

[Has the fix been verified in Nightly?]:

Yes.

[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?]:

Low risk.

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

We just enforce that the requested capacity of a string is within our maximum capacity.

[String changes made/needed]:

N/A
Flags: needinfo?(erahm)
Attachment #8854557 - Flags: approval-mozilla-aurora?
Comment on attachment 8854560 [details] [diff] [review]
Share max capacity logic in nsTString for beta and esr52

See comment 21.
Attachment #8854560 - Flags: approval-mozilla-beta?
Comment on attachment 8854560 [details] [diff] [review]
Share max capacity logic in nsTString for beta and esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 

Possible OOB write if nsTSubstring_CharT::Adopt is misused.

Fix Landed on Version:

55

Risk to taking this patch (and alternatives if risky): 

Low.

String or UUID changes made by this patch: 

N/A
Attachment #8854560 - Flags: approval-mozilla-esr52?
Comment on attachment 8854557 [details] [diff] [review]
Share max capacity logic in nsTString for aurora

Fix a security issue. Aurora54+.
Attachment #8854557 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8854560 [details] [diff] [review]
Share max capacity logic in nsTString for beta and esr52

Seems reasonable, let's try to get this fix into beta 10.
Attachment #8854560 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
It looks like I missed a qref. Now with changes to the header as well.
Attachment #8854560 - Attachment is obsolete: true
Attachment #8854560 - Flags: approval-mozilla-esr52?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> Backed out from Beta for bustage (yes, this was using the attached beta
> patch).
> https://hg.mozilla.org/releases/mozilla-beta/rev/
> db49c7b778064599f028d5b3a96cda1deb257e5d
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=89161670&repo=mozilla-
> beta

Latest version should be fixed, sorry about that!
Flags: needinfo?(erahm) → needinfo?(ryanvm)
Comment on attachment 8855458 [details] [diff] [review]
Share max capacity logic in nsTString for beta and esr52

Looks like the esr52 request got lost along the way there.
Attachment #8855458 - Flags: approval-mozilla-esr52?
Comment on attachment 8855458 [details] [diff] [review]
Share max capacity logic in nsTString for beta and esr52

esr52+, thanks for rescuing this Ryan
Attachment #8855458 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Setting qe-verify- based on Eric's assessment on manual testing needs (see Comment 21) and the fact that this fix has automated coverage.
Flags: qe-verify-
Whiteboard: [adv-main53+][adv-esr52.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.