942940 - Potential dangling peerreflexive candidate on some types of failure.

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

When a new peer reflexive candidate is created, it is inserted into the candidate list prior to creation of the required candidate pair.

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#523

If some failure is encountered while creating the pair, the code at the abort: label destroys the already inserted candidate, leaving us open for use after free, and guaranteeing a double free.

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#587

Comment 1

[Byron Campen [:bwc]]

Attachment: Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

Comment 2

[Eric Rescorla (:ekr)]

I'd like to see some analysis of how this can actually happen before
deciding it's critical.

Comment 3

[Eric Rescorla (:ekr)]

If this is a real issue, it's going to be an issue in existing
versions of Firefox as well as in nICEr so we need to:

1. Verify if this is a real issue by walking through the candidate
pair creation logic.
2. If it is a real issue, then we need to do sec-approval and
simultaeneous uplift to nICEr.

Comment 4

[Byron Campen [:bwc]]

Looking through this, the priority calculation logic relies on app implementation details not within nICEr; if any of the app implementation used (the registry stuff for getting type preference values, the interface prioritizer if any, or the nr_socket_getaddr implementation) is fragile, we could fail to create the candidate pair.

I will continue looking for stuff that could go wrong after the priority is calculated.

Comment 5

[Byron Campen [:bwc]]

Ok, finished looking, and only found more of the same (the only new thing I could find is some failures that could be caused by the app registering too many r_log facilities).

Comment 6

[Eric Rescorla (:ekr)]

Adam, can you weigh in here?

Comment 7

[Adam Roach [:abr]]

(In reply to Eric Rescorla (:ekr) from comment #6)
> Adam, can you weigh in here?

I agree that the fact that the call into nr_interface_prioritizer_get_priority dispatches to an application-supplied function does give us a problem in nICEr in general. However, I'll note that this interface hasn't been upstreamed yet, so this flaw doesn't exist in public nICEr. So, from that perspective, we need to only worry about about what Firefox does with this callback at the moment.

On a cursory read, I agree with Byron's analysis that the remaining failures that could trigger this situation are largely "can't happens," such as IPv6 addresses and OOM conditions. The one that is a little worrisome, since it can be forced by an outside party, is interface priority exhaustion, as would occur if someone created more than 128 interfaces on the host.

That said, the call tree gets pretty deep here, and a thorough manual audit can't plausibly catch every corner case that might arise.

In terms of our own user-supplied function in nrinterfaceprioritizer.cpp, and assuming nICEr is guaranteed to call "sort" before asking for I priority (I didn't verify this, but assume it must), I think we're pretty failure-proof (note, however, that we've had bugs in the past where interface names didn't match what we expected, which makes it difficult to *guarantee* success).

All of which is to say I think there's probably a real problem here, although it would be obscure, and I can't identify a plausible exploit for it in the time I've spent looking at it.

Comment 8

[Adam Roach [:abr]]

With regards to upstreaming, I agree that we'll want to land this patch upstream shortly after committing it to the Mozilla tree. I'll gladly note that Byron has commit access to the resiprocate repo. ;-)

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 8337868 [details] [diff] [review]
Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

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

Adam, please review.

Byron, once Adam is done, please ask for sec-approval for Aurora and consult
with security on Beta (this should be an issue there, if it is one).

Comment 10

[Adam Roach [:abr]]

Comment on attachment 8337868 [details] [diff] [review]
Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

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

Looks good, with one proposed improvement to avoid a future obscure leak.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +527,5 @@
>            ABORT(r);
>          }
>          nr_ice_candidate_pair_set_state(pair->pctx,pair,NR_ICE_PAIR_STATE_FROZEN);
>  
>          if(r=nr_ice_candidate_pair_insert(&comp->stream->check_list,pair)) {

Since we're poking around at memory management in the face of errors, I think we have a chance to leak the newly created pair if this insert fails. Admittedly, this can't *currently* fail, but we probably want to add a nr_ice_candidate_pair_destroy() inside the failure here in case future implementation changes that.

Comment 11

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #9)
> Comment on attachment 8337868 [details] [diff] [review]
> Fix bug where a destroyed peerreflexive candidate could be left in the
> component list if candidate pair creation/insertion failed for some reason.
> 
> Review of attachment 8337868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Adam, please review.
> 
> Byron, once Adam is done, please ask for sec-approval for Aurora and consult
> with security on Beta (this should be an issue there, if it is one).

I'm assuming this will need to go into the release version too? So do I set the sec-approval, approval‑mozilla‑aurora, approval‑mozilla‑beta, and approval‑mozilla‑release flags?

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Byron Campen [:bwc] from comment #11)
> I'm assuming this will need to go into the release version too? So do I set
> the sec-approval, approval‑mozilla‑aurora, approval‑mozilla‑beta, and
> approval‑mozilla‑release flags?

Just do sec-approval first, which will let you land it on trunk.  Once it has been on trunk enough that you know it is stable, you can ask for approval to land on aurora (beta is a lost cause at this point).

Comment 13

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #10)
> Comment on attachment 8337868 [details] [diff] [review]
> Fix bug where a destroyed peerreflexive candidate could be left in the
> component list if candidate pair creation/insertion failed for some reason.
> 
> Review of attachment 8337868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, with one proposed improvement to avoid a future obscure leak.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
> @@ +527,5 @@
> >            ABORT(r);
> >          }
> >          nr_ice_candidate_pair_set_state(pair->pctx,pair,NR_ICE_PAIR_STATE_FROZEN);
> >  
> >          if(r=nr_ice_candidate_pair_insert(&comp->stream->check_list,pair)) {
> 
> Since we're poking around at memory management in the face of errors, I
> think we have a chance to leak the newly created pair if this insert fails.
> Admittedly, this can't *currently* fail, but we probably want to add a
> nr_ice_candidate_pair_destroy() inside the failure here in case future
> implementation changes that.

Ok, I'll put up a separate patch for that one-liner.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8337868 [details] [diff] [review]
Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

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

This bug is present on at least aurora and beta, and probably released versions as well.

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8337868 [details] [diff] [review]
Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

Clearing sec-approval flag, so I can try another way and maybe get the template this time.

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8337868 [details] [diff] [review]
Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

    Not 100% sure. There are lots of places where an error could trigger this bug, and I have not had time to delve down into the implementation of the interface prioritizer, the NR_registry stuff, or nr_socket_getaddr and determine which would be easiest to make fail.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   They make it extremely easy to identify the flaw, but figuring out how to actually trigger it would require some work.

Which older supported branches are affected by this flaw?

   Everything back to FFX 22 is affected, but we only support back to 24esr.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

   The backports should be trivial, as the code is basically the same modulo stuff like whitespace and maybe log levels.

How likely is this patch to cause regressions; how much testing does it need?

   Since this code has not changed much, I suspect that running the current test-cases would be more than sufficient. I do not expect this change to cause any regressions.

Comment 17

[Byron Campen [:bwc]]

Some further comments re sec-approval; this bug was discovered solely through code inspection, and has not been reproduced at any point, nor do we have a concrete way of triggering it in mind. In order to trigger this bug, one of the following things would do the trick (there might be others):

1.) Run the interface prioritization code (this is code that looks at local network addresses, and assigns each a priority) out of automatic preference values. This requires 224 local network addresses to be identified by the code. This seems to be pretty hard to trigger remotely.

2.) Confuse the interface prioritization code enough that it returns an error response when nr_interface_prioritizer_get_priority is called (again, this is strictly internal stuff, so you would probably have to induce the OS to do something weird enough that the prioritization code gets confused).

3.) Confuse the NR_registry implementation in some way (ie; somehow confuse an internal configuration store).

4.) Confuse the implementation of nr_socket_getaddr into returning an error (this seems to be impossible in our codebase).

Comment 18

[Byron Campen [:bwc]]

Sorry, s/local network addresses/local network interfaces/g in above comment. The former is pretty easy, the latter not so much.

Comment 19

[Byron Campen [:bwc]]

Attachment: Part 2. As long as we're working on the memory management in here, plug a potential leak.

Fix for abr

Comment 20

[Byron Campen [:bwc]]

Comment on attachment 8342046 [details] [diff] [review]
Part 2. As long as we're working on the memory management in here, plug a potential leak.

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

Keeping this separate from the stuff that needs sec-approval.

Comment 21

[Adam Roach [:abr]]

Comment on attachment 8342046 [details] [diff] [review]
Part 2. As long as we're working on the memory management in here, plug a potential leak.

LGTM

Comment 22

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

We need to get a security rating on this before it can be approved to go in. How bad is this as a potential security exploit? As a use-after-free, it looks sec-critical but comments downplay it since it is from code inspection.

Comment 23

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

The current belief (from comment 17) is that there's no way to trigger this failure without control of the local machine (to add hundreds of bogus local interfaces) or a wrapper around mozilla, or presuming another bug in the registry code that we don't know to exist, or by accessing internal-only interfaces.  So from that it's unclear to me what the final rating would be.  Worst-case would be critical, but the analysis would seem to downgrade that per comment 17.

Comment 24

[Daniel Veditz [:dveditz]]

Sounds good enough to downgrade this to sec-moderate

Comment 25

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

Comment on attachment 8337868 [details] [diff] [review]
Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

As a sec-moderate, this doesn't need approval to go in then. Check in at leisure.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22b9018bf95c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5544b1b9b81e

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22b9018bf95c
https://hg.mozilla.org/mozilla-central/rev/5544b1b9b81e

B2g26 (v1.2) is presumably affected by this? Should we nominate this for koi?

Comment 28

[Maire Reavy [:mreavy]]

v1.2 doesn't support PeerConnection, so this fix shouldn't be needed there.

Comment 29

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

Comment on attachment 8337868 [details] [diff] [review]
Fix bug where a destroyed peerreflexive candidate could be left in the component list if candidate pair creation/insertion failed for some reason.

[Approval Request Comment]
See earlier s-a request.  This affects 27 as well, and is landed in 28.

Comment 30

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/8de854b69d18

Comment 31

[u279076]

I don't think this needs QA verification. If anyone thinks that's a mistake please remove the [qa-] whiteboard tag and add the verifyme keyword.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Do we need this on b2g26 (v1.2)?

Comment 33

[Byron Campen [:bwc]]

Nope, since we don't support PeerConnections on 1.2.