1008796 - Spurious success response in nr_ice_component_stun_server_default_cb

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Found by Sylveste Lendru using static analysis.

This function always returns 0 even if there is an error, because of the return 0 at the end. 

static int nr_ice_component_stun_server_default_cb(void *cb_arg,nr_stun_server_ctx *stun_ctx,nr_socket *sock, nr_stun_server_request *req, int *dont_free, int *error)
  {
    int r, _status;
    nr_ice_component *comp = (nr_ice_component *)cb_arg;
    nr_ice_pre_answer_request *par = 0;
    r_log(LOG_ICE,LOG_DEBUG,"ICE(%s)/STREAM(%s)/COMP(%d): Received STUN request pre-answer from %s",
          comp->ctx->label, comp->stream->label, comp->component_id, req->src_addr.as_string);

    if (r=nr_ice_pre_answer_request_create(sock, req, &par))
      ABORT(r);

    *dont_free = 1;
    STAILQ_INSERT_TAIL(&comp->pre_answer_reqs, par, entry);

    _status=0;
 abort:
    return 0;
  }


The only failure here is in:

nr_ice_pre_answer_request_create
(http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_component.c#54)


    if (!(par = RCALLOC(sizeof(nr_ice_pre_answer_request))))
      ABORT(R_NO_MEMORY);

    par->req = *req; /* Struct assignment */
    memset(req, 0, sizeof(*req)); /* Zero contents to avoid confusion */

    if (r=nr_socket_getaddr(sock, &par->local_addr))
      ABORT(r);
    if (!nr_stun_message_has_attribute(par->req.request, NR_STUN_ATTR_USERNAME, &attr))
      ABORT(R_INTERNAL);
    if (!(par->username = r_strdup(attr->u.username)))
      ABORT(R_NO_MEMORY);


So, this can happen with:

1. OOM.
2. Failure to get the address on the socket(shouldn't happen)
3. No username attribute (in which case we should never have
matched on the username field to get here in the first place).


The impact of a failure is that we never enqueue the request
for later processing (this is expected) and then since the
return value is used to determine if we send success or failure
(http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c#330)
we send an ICE success rather than a failure leaving the other
side in a half-complete state. It shouldn't allow us to send
packets when we otherwise should not.

Marking security because ICE is used to enforce consent checking
so this needs analysis to ensure there's no way that it could
cause spurious packets to be sent.

Byron, please verify above analysis and if you concur, have this
un-marked, verify that returning _status is safe here, and fix the
code accordingly.

Comment 1

[Byron Campen [:bwc]]

I agree that we will fail earlier if username is missing, and this code is not the place to be checking for general deficiencies in the request anyway. I think we're ok from a security standpoint. Once I am done with the current ICE PCP review cycle, I can put up a patch.

Comment 2

[Daniel Veditz [:dveditz]]

Byron: does that mean we can unhide the bug?

Comment 3

[Byron Campen [:bwc]]

I believe so, yes.

Comment 4

[Byron Campen [:bwc]]

Attachment: Fix return value in nr_ice_component_stun_server_default_cb.

Comment 5

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=a2ab46445f6d

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 8437827 [details] [diff] [review]
Fix return value in nr_ice_component_stun_server_default_cb.

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

lgtm

Comment 7

[Byron Campen [:bwc]]

Seeing a couple of failures on try, but neither seem to have anything to do with this.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef635e702a20

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef635e702a20