1643437 - (CVE-2020-12420) Crash in [@ nr_ice_component_process_incoming_check]

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

This bug is for crash report bp-54620aeb-8b4a-4d7d-ac49-122e50200604.

Top 10 frames of crashing thread:

0 xul.dll nr_ice_component_process_incoming_check media/mtransport/third_party/nICEr/src/ice/ice_component.c:905
1 xul.dll nr_ice_component_stun_server_cb media/mtransport/third_party/nICEr/src/ice/ice_component.c:1003
2 xul.dll nr_stun_server_process_request media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c:327
3 xul.dll nr_ice_socket_readable_cb media/mtransport/third_party/nICEr/src/ice/ice_socket.c:120
4 xul.dll mozilla::NrUdpSocketIpc::recv_callback_s media/mtransport/nr_socket_prsock.cpp:1594
5 xul.dll static std::_Invoker_pmf_pointer::_Call<void  
6 xul.dll mozilla::runnable_args_memfn<RefPtr<mozilla::NrUdpSocketIpc>, void  media/mtransport/runnable_utils.h:121
7 xul.dll mozilla::detail::runnable_args_base<mozilla::detail::NoResult>::Run media/mtransport/runnable_utils.h:41
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1211
9 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:501

Seems to be happening on all release channels.

Comment 1

[Byron Campen [:bwc]]

It is a little hard to determine what is going on here, since it looks like the stack is not quite complete, but one possibility is that |pair| is a dangling pointer.

Comment 2

[Byron Campen [:bwc]]

This might be a more complete stack from esr:

https://crash-stats.mozilla.org/report/index/6bc56480-100f-4d51-97fb-d1a6a0200602

Comment 3

[Byron Campen [:bwc]]

So I definitely see a problem with peer (ie, remote) candidates here:

https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/media/mtransport/third_party/nICEr/src/ice/ice_component.c#961-969

If nr_ice_component_insert_pair fails here, the candidate pair stays in the check_list: https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/media/mtransport/third_party/nICEr/src/ice/ice_component.c#1712

Then, nr_ice_component_process_incoming_check will destroy the peer candidate here:

https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/media/mtransport/third_party/nICEr/src/ice/ice_component.c#979

This could lead to a UAF here, which is the same place we see an esr crash in comment 2: https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/media/mtransport/third_party/nICEr/src/ice/ice_component.c#795

Comment 4

[Byron Campen [:bwc]]

Filed cover bug 1644477.

Comment 5

[Byron Campen [:bwc]]

Attachment: Bug 1643437: (Copy of patch on cover bug) Make candidate pair insertion code easier to read/understand.

Includes removing an error code for a function that never fails, and removing
an error return when the function successfully did what it said it would.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 9155334 [details]
Bug 1643437: (Copy of patch on cover bug) Make candidate pair insertion code easier to read/understand.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Probably pretty difficult, since it does not look like a sec-bug fix.
• 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?: Pretty easy.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 9155334 [details]
Bug 1643437: (Copy of patch on cover bug) Make candidate pair insertion code easier to read/understand.

sec-approval+

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Looks like cover bug landed successfully. Please nominate it for Beta and ESR68 approval when you get a chance. It grafts cleanly to both as-landed.

Comment 9

[Byron Campen [:bwc]]

Done.

Comment 10

[Byron Campen [:bwc]]

I am not sure that this is fixed just yet; we'll have to wait and see whether bug 1644477 solved the problem.

Comment 11

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

The patch landed in nightly and beta is affected.
:bwc, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[Byron Campen [:bwc]]

Flags are being set in other bugs.

Comment 13

[Julien Cristau [:jcristau]]

Seems like we should mark this fixed.

Comment 14

[Byron Campen [:bwc]]

It looks like bug 1644477 probably solved this, yes. I'm going to mark this fixed for now.

Comment 15

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.