1152137 - Numerous UAF bugs in nr_stun_message_add_*_attribute

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

Many of the attribute adding functions defined in stun_msg.c do the wrong thing when an error is encountered. Here's one example:

int
nr_stun_message_add_xor_mapped_address_attribute(nr_stun_message *msg, nr_transport_addr *mapped_address)
NR_STUN_MESSAGE_ADD_ATTRIBUTE(
    NR_STUN_ATTR_XOR_MAPPED_ADDRESS,
    {
        if ((r=nr_transport_addr_copy(&attr->u.xor_mapped_address.unmasked, mapped_address)))
            ABORT(r);
    }
)

NR_STUN_MESSAGE_ADD_ATTRIBUTE is a boilerplate macro that wraps the the code that fills out the attribute:

#define NR_STUN_MESSAGE_ADD_ATTRIBUTE(__type, __code) \
  { \
    int r,_status; \
    nr_stun_message_attribute *attr = 0; \
    if ((r=nr_stun_message_attribute_create(msg, &attr))) \
        ABORT(r); \
    attr->type = (__type); \
    { __code } \
    _status=0; \
  abort: \
    if (_status) RFREE(attr); \
    return(_status); \
  }

The above code expands to the following (modulo whitespace):

int
nr_stun_message_add_xor_mapped_address_attribute(nr_stun_message *msg, nr_transport_addr *mapped_address)
  {
    int r,_status;
    nr_stun_message_attribute *attr = 0;
    if ((r=nr_stun_message_attribute_create(msg, &attr)))
        ABORT(r);
    attr->type = ( NR_STUN_ATTR_XOR_MAPPED_ADDRESS );
    { {
        if ((r=nr_transport_addr_copy(&attr->u.xor_mapped_address.unmasked, mapped_address)))
            ABORT(r);
    } }
    _status=0;
  abort:
    if (_status) RFREE(attr);
    return(_status);
  }

If we ABORT due to a failure in nr_transport_addr_copy, we free the attr but do not remove it from the stun message. This sets up a UAF later, which can include dumping the freed memory onto the wire.

I have not spent the time yet to determine how likely it is that any of these setters fail in this way, so no security rating yet, but this is the kind of thing that could be elicited at will with crafted network traffic.

Comment 1

[Byron Campen [:bwc]]

I think this bug may be the cause of the following reports on crash-stats:

https://crash-stats.mozilla.com/signature/?date=%3E01%2F01%2F2015&signature=nr_stun_message_has_attribute&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1

https://crash-stats.mozilla.com/signature/?date=%3E01%2F01%2F2015&signature=nr_stun_message_attribute_create&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1

Comment 2

[Byron Campen [:bwc]]

Here are the functions we actually need to worry about:

nr_stun_message_add_message_integrity_attribute
   used, but a little complicated to trace through. Looks like it might be possible to cause a failure here reliably, will need to investigate.

nr_stun_message_add_data_attribute
   used, seems like we could hit this if we tried to send too large of a packet over TURN

There are others that have an ABORT, but that are not a threat:

nr_stun_message_add_alternate_server_attribute
   unused

nr_stun_message_add_xor_mapped_address_attribute
nr_stun_message_add_xor_peer_address_attribute
   used, but nr_transport_addr_copy would have to fail, which is not currently possible

Comment 3

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #2)
> Here are the functions we actually need to worry about:
> 
> nr_stun_message_add_message_integrity_attribute
>    used, but a little complicated to trace through. Looks like it might be
> possible to cause a failure here reliably, will need to investigate.
> 
> nr_stun_message_add_data_attribute
>    used, seems like we could hit this if we tried to send too large of a
> packet over TURN
> 
> There are others that have an ABORT, but that are not a threat:
> 
> nr_stun_message_add_alternate_server_attribute
>    unused
> 
> nr_stun_message_add_xor_mapped_address_attribute
> nr_stun_message_add_xor_peer_address_attribute
>    used, but nr_transport_addr_copy would have to fail, which is not
> currently possible

These generally cause return with error, which it seems might lead
to a double free. See for instance:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_build.c#106

int
nr_stun_build_req_lt_auth(nr_stun_client_stun_binding_request_params *params, nr_stun_message **msg)
{
   int r,_status;
   nr_stun_message *req = 0;

   if ((r=nr_stun_form_request_or_indication(NR_STUN_MODE_STUN, NR_STUN_MSG_BINDING_REQUEST, &req)))
       ABORT(r);

   if ((r=nr_stun_message_add_username_attribute(req, params->username)))
       ABORT(r);

   if (params->realm && params->nonce) {
       if ((r=nr_stun_message_add_realm_attribute(req, params->realm)))
           ABORT(r);

       if ((r=nr_stun_message_add_nonce_attribute(req, params->nonce)))
           ABORT(r);

       if ((r=nr_stun_message_add_message_integrity_attribute(req, params->password)))
           ABORT(r);
   }

   *msg = req;

   _status=0;
 abort:
   if (_status) nr_stun_message_destroy(&req);
   return _status;
}

If nr_stun_add_message_integrity_attribute() fails, then you end up in nr_stun_message_destroy(),
which tries to free this.


Generally, if it's going to push the freed data onto the wire it would need to be
a function which ignored the return code, right?

Comment 4

[Byron Campen [:bwc]]

That's something I'll need to figure out on a case-by-case basis.

Comment 5

[Byron Campen [:bwc]]

If we hit a failure in the following call stack, we might just dump uninitialized memory onto the wire because we try to send an error response anyway, and we never touch the memory that we alloced for the attribute:

nr_stun_message_add_message_integrity_attribute
nr_stun_form_success_response
nr_stun_server_process_request (either of the calls to nr_stun_form_success_response will do the trick)

Need to determine what it would take to hit this error.

Comment 6

[Byron Campen [:bwc]]

I cannot find any callstacks where a failure in nr_stun_message_add_data_attribute results in an attempt to do anything other than free the stun message.

Comment 7

[Byron Campen [:bwc]]

Looks like this is going to be a problem:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c?from=nr_stun_server_process_request&case=true#314

if ((r=nr_stun_form_success_response(req, peer_addr, &clnt->password, res)))

clnt->password is set by the following call stack:

nr_stun_server_client_create
nr_stun_server_add_client
nr_ice_component_pair_candidates, which passes pcomp->stream->r2l_pass

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

r2l_pass is set by nr_ice_peer_ctx_parse_stream_attributes, based on the value of stream->pwd

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c#131
https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c#143

stream->pwd is set by nr_ice_peer_ctx_parse_media_stream_attribute, which parses remote SDP

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/ice/ice_parser.c#383

My suspicion is that if the remote SDP contains an ice-pwd attribute greater than 2048 characters long, we'll trip over this bug. The question is, will we be able to emit anything?

Comment 8

[Byron Campen [:bwc]]

Ok, this was a close call. We are saved by some earlier validation code:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/stun_codec.c#1457

|get_password| is nr_stun_server_get_password, which uses the same clnt->password as the code in comment 7, and then length checks it in the same way as nr_stun_message_add_message_integrity_attribute. If this length check fails, an error response is sent, and we never get to the code that tries to send a response.

Comment 9

[Byron Campen [:bwc]]

Now, I need to figure out whether we can get a "mere" double-free out of this bug.

Comment 10

[Byron Campen [:bwc]]

I cannot find any other way to get a password that is too large to nr_stun_message_add_message_integrity_attribute, since everywhere else we use this is for requests, which uses our own password that is set by nICEr (we do not honor rewriting of ice-pwd in local SDP).

Comment 11

[Byron Campen [:bwc]]

nr_stun_message_add_data_attribute is used for send indications, which means we need to be concerned about whether an RTP packet could ever get large enough to cause a failure (> 2048 bytes), but it seems unlikely.

Comment 12

[Byron Campen [:bwc]]

Ok, I'm pretty sure this is not exploitable right now. Fix coming right up.

Comment 13

[Byron Campen [:bwc]]

Attachment: Part 1: Test case

Comment 14

[Byron Campen [:bwc]]

Attachment: Part 2: Remove attributes that could not be initted properly instead of just freeing them

Comment 15

[Byron Campen [:bwc]]

Comment on attachment 8589968 [details] [diff] [review]
Part 1: Test case

Is this test-case worth checking in?

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 8589969 [details] [diff] [review]
Part 2: Remove attributes that could not be initted properly instead of just freeing them

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f435a161b1e

Comment 17

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] (PTO Apr 10-19) from comment #11)
> nr_stun_message_add_data_attribute is used for send indications, which means
> we need to be concerned about whether an RTP packet could ever get large
> enough to cause a failure (> 2048 bytes), but it seems unlikely.

IIRC, VideoEngine and VoiceEngine don't do a good (any?) job of PMTU
discovery and so they limit packets to ~1200bytes.

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 8589968 [details] [diff] [review]
Part 1: Test case

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

I don't need to see this again, but I'd like you to at least fix the ASSERTs.

::: media/mtransport/test/ice_unittest.cpp
@@ +2304,5 @@
>  }
>  
> +TEST(InternalsTest, TestAddBogusAttribute) {
> +  nr_stun_message *req;
> +  ASSERT_FALSE(nr_stun_message_create(&req));

I'm kind of sad about these ASSERT macros, since in this context TRUE is failure and FALSE is success. The convention in the surrounding code seems to be to use ASSERT_EQ(0) etc.

@@ +2309,5 @@
> +  std::string bigPwd(3000, 'A');
> +  Data *data;
> +  ASSERT_FALSE(r_data_create(&data,
> +                             reinterpret_cast<const unsigned char*>(bigPwd.c_str()),
> +                             bigPwd.size()));

Note: you could use r_data_alloc() followed by memset instead. That might be cleaner.

Comment 19

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

(In reply to Eric Rescorla (:ekr) from comment #17)
> (In reply to Byron Campen [:bwc] (PTO Apr 10-19) from comment #11)
> > nr_stun_message_add_data_attribute is used for send indications, which means
> > we need to be concerned about whether an RTP packet could ever get large
> > enough to cause a failure (> 2048 bytes), but it seems unlikely.
> 
> IIRC, VideoEngine and VoiceEngine don't do a good (any?) job of PMTU
> discovery and so they limit packets to ~1200bytes.

Agreed, but they might probe eventually, and larger packets are possible in GB ethernet, etc.  So long as there's a clean failure path with a scream of pain if it fails, that should be good for now.

Comment 20

[Eric Rescorla (:ekr)]

Comment on attachment 8589969 [details] [diff] [review]
Part 2: Remove attributes that could not be initted properly instead of just freeing them

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

lgtm.

::: media/mtransport/third_party/nICEr/src/stun/stun_msg.c
@@ +185,5 @@
>      attr->type = (__type); \
>      { __code } \
>      _status=0; \
>    abort: \
> +    if (_status) nr_stun_message_attribute_destroy(msg, &attr); \

Let's brace this clause for easier reading.

Comment 21

[Daniel Veditz [:dveditz]]

(In reply to Byron Campen [:bwc] (PTO Apr 10-19) from comment #12)
> Ok, I'm pretty sure this is not exploitable right now.

Some of the crashes you linked to seemed potentially so, why do you say this? There didn't seem too many of them, but that doesn't speak to how reliable or not such a crash could be if we knew the exact trigger.

Comment 22

[Eric Rescorla (:ekr)]

Byron is PTO now, but my understanding is that he believes that the crashes have some other cause and that the issue this patch fixes is a sharp object lying around but not something that we know how to trigger.

Comment 23

[Byron Campen [:bwc]]

(In reply to Daniel Veditz [:dveditz] from comment #21)
> (In reply to Byron Campen [:bwc] (PTO Apr 10-19) from comment #12)
> > Ok, I'm pretty sure this is not exploitable right now.
> 
> Some of the crashes you linked to seemed potentially so, why do you say
> this? There didn't seem too many of them, but that doesn't speak to how
> reliable or not such a crash could be if we knew the exact trigger.

   I'm pretty sure those crashes are caused by something else. What, I am not sure...

Comment 24

[Byron Campen [:bwc]]

Attachment: Part 1: Test case

Incorporate feedback.

Comment 25

[Byron Campen [:bwc]]

Comment on attachment 8594886 [details] [diff] [review]
Part 1: Test case

Carry forward r=ekr.

Comment 26

[Byron Campen [:bwc]]

Attachment: Part 2: Remove attributes that could not be initted properly instead of just freeing them

Incorporate feedback.

Comment 27

[Byron Campen [:bwc]]

Comment on attachment 8594887 [details] [diff] [review]
Part 2: Remove attributes that could not be initted properly instead of just freeing them

Carry forward r=ekr.

Comment 28

[Byron Campen [:bwc]]

Since this is not exploitable as far as I can tell, we should probably un-set the sec flag.

Comment 29

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1434dd258f25

Comment 30

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a537946a4262
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2009551c1be

Comment 31

[Byron Campen [:bwc]]

https://github.com/resiprocate/resiprocate/pull/13

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/a537946a4262
https://hg.mozilla.org/mozilla-central/rev/e2009551c1be