1356025 - Possible write beyond bounds due to passing a large buffer to nsTSubstring_CharT::nsTSubstring_CharT()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[q1]

Like nsTSubstring_CharT::Adopt() in https://bugzilla.mozilla.org/show_bug.cgi?id=1349719, nsTSubstring_CharT::nsTSubstring_CharT() (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, nsTSubstring_CharT::nsTSubstring_CharT() is used to form a string >= 0x80000000 characters, overflow and write beyond bounds as in https://bugzilla.mozilla.org/show_bug.cgi?id=1349719 can occur.

I didn't notice this bug when I reported https://bugzilla.mozilla.org/show_bug.cgi?id=1349719 , but found it via code review by tracing the assembly-code call chain:

   DataStruct::GetData()
     -> DataStruct::ReadCache()
     -> nsPrimitiveHelpers::CreatePrimitiveForData()
     -> SubString() (nstdependentsubstring.h)
     -> nsTDependentSubstring_CharT(const CharT *, const CharT *)
     -> nsDependentCSubstring::nsDependentCSubstring(const char *, const char *)
     -> nsACString_internal::nsACString_internal(char *, unsigned int, unsigned int)
     -> nsTSubstring_CharT::nsTSubstring_CharT(char_type *, size_type, uint32_t)


I do not yet have a POC.

Comment 1

[Eric Rahm [:erahm]]

I'll take a look at this.

Comment 2

[q1]

Hmm, this usage needs some careful examination. I stumbled across a usage in WebSocket::Send() that uses this code path to create a nsDependentCSubstring from an ArrayBuffer, so there could be some side-effects from limiting the size of strings created this way.

Comment 3

[Eric Rahm [:erahm]]

(In reply to q1 from comment #2)
> Hmm, this usage needs some careful examination. I stumbled across a usage in
> WebSocket::Send() that uses this code path to create a nsDependentCSubstring
> from an ArrayBuffer, so there could be some side-effects from limiting the
> size of strings created this way.

For the |WebSocket::Send| case we eventually copy the string to a new nsCString so that should do length checks and trigger an OOM. I think limiting the size here is fine. Why we're using strings for binary data is beyond me, but that's the way it is. If we ever need to send a larger chunk of data we can switch over from using strings.

Comment 4

[q1]

> Why we're using strings for binary data is beyond me...

Maybe a holdover from Navigator days? But presumably they had nsTArray then, so beats me.

Comment 5

[Eric Rahm [:erahm]]

This is probably at most sec-moderate, the common way of combining strings (stringA + stringB) would fail as expected but yes, adding sizes might overflow. I don't know how common that is, but I would guess not very. I think we can add a MOZ_RELEASE_ASSERT(CheckCapacity(aLength)) type assert in the ctor just in case.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Add Capacity checks to nsTSubstring constructor

I think this should do it. We could possibly move the check to be
|nsTDependentSubstring| only but this seemed safest.

MozReview-Commit-ID: 6RIwuvalcRz

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8867413 [details] [diff] [review]
Add Capacity checks to nsTSubstring constructor

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

::: xpcom/string/nsTSubstring.h
@@ +1121,5 @@
>  #undef XPCOM_STRING_CONSTRUCTOR_OUT_OF_LINE
>    nsTSubstring_CharT(char_type* aData, size_type aLength, uint32_t aFlags)
>      : nsTStringRepr_CharT(aData, aLength, aFlags)
>    {
> +    MOZ_RELEASE_ASSERT(CheckCapacity(aLength), "String is too large.");

I am not super-excited about adding this check to a possibly inlined function, but maybe it will be OK.  Can you file a followup bug for investigating size changes as a result of out-of-lining this constructor all the time?

Comment 8

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5ae949a6b9a2b28f46e4aff033468cf91f7ae5
Bug 1356025 - Add Capacity checks to nsTSubstring constructor. r=froydnj

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

hrm, AFAICT, this goes back a loooong way, which means it probably shouldn't have landed without a sec rating (and sec-approval if necessary). Eric, can you suggest a rating for this and request branch approvals where applicable?

Comment 11

[Andrew McCreight [:mccr8]]

In comment 5, Eric said this was at most a sec-moderate, so I'll mark it sec-moderate. It would still be nice to uplift if it is low risk.

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8867413 [details] [diff] [review]
Add Capacity checks to nsTSubstring constructor

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

N/A, this has been around a long time.

[User impact if declined]:

Theoretical buffer overflow if we combine large strings.

[Is this code covered by automated tests?]:

Code is heavily used.

[Has the fix been verified in Nightly?]:

Yes, baked a few days.

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

No.

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

N/A

[Is the change risky?]:

Low risk.

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

Just adds a release assertion.

[String changes made/needed]:

N/A

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8867413 [details] [diff] [review]
Add Capacity checks to nsTSubstring constructor

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

Theoretical buffer overflow, simple fix adding a release assertion.

User impact if declined: 

Possible buffer overflow.

Fix Landed on Version:

55.

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

Low risk, just adds a release assertion.

String or UUID changes made by this patch: 

N/A

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8867413 [details] [diff] [review]
Add Capacity checks to nsTSubstring constructor

Prevent a theoretical buffer overflow. Should be low risk. Beta54+ & ESR52+. Should be in 54 beta 10 & 52.2esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1094b15fa46e
https://hg.mozilla.org/releases/mozilla-esr52/rev/f5967db0a0f3

Comment 16

[Andrei Vaida [:avaida]]

(In reply to Eric Rahm [:erahm] from comment #12)
> [Is this code covered by automated tests?]:
> 
> Code is heavily used.
> 
> [Has the fix been verified in Nightly?]:
> 
> Yes, baked a few days.
> 
> [Needs manual test from QE? If yes, steps to reproduce]:
> 
> No.

Setting qe-verify- based on Eric's assessment on manual testing needs.

Comment 17

[Gerry Chang [:gchang]]

Track 54+/55+ as potential buffer overflow issue.