Potential dangling peerreflexive candidate on some types of failure.

RESOLVED FIXED in Firefox 27, Firefox OS v1.3

Status

()

Core
WebRTC: Networking
--
major
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({csectype-uaf, sec-moderate})

Trunk
mozilla28
csectype-uaf, sec-moderate
Points:
---

Firefox Tracking Flags

(firefox25 wontfix, firefox26 wontfix, firefox27 fixed, firefox28 fixed, firefox-esr24 wontfix, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.3T fixed)

Details

(Whiteboard: [qa-][adv-main27+])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Created 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.
Keywords: csectype-uaf, sec-critical
(Assignee)

Updated

4 years ago
Attachment #8337868 - Flags: review?(ekr)

Comment 2

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

Comment 3

4 years ago
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.
Keywords: sec-critical
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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

4 years ago
Adam, can you weigh in here?
Flags: needinfo?(adam)

Comment 7

4 years ago
(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.
Flags: needinfo?(adam)

Comment 8

4 years ago
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

4 years ago
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).
Attachment #8337868 - Flags: review?(ekr) → review?(adam)

Comment 10

4 years ago
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.
Attachment #8337868 - Flags: review?(adam) → review+
(Assignee)

Comment 11

4 years ago
(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?
(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).
(Assignee)

Comment 13

4 years ago
(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.
(Assignee)

Comment 14

4 years ago
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.
Attachment #8337868 - Flags: sec-approval?
(Assignee)

Updated

4 years ago
status-firefox25: --- → affected
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox-esr24: --- → affected
(Assignee)

Comment 15

4 years ago
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.
Attachment #8337868 - Flags: sec-approval?
(Assignee)

Comment 16

4 years ago
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.
Attachment #8337868 - Flags: sec-approval?
(Assignee)

Comment 17

4 years ago
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).
(Assignee)

Comment 18

4 years ago
Sorry, s/local network addresses/local network interfaces/g in above comment. The former is pretty easy, the latter not so much.
(Assignee)

Comment 19

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

Fix for abr
(Assignee)

Comment 20

4 years ago
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.
Attachment #8342046 - Flags: review?(adam)

Comment 21

4 years ago
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
Attachment #8342046 - Flags: review?(adam) → review+
status-firefox25: affected → wontfix
status-firefox26: affected → wontfix
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.
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.
Sounds good enough to downgrade this to sec-moderate
Keywords: sec-moderate
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.
Attachment #8337868 - Flags: sec-approval?
(Assignee)

Updated

4 years ago
Whiteboard: [checkin-needed]
Keywords: checkin-needed
Whiteboard: [checkin-needed]
https://hg.mozilla.org/integration/mozilla-inbound/rev/22b9018bf95c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5544b1b9b81e
Keywords: checkin-needed
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?
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → affected
status-firefox28: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
v1.2 doesn't support PeerConnection, so this fix shouldn't be needed there.
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.
Attachment #8337868 - Flags: approval-mozilla-aurora?
Attachment #8337868 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8de854b69d18
status-firefox27: affected → fixed
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.
Whiteboard: [qa-]
Do we need this on b2g26 (v1.2)?
status-b2g-v1.3: --- → fixed
Flags: needinfo?(docfaraday)
(Assignee)

Comment 33

4 years ago
Nope, since we don't support PeerConnections on 1.2.
Flags: needinfo?(docfaraday)
status-b2g-v1.2: affected → wontfix
status-firefox-esr24: affected → wontfix
Whiteboard: [qa-] → [qa-][adv-main27+]
status-b2g-v1.3T: --- → fixed
Group: core-security
You need to log in before you can comment on or make changes to this bug.