1580320 - Possible use of uninitialized memory in PeerConnectionImpl::GetFingerprint

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Christian Holler (:decoder)]

I wrote a Semmle query to find calls to Getters where we pass pointers to uninitialized memory and ignore the return value and discovered this:

https://searchfox.org/mozilla-central/rev/250f5cc9fb8bdcbb6b23d2a06acfd48addb2f99b/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h#401

If the inner GetFingerprint call fails (and it can fail in various ways), this would expose uninitialized memory to the caller. I don't know where/how this is consumed so marking s-s.

Comment 1

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

The priority flag is not set for this bug.
:drno, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Nils Ohlmeier [:drno]]

It doesn't look to me like this is exposed to JS https://dxr.mozilla.org/mozilla-central/source/dom/webidl/RTCCertificate.webidl#16 (this is where the spec compliant function with the same name would live).

decoder: as you assigned the bug to yourself where you planing on providing a patch yourself?

Comment 3

[Christian Holler (:decoder)]

The function was added in this patch:

https://hg.mozilla.org/mozilla-central/rev/4dd987eef4313d4163da241b79e18000504f07b0

And it looks like the result of it is exposed through the fingerprint attribute on PeerConnectionImpl. And that is exposed, right?

Also, I can make a patch next week some time, if we still need this functionality.

Comment 4

[Nils Ohlmeier [:drno]]

Yes that functionality is still part of the spec. So we need it.

Comment 5

[Christian Holler (:decoder)]

Attachment: Bug 1580320 - Check return value of PeerConnectionImpl::GetFingerprint. r?drno

Comment 6

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

https://hg.mozilla.org/integration/autoland/rev/b79de21aa7b1e42ca74ccaa438e0617ab5148160
https://hg.mozilla.org/mozilla-central/rev/b79de21aa7b1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Is this something we'd eventually want to uplift to ESR68 also? It'd be for the next cycle at this point.

Comment 8

[Nils Ohlmeier [:drno]]

Given how long we are still supporting ESR 68 it's probably a good idea to uplift this.

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 9099512 [details]
Bug 1580320 - Check return value of PeerConnectionImpl::GetFingerprint. r?drno

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is just a safety check to ensure nobody can abuse this going forward.
• User impact if declined: This should prevent an attacker potentially getting access to uninitialized memory.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change only checks the return value from a function call, which should have been checked from the beginning. It's not functional change at all.
• String or UUID changes made by this patch: N/A

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9099512 [details]
Bug 1580320 - Check return value of PeerConnectionImpl::GetFingerprint. r?drno

Fixes a webrtc sec issue. Approved for 68.3esr.

Comment 11

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/9dced159051d