Closed Bug 1580320 Opened 11 months ago Closed 10 months ago

Possible use of uninitialized memory in PeerConnectionImpl::GetFingerprint

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- fixed
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: decoder, Assigned: decoder)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: csectype-uninitialized, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main71+r])

Attachments

(1 file)

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.

Group: core-security → media-core-security

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(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?

Flags: needinfo?(drno)
Priority: -- → P2

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.

Flags: needinfo?(choller)

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

Group: media-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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

Flags: needinfo?(choller) → needinfo?(drno)
Regressed by: 892148
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

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

Flags: needinfo?(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
Attachment #9099512 - Flags: approval-mozilla-esr68?

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

Fixes a webrtc sec issue. Approved for 68.3esr.

Attachment #9099512 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main71+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.