1741665 - Prevent integer truncation bugs at nsCString APIs by accepting size_t in public APIs

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Nika Layzell [:nika] (ni? for response)]

There are some potential bugs which can be caused by callers passing a size_t which overflows uint32_t to a nsTString API like SetLength() or EnsureMutable(), which will end up creating a string with a much smaller buffer than the input string. To avoid this, this bug proposes changing nsTStringRepr::size_type to be size_t, while still storing the length at-rest as a uint32_t, to allow for the relevant size checks to be performed in these cases.

Because the underlying size of a nsTString is still required to fit within an int32_t (due to kMaxCapacity), this shouldn't break any call-sites which convert a string's length to uint32_t, and will instead just help callers which are implicitly truncating a size_t on 64-bit platforms.

Changing nsTString to actually use a size_t length is not in scope for this bug, as it would require massive audits across the codebase of places where a string's length is used to make sure it handles larger lengths properly.

In addition, the Rust API is not changed in this bug, as rust does not have implicit integer casts. It will probably be updated in a future patch to more closely align with the C++ API, however. As the string type's representation did not change, the existing FFI code should continue to function.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1741665 - Align nsCString's public size_type better with other C++ APIs, r=mccr8

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1741665 - (BETA) Align nsCString's public size_type better with other C++ APIs

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1741665 - (ESR91) Align nsCString's public size_type better with other C++ APIs

Comment 4

[Nika Layzell [:nika] (ni? for response)]

:mccr8 I can't seem to re-request review on the patch, but it's changed enough I think you should look at it again after thanksgiving weekend.

Comment 5

[Andrew McCreight [:mccr8]]

I've now re-reviewed the patch.

Comment 6

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: I think Nika is going to land this on Nightly soon. We should also try to fix this on ESR (and on beta if we can), because it is possible that this fixes a number of security issues we haven't found yet (there are over 700 calls to SetLength() alone).

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Align nsCString's public size_type better with other C++ APIs, r=mccr8,geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/43563b9a5f5d132449ec86456cf246b85ccf8306
https://hg.mozilla.org/mozilla-central/rev/43563b9a5f5d

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Andrew McCreight [:mccr8]]

This has been in a Nightly or two, and I don't see any crashes on crash-stats where the crash reason contains "string is too large", so that's good.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

The patches which I currently have attached for ESR and Beta are no longer up to date with the landed version. I think that the landed version of the patch should be able to rebase relatively cleanly onto beta, however some tweaks will be required to get it to land onto ESR.

I unfortunately may not get around to doing this rebase or uplift request before I go on PTO for the holidays tomorrow.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Per Slack discussion with Nika, we're just going to let this go out in 97/91.6esr.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Not super urgent at the moment, but we can take an ESR patch for 91.6 whenever you've got cycles to rebase.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9252175 [details]
Bug 1741665 - (ESR91) Align nsCString's public size_type better with other C++ APIs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potential security issues due to string overflows in ESR
• Fix Landed on Version: 97
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Patch is fairly large and required rebasing to apply, but has been largely tested on 97 through nightly and beta and hasn't caused issues.

Patch hasn't been properly tested yet, but seemed to build locally.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9252175 [details]
Bug 1741665 - (ESR91) Align nsCString's public size_type better with other C++ APIs

Approved for 91.6esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/d47fdd938c09