Closed Bug 1741665 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
97 Branch
Tracking Status
firefox-esr91 97+ fixed
firefox94 --- wontfix
firefox95 --- wontfix
firefox96 + wontfix
firefox97 + fixed

People

(Reporter: nika, Assigned: nika)

References

Details

(Keywords: sec-want, Whiteboard: [post-critsmash-triage][adv-main97-][adv-esr91.6-])

Attachments

(2 files, 1 obsolete file)

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.

Group: core-security → dom-core-security
Keywords: sec-want

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

Flags: needinfo?(continuation)

I've now re-reviewed the patch.

Flags: needinfo?(continuation)

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

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

Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

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.

Flags: needinfo?(nika)

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.

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.

Flags: needinfo?(nika)

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

Regressions: 1747185

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

Flags: needinfo?(nika)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Attachment #9252174 - Attachment is obsolete: true

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.

Flags: needinfo?(nika)
Attachment #9252175 - Flags: approval-mozilla-esr91?

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

Approved for 91.6esr.

Attachment #9252175 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main97-]
Whiteboard: [post-critsmash-triage][adv-main97-] → [post-critsmash-triage][adv-main97-][adv-esr91.6-]
Regressions: 1757253
Group: core-security-release
Regressions: 1774462
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: