1026774 - malloc of undefined size in stun_get_mib_addrs in rare cases

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

If this call fails, |needed| is left uninitialized, and then passed to malloc (errx only prints to stderr, it does not bail).

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#242

Granted, this failure should be rare, but I am not totally sure how rare.

Comment 1

[Byron Campen [:bwc]]

So, if the malloc call doesn't crash, I would expect us to deref null here:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#254

This is probably just a straightforward crash bug.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

So, at absolute most this is a DoS (if they can force you to allocate a ton of memory, and there are far easier ways to do that) or a null deref (which isn't a sec issue).

Comment 3

[Byron Campen [:bwc]]

Well, if we allocate a ton of memory, we'll then start trying to parse it, which could send us into the weeds. If we get past here by sheer luck:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#254

We might end up going off into the weeds using random data.

Comment 4

[Byron Campen [:bwc]]

Attachment: Return errors on some rare failure cases in stun_get_mib_addrs.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8481586 [details] [diff] [review]
Return errors on some rare failure cases in stun_get_mib_addrs.

Review of attachment 8481586 [details] [diff] [review]:
-----------------------------------------------------------------

https://tbpl.mozilla.org/?tree=Try&rev=275586fa59fc

Comment 6

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e48e922e0d

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Benjamin Kerensa [:bkerensa]]

Did this impact ESR31?

Comment 9

[Byron Campen [:bwc]]

Yes.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Would this be worth backporting to b2g34 for v2.1 still?

Comment 11

[Byron Campen [:bwc]]

I suspect the existing patch will apply cleanly there, so why not?

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8481586 [details] [diff] [review]
Return errors on some rare failure cases in stun_get_mib_addrs.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

   Initial webrtc landing

User impact if declined: 

   Potential use of uninitialized memory causing crashes or other undefined behavior in rare cases.

Testing completed: 

   The usual CI testing.

Risk to taking this patch (and alternatives if risky): 

   Basically no risk.

String or UUID changes made by this patch:

   None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e57b91cfb324

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/e57b91cfb324