Closed Bug 942958 Opened 6 years ago Closed 6 years ago

Incorrect error code if we fail to populate a peer reflexive candidate's foundation string

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bwc, Assigned: bwc)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

Minor typo results in an out param not being set, but no error code being returned here, since we call ABORT(0):

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

The calling code dereferences the out param |pcand| without checking here:

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

Calling this a sec bug at ekr's request, but it is fairly minor for a sec bug, since it is strictly a null pointer crash, and getting it to happen requires a pretty weird failure.
Attachment #8337894 - Attachment is obsolete: true
If it is a true nullptr crash, and no possibility of anything else, we can drop the sec attribute
Keywords: crash
Clear the sec-low flag if you unhide it.
Attachment #8337987 - Flags: review?(ekr)
Comment on attachment 8337987 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would not result in an error return from nr_ice_peer_peer_rflx_candidate_create, resulting in nr_ice_component_process_incoming_check dereferencing a NULL pointer.

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

Please replace with R_NO_MEMORY

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +235,5 @@
>      if(r=nr_transport_addr_copy(&cand->addr,addr))
>        ABORT(r);
>      /* Bogus foundation */
>      if(!(cand->foundation=r_strdup(cand->addr.as_string)))
> +      ABORT(R_BAD_DATA);

You're right that we should do something other than ABORT(r) here, but
I believe your analysis of the consequences is wrong.

ABORT() checks for you to pass it a zero and if you do, it substitutes
a nonzero value.

See:
http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/util/libekr/r_macros.h#112

The right return value here is R_NO_MEMORY
Attachment #8337987 - Flags: review?(ekr)
Byron,

Please provide a new patch per c5. If you believe that my analysis of the
consequences is correct, then you can remove the security flags and
land.
Flags: needinfo?(docfaraday)
(In reply to Eric Rescorla (:ekr) from comment #6)
> Please provide a new patch per c5. If you believe that my analysis of the
> consequences is correct, then you can remove the security flags and
> land.

FWIW, I agree that this is a trivial flaw (wrong error code) rather than a nullptr deref. I would adjust the bug title to reflect this fact, and remove the bug's various security decorations.
I agree with the analysis as well, but I do not have the privileges to untick the "Security-Sensitive Core Bug" checkbox. Someone who does should probably clear the rest of the sec flags too. I will have a patch shortly.
Flags: needinfo?(docfaraday) → needinfo?(adam)
Group: core-security
Flags: needinfo?(adam)
Attachment #8337987 - Attachment is obsolete: true
Comment on attachment 8341843 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would not result in an error return from nr_ice_peer_peer_rflx_candidate_create, resulting in nr_ice_component_process_incoming_check dereferencing a NULL pointer.

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

Carry forward r+, and request checkin.
Attachment #8341843 - Flags: review+
Attachment #8341843 - Flags: checkin?(adam)
Keywords: crash
Summary: Potential null pointer dereference if we fail to populate a peer reflexive candidate's foundation string → Incorrect error code if we fail to populate a peer reflexive candidate's foundation string
Comment on attachment 8341843 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would not result in an error return from nr_ice_peer_peer_rflx_candidate_create, resulting in nr_ice_component_process_incoming_check dereferencing a NULL pointer.

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

Clear checking request, as description was wrong.
Attachment #8341843 - Flags: checkin?(adam)
Attachment #8341843 - Attachment is obsolete: true
Comment on attachment 8341846 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would result in an incorrect error return from nr_ice_peer_peer_rflx_candidate_create.

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

Carry forward r+, requesting checkin for real this time.
Attachment #8341846 - Flags: review+
Attachment #8341846 - Flags: checkin?(adam)
Comment on attachment 8341846 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would result in an incorrect error return from nr_ice_peer_peer_rflx_candidate_create.

Carry forward r+? I don't see any given here or previous comments.
Attachment #8341846 - Flags: checkin?(adam)
Comment on attachment 8341846 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would result in an incorrect error return from nr_ice_peer_peer_rflx_candidate_create.

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

Apparently this never got marked r+
Attachment #8341846 - Flags: review+ → review?(ekr)
Comment on attachment 8341846 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would result in an incorrect error return from nr_ice_peer_peer_rflx_candidate_create.

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

lgtm
Attachment #8341846 - Flags: review?(ekr) → review+
Attachment #8341846 - Flags: checkin?(adam)
Comment on attachment 8341846 [details] [diff] [review]
Fix bug where a failure to populate the foundation of a peerreflexive candidate would result in an incorrect error return from nr_ice_peer_peer_rflx_candidate_create.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ede459abb05

Also, you can just use the checkin-needed keyword for bugs like these. The checkin? flag is mostly useful for bugs with multiple patches landing at different times.
Attachment #8341846 - Flags: checkin?(adam) → checkin+
https://hg.mozilla.org/mozilla-central/rev/6ede459abb05
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.