1893332 - Latent use after free in QueuedDataMessage()

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[mozillabugs]

QueuedDataMessage() (netwerk/sctp/datachannel/DataChannel.h) would cause uses after free if its defaulted move constructor or its defaulted move-assignment operator were used. They currently are not used, as FF still builds successfully when I change them to deleted.

From FIREFOX_124_0_2_RELEASE:

    103: class QueuedDataMessage {
    104:  public:
    105:   QueuedDataMessage(uint16_t stream, uint32_t ppid, int flags, const void* data,
    106:                     uint32_t length)
    107:       : mStream(stream), mPpid(ppid), mFlags(flags), mLength(length) {
    108:     mData = static_cast<uint8_t*>(moz_xmalloc((size_t)length));  // infallible
    109:     memcpy(mData, data, (size_t)length);
    110:   }
    111:   QueuedDataMessage(QueuedDataMessage&& other) = default;
    112:   QueuedDataMessage& operator=(QueuedDataMessage&& other) = default;
    113:   ~QueuedDataMessage() { free(mData); }
    ...
    119:   uint8_t* mData;
    120: };

Comment 1

[Andrew McCreight [:mccr8]]

I'm not sure what the right rating for this is. Surely we should fix it, but presumably if we did start calling it, the UAF would get detected by ASan, so it would have to be some use we didn't test which doesn't seem too likely.

Comment 2

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

Set release status flags based on info from the regressing bug 1862740

Comment 3

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

Set release status flags based on info from the regressing bug 1862740

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - In DataChannel.h's OutgoingMsg per C.20 use rule of zero. r?bwc!

See C.20 at
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c20-if-you-can-avoid-defining-default-operations-do

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - In DataChannel.h per C.22 don't use default move ctor/assignment with custom dtor. r?bwc!

See C.22 at
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c22-make-default-operations-consistent

These are not needed so deleting them is simpler than implementing them.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - In DataChannel.h per C.21 "the rule of five", also define copy constructors and assignment operators. r?bwc!

See C.21 at
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - In DataChannel, update includes. r?bwc!

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - In OutgoingMsg and BufferedOutgoingMsg, per C.20, remove the need for a custom dtor. r?bwc!

See C.20 at
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c20-if-you-can-avoid-defining-default-operations-do

This is refering to C.20's:

Enforcement

(Not enforceable) While not enforceable, a good static analyzer can detect
patterns that indicate a possible improvement to meet this rule. For example,
a class with a (pointer, size) pair of members and a destructor that deletes
the pointer could probably be converted to a vector.

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - Make OutgoingMsg safer by returning Span instead of raw getters. r?bwc!

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - In QueuedDataMessage, per C.20, remove the need for a custom dtor. r?bwc!

See C.20 at
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c20-if-you-can-avoid-defining-default-operations-do

This is refering to C.20's:

Enforcement

(Not enforceable) While not enforceable, a good static analyzer can detect
patterns that indicate a possible improvement to meet this rule. For example,
a class with a (pointer, size) pair of members and a destructor that deletes
the pointer could probably be converted to a vector.

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1893332 - Tighten DataChannel thread-safety annotations. r?bwc!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7d0e9781f0b
In DataChannel.h's OutgoingMsg per C.20 use rule of zero. r=bwc
https://hg.mozilla.org/integration/autoland/rev/03198220d900
In DataChannel.h per C.22 don't use default move ctor/assignment with custom dtor. r=bwc
https://hg.mozilla.org/integration/autoland/rev/58797638fb29
In DataChannel.h per C.21 "the rule of five", also define copy constructors and assignment operators. r=bwc
https://hg.mozilla.org/integration/autoland/rev/97f62907f179
In DataChannel, update includes. r=bwc
https://hg.mozilla.org/integration/autoland/rev/aba58f0f46eb
In OutgoingMsg and BufferedOutgoingMsg, per C.20, remove the need for a custom dtor. r=bwc
https://hg.mozilla.org/integration/autoland/rev/b027447fbfdf
In QueuedDataMessage, per C.20, remove the need for a custom dtor. r=bwc
https://hg.mozilla.org/integration/autoland/rev/2e0a2fc356c1
Tighten DataChannel thread-safety annotations. r=bwc

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/e7d0e9781f0b
https://hg.mozilla.org/mozilla-central/rev/03198220d900
https://hg.mozilla.org/mozilla-central/rev/58797638fb29
https://hg.mozilla.org/mozilla-central/rev/97f62907f179
https://hg.mozilla.org/mozilla-central/rev/aba58f0f46eb
https://hg.mozilla.org/mozilla-central/rev/b027447fbfdf
https://hg.mozilla.org/mozilla-central/rev/2e0a2fc356c1

Comment 14

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

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

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

For more information, please visit BugBot documentation.