1497246 - (CVE-2022-34481) integer overflow in nsTArray::ReplaceElementsAt

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nathan Froyd [:froydnj]]

+++ This bug was initially created as a clone of Bug #1483699 +++

Bug 1483699 handled a few cases of potential integer overflow in AppendElements/InsertElementsAt.  There's one case in ReplaceElementsAt that we didn't handle and can handle in this bug:

  // Adjust memory allocation up-front to catch errors.
  if (!ActualAlloc::Successful(this->template EnsureCapacity<ActualAlloc>(
        Length() + aArrayLen - aCount, sizeof(elem_type)))) {
    return nullptr;
  }

The addition of Length() + aArrayLen - aCount should be checked for overflow in some fashion.

Comment 1

[mozillabugs]

(In reply to Nathan Froyd [:froydnj] from comment #0)
> +++ This bug was initially created as a clone of Bug #1483699 +++
> 
> Bug 1483699 handled a few cases of potential integer overflow in
> AppendElements/InsertElementsAt.  There's one case in ReplaceElementsAt that
> we didn't handle and can handle in this bug:
> 
>   // Adjust memory allocation up-front to catch errors.
>   if (!ActualAlloc::Successful(this->template EnsureCapacity<ActualAlloc>(
>         Length() + aArrayLen - aCount, sizeof(elem_type)))) {
>     return nullptr;
>   }
> 
> The addition of Length() + aArrayLen - aCount should be checked for overflow
> in some fashion.

Yes, please! I don't think the expression can overflow (because |aArrayLen| elements must exist in the array being added), but it can underflow because |aCount| appears to be unchecked on all nsTArray paths into ReplaceElementsAt ().

Comment 2

[Andrew McCreight [:mccr8]]

Nika, any ideas about who might be able to look at this bug? It seems like a shame to leave a fairly simple looking sec bug open for this long. Thanks.

Comment 3

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

Attachment: Bug 1497246 - Release-check aCount for underflow in ReplaceElementsAt, r=#xpcom-reviewers

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/243ca18dc172

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta/ESR91 approval when you get a chance.

Comment 6

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

Comment on attachment 9279959 [details]
Bug 1497246 - Release-check aCount for underflow in ReplaceElementsAt, r=#xpcom-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Slightly less validation in some nsTArray methods
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor change double-checking some math for underflow in nsTArray. The underflowing case should never be hit as it would require an index out-of-bounds error, but this adds some extra defense-in-depth.
• String changes made/needed: None
• Is Android affected?: Yes

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9279959 [details]
Bug 1497246 - Release-check aCount for underflow in ReplaceElementsAt, r=#xpcom-reviewers

Approved for 102 beta 8, thanks.

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/afdfb5a05608

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9279959 [details]
Bug 1497246 - Release-check aCount for underflow in ReplaceElementsAt, r=#xpcom-reviewers

Approved for 91.11esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/60a5196e74ad

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt