1581950 - Failure in nr_ice_component_insert_pair might leave a dangling pointer

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

When nr_ice_component_insert_pair fails, it is impossible for the caller to determine whether the inserted pair has been used or not.

If this function fails in here, the pair has not been stored anywhere, and the caller still owns it: https://searchfox.org/mozilla-central/rev/1d0e84d2ec2843924bc6e5ed6658f93439e12351/media/mtransport/third_party/nICEr/src/ice/ice_component.c#1681-1691

On the other hand, if the function fails here, the pair has already been stored, and the caller does not have ownership: https://searchfox.org/mozilla-central/rev/1d0e84d2ec2843924bc6e5ed6658f93439e12351/media/mtransport/third_party/nICEr/src/ice/ice_component.c#1693-1705

This function is used in three places, currently:

https://searchfox.org/mozilla-central/rev/1d0e84d2ec2843924bc6e5ed6658f93439e12351/media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c#296

Here, the caller never takes ownership, so a failure can result in a leak.

https://searchfox.org/mozilla-central/rev/1d0e84d2ec2843924bc6e5ed6658f93439e12351/media/mtransport/third_party/nICEr/src/ice/ice_component.c#955

Here, the caller takes ownership on failure, which could lead to a UAF.

https://searchfox.org/mozilla-central/rev/1d0e84d2ec2843924bc6e5ed6658f93439e12351/media/mtransport/third_party/nICEr/src/ice/ice_component.c#1127

Here, the caller never takes ownership, so a failure can result in a leak.

I think it makes sense to ensure that this function always takes ownership, and fix the one callsite that UAFs.

Comment 1

[Byron Campen [:bwc]]

I should note that this may be causing these crashes in nr_ice_media_stream_check_timer_cb and nr_ice_component_process_incoming_check on crash stats, since both appear to be dangling pointers to candidate pairs in the check_list, which is exactly what we would expect to see.

Comment 2

[Byron Campen [:bwc]]

This bug has existed since at least 2014, so if it is causing trouble recently, it would be because of either timing changes, or new failures in here:

https://searchfox.org/mozilla-central/rev/1d0e84d2ec2843924bc6e5ed6658f93439e12351/media/mtransport/third_party/nICEr/src/ice/ice_component.c#1693-1705

Comment 3

[Byron Campen [:bwc]]

Marking sec-high, because I'm not sure how easy it is to exploit this.

Comment 4

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8fb3099ef6bc78b0826ddc981d701a138d85bb4

Comment 5

[Byron Campen [:bwc]]

Attachment: Bug 1581950: Prevent nr_ice_component_insert_pair from leaking. r?mjf

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 9093451 [details]
Bug 1581950: Prevent nr_ice_component_insert_pair from leaking. r?mjf

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not very.
• 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?: No
• If not, how different, hard to create, and risky will they be?: Probably trivial.
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely.

Comment 7

[Byron Campen [:bwc]]

Try never ran any jobs. Weird.

Comment 8

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6834b0325fd15e84771bee9b8af900302184931e

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 9093451 [details]
Bug 1581950: Prevent nr_ice_component_insert_pair from leaking. r?mjf

sec-approval+ for checkin on mozilla-central

Comment 10

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

https://hg.mozilla.org/integration/autoland/rev/c3ec34310124f734ddad9c11d02ba8957b5ff015
https://hg.mozilla.org/mozilla-central/rev/c3ec34310124

Comment 11

[Julien Cristau [:jcristau] (back April 22)]

Please request uplift to beta/esr68 when you get a chance.

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 9093451 [details]
Bug 1581950: Prevent nr_ice_component_insert_pair from leaking. r?mjf

Beta/Release Uplift Approval Request

• User impact if declined: This is a UAF, and I see crashes on crash-stats that could be caused by this bug.
• 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): This was a pretty simple fix.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: This is a UAF, and I see crashes on crash-stats that could be caused by this bug.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This was a pretty simple fix.
• String or UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9093451 [details]
Bug 1581950: Prevent nr_ice_component_insert_pair from leaking. r?mjf

Simple UAF fix. Approved for 70.0b9 and 68.2esr.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/99c61c3e15a1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/41dc8707b6ad