Closed Bug 1356025 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPCOM, defect)

52 Branch
defect
Not set

Tracking

()

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

People

(Reporter: q1, Assigned: erahm)

Details

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

Attachments

(1 file)

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.
I'll take a look at this.
Flags: needinfo?(erahm)
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.
Group: core-security → dom-core-security
(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.
Flags: needinfo?(erahm)
Summary: Probable write beyond bounds due to nsTSubstring_CharT::nsTSubstring_CharT() → Possible write beyond bounds due to passing a large buffer to nsTSubstring_CharT::nsTSubstring_CharT()
> 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.
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.
I think this should do it. We could possibly move the check to be
|nsTDependentSubstring| only but this seemed safest.

MozReview-Commit-ID: 6RIwuvalcRz
Attachment #8867413 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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?
Attachment #8867413 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/cd5ae949a6b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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?
Flags: needinfo?(erahm)
Target Milestone: --- → mozilla55
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.
Flags: needinfo?(erahm)
Keywords: sec-moderate
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
Attachment #8867413 - Flags: approval-mozilla-beta?
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
Attachment #8867413 - Flags: approval-mozilla-esr52?
Group: dom-core-security → core-security-release
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.
Attachment #8867413 - Flags: approval-mozilla-esr52?
Attachment #8867413 - Flags: approval-mozilla-esr52+
Attachment #8867413 - Flags: approval-mozilla-beta?
Attachment #8867413 - Flags: approval-mozilla-beta+
(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.
Flags: qe-verify-
Track 54+/55+ as potential buffer overflow issue.
Whiteboard: [adv-main54+][adv-esr52.2+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.