q1 Reporter
	Description • 7 years ago

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.

	Eric Rahm [:erahm] Assignee
	Comment 1 • 7 years ago

I'll take a look at this.

	q1 Reporter
	Comment 2 • 7 years ago

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.

	Eric Rahm [:erahm] Assignee
	Comment 3 • 7 years ago

(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.

	q1 Reporter
	Comment 4 • 7 years ago

> 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.

	Eric Rahm [:erahm] Assignee
	Comment 5 • 7 years ago

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.

	Eric Rahm [:erahm] Assignee
	Comment 6 • 7 years ago

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

MozReview-Commit-ID: 6RIwuvalcRz

	Nathan Froyd [:froydnj]
	Comment 7 • 7 years ago

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?

	Eric Rahm [:erahm] Assignee
	Comment 8 • 7 years ago

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

	Carsten Book [:Tomcat]
	Comment 9 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 7 years ago

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?

	Andrew McCreight [:mccr8]
	Comment 11 • 7 years ago

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.

	Eric Rahm [:erahm] Assignee
	Comment 12 • 7 years ago

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

	Eric Rahm [:erahm] Assignee
	Comment 13 • 7 years ago

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

	Gerry Chang [:gchang]
	Comment 14 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 7 years ago
uplift

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

	Andrei Vaida [:avaida]
	Comment 16 • 7 years ago

(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.

	Gerry Chang [:gchang]
	Comment 17 • 7 years ago

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