1883158 - (CVE-2024-3861) Potential memory hazard due to AlignedBuffer& AlignedBuffer::operator=(&&)

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[mozillabugs]

AlignedBuffer& operator=(&&) (dom/media/MediaData.h) creates an invalid member UniquePtr<uint8_t[]> mBuffer on self-move. The bug is that it doesn't check for self-move before calling ~AlignedBuffer(), which invalidates the entire object. (Following code from FIREFOX_122_0_RELEASE):

     97:  AlignedBuffer& operator=(AlignedBuffer&& aOther) noexcept {
     98:    this->~AlignedBuffer();
     99:    new (this) AlignedBuffer(std::move(aOther));
    100:    return *this;
    101:  }

Line 99 then constructs a new object from the invalid object using the move constructor:

    87:  AlignedBuffer(AlignedBuffer&& aOther) noexcept
    88:      : mData(aOther.mData),
    89:        mLength(aOther.mLength),
    90:        mBuffer(std::move(aOther.mBuffer)),
    91:        mCapacity(aOther.mCapacity) {
    92:    aOther.mData = nullptr;
    93:    aOther.mLength = 0;
    94:    aOther.mCapacity = 0;
    95:  }

but the mBuffer member:

    246:  UniquePtr<uint8_t[]> mBuffer;

is never reinitialized, and thus retains the invalid value it received on line 98 (and which got moved to itself on line 90). That value eventually will get used by ~UniquePtr<uint8_t[]>(), possibly releasing memory it doesn't own.

I don't know whether any code actually does a self-move.

https://bugzilla.mozilla.org/show_bug.cgi?id=1815108 might be related to this bug.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1883158.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

There is a lot of code to cover, so I am not 100% certain, but I think this is theoretical, since this isn't a useful pattern for us to replicate in general. We are usually moving the AlignedBuffer objects from one part of the pipeline to another, and so we don't actually run the risk of moving into itself. It is easy enough to fix though this and make the code very clear/simple.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9389750 [details]
Bug 1883158.

Beta/Release Uplift Approval Request

• User impact if declined: Sec issue
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): Trivial change to correct move operator in the case of self move, now works exactly the same as the move constructor.
• String changes made/needed:
• Is Android affected?: Yes

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbd98998a1c5
r=media-playback-reviewers,chunmin,azebrowski

Comment 5

[Dianna Smith [:diannaS]]

Comment on attachment 9389750 [details]
Bug 1883158.

This is too late and doesn't seem critical. We have already started building an RC .

Comment 6

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

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR115 approval when you get a chance.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9389750 [details]
Bug 1883158.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Minor sec issue, very low risk
• User impact if declined: Sec issue
• Fix Landed on Version: 125
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial patch making the move operator mirror the move constructor

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9389750 [details]
Bug 1883158.

Approved for 115.10esr.

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/cc048f3d96ec

Comment 11

[Daniel Veditz [:dveditz]]

Attachment: advisory.txt