1879444 - (CVE-2024-2612) Potential UAFs caused by SafeRefPtr::operator=(SafeRefPtr &&)

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[mozillabugs]

SafeRefPtr::operator=(SafeRefPtr &&) (dom/indexedDB/SafeRefPtr.h) doesn't check for self-assignment. If self-assignment occurs, the function spuriously decrements the reference count of its mRawPtr member, likely causing a UAF in other code that uses the object to which mRawPtr points.

There are several uses of this operator that might be unsafe, though many other usages are of the form object = nullptr, which is clearly safe. From FIREFOX_122_0_RELEASE:

221:   SafeRefPtr& operator=(SafeRefPtr&& aOther) noexcept {
222:     assign_assuming_AddRef(aOther.mRawPtr);
223:     aOther.mRawPtr = nullptr;
224:     return *this;
225:   }

166:  void assign_assuming_AddRef(T* aNewPtr) {
167:     T* oldPtr = mRawPtr;
168:     mRawPtr = aNewPtr;
169:     if (oldPtr) {
170:       ConstRemovingRefPtrTraits<T>::Release(oldPtr);
171:     }
172:  }

The fix is trivial and low risk.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Does this apply to RefPtr too? https://searchfox.org/mozilla-central/rev/9013524d23da6523a7ec4479b5682407a1323f6c/mfbt/RefPtr.h#225-230

  template <typename I,
            typename = std::enable_if_t<std::is_convertible_v<I*, T*>>>
  RefPtr<T>& operator=(RefPtr<I>&& aRefPtr) {
    assign_assuming_AddRef(aRefPtr.forget().take());
    return *this;
  }

https://searchfox.org/mozilla-central/rev/9013524d23da6523a7ec4479b5682407a1323f6c/mfbt/RefPtr.h#64-70

  void assign_assuming_AddRef(T* aNewPtr) {
    T* oldPtr = mRawPtr;
    mRawPtr = aNewPtr;
    if (oldPtr) {
      ConstRemovingRefPtrTraits<T>::Release(oldPtr);
    }
  }

I wonder we have any gtest for RefPtrs.

Comment 2

[Kagami Rosylight [:saschanaz] (they/them)]

mfbt/tests/TestRefPtr.cpp, yup.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Kagami [:saschanaz] (they/them) from comment #1)

Does this apply to RefPtr too?

No, because forget() will unset mRawPtr, so the oldPtr block won't run. That's done by bug 1517237, we should do the same here.

And I guess the real future work item is to move SafeRefPtr to mfbt/xpcom or just remove it.

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1879444 - Make SafeRefPtr move assignment work as RefPtr does r=smaug

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1879444 - Add test r=smaug

Comment 6

[Kagami Rosylight [:saschanaz] (they/them)]

I tried removing the operator to see how many code break, and am surprised that many functions return SafeRefPtr instead of already_AddRefed.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Comment on attachment 9379732 [details]
Bug 1879444 - Make SafeRefPtr move assignment work as RefPtr does r=smaug

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not straightforward, one would:

1. See why the patch is needed at all (probably not immediately clear)
2. If understood, find whether there's any affected code in m-c
3. Find a way to trigger self assignment there

An experienced security researcher may still be interested though.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: The fix is straightforward and shouldn't cause any regression.
• Is Android affected?: Yes

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

For now I'm setting S3. Not sure the severity matters at all in a sec bug where security keywords exist separately...

Comment 9

[Jens Stutte [:jstutte]]

(In reply to Kagami [:saschanaz] (they/them) from comment #8)

Not sure the severity matters at all in a sec bug where security keywords exist separately...

A sec-high implies S2 (and we should set it accordingly in case). This bug has no sec rating, yet, but you are kind of saying "this does not merit a sec-high" when setting it to S3. And "find whether there's any affected code in m-c" from the exploitation steps above seems to imply, we do not even know if there is some such code, which probably would justify such a rating.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Comment on attachment 9379732 [details]
Bug 1879444 - Make SafeRefPtr move assignment work as RefPtr does r=smaug

(sec-moderate)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5e9b0006e96
Make SafeRefPtr move assignment work as RefPtr does r=smaug,dom-storage-reviewers,asuth

Comment 12

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

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

Comment 13

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

The patch landed in nightly and beta is affected.
:saschanaz, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox124 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Kagami Rosylight [:saschanaz] (they/them)]

Comment on attachment 9379732 [details]
Bug 1879444 - Make SafeRefPtr move assignment work as RefPtr does r=smaug

Beta/Release Uplift Approval Request

• User impact if declined: User might become the target of potential security attack
• Is this code covered by automated tests?: Yes
• 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): Straightforward one-line patch that already has been there in RefPtr for years
• String changes made/needed:
• Is Android affected?: Yes

Comment 15

[Dianna Smith [:diannaS]]

Comment on attachment 9379732 [details]
Bug 1879444 - Make SafeRefPtr move assignment work as RefPtr does r=smaug

Approved for 124.0b3

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/270c098b13cd

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9379732 [details]
Bug 1879444 - Make SafeRefPtr move assignment work as RefPtr does r=smaug

Approved for 115.9esr

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/releases/mozilla-esr115/rev/8d29923301cb
Make SafeRefPtr move assignment work as RefPtr does r=smaug,dom-storage-reviewers,asuth, a-dsmith

Comment 19

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8264a4d4762f
Add test r=smaug,dom-storage-reviewers,asuth

Comment 21

[Norisz Fay [:noriszfay]]

Backed out for causing bustages on TestRefPtr.cpp

Backout link

Push with failures

Failure log

Comment 22

[Kagami Rosylight [:saschanaz] (they/them)]

We missed setting a reminder here btw... Is it automatic or manual?

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0558a5568ed1
Add test r=smaug,dom-storage-reviewers,asuth

Comment 24

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0558a5568ed1