Numerous UAF bugs in nr_stun_message_add_*_attribute

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

({csectype-uaf})

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

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

Comment 2

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

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

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

Comment 10

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

Comment 11

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

Comment 12

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

Comment 13

4 years ago
Posted patch Part 1: Test case (obsolete) — Splinter Review
Assignee

Updated

4 years ago
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Assignee

Updated

4 years ago
Attachment #8589968 - Attachment is obsolete: true
Assignee

Comment 15

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

Is this test-case worth checking in?
Attachment #8589968 - Attachment filename: . → test_case.patch
Attachment #8589968 - Attachment is obsolete: false
Attachment #8589968 - Flags: feedback?(ekr)
Assignee

Comment 16

4 years ago
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
Attachment #8589969 - Attachment filename: . → destroy_bogus_attributes.patch
Attachment #8589969 - Flags: review?(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 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.
Attachment #8589968 - Flags: feedback?(ekr) → review+
(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 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.
Attachment #8589969 - Flags: review?(ekr) → review+
(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.
Keywords: sec-high
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.
Assignee

Comment 23

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

Comment 24

4 years ago
Incorporate feedback.
Assignee

Comment 25

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

Carry forward r=ekr.
Attachment #8594886 - Attachment filename: . → test_case.patch
Attachment #8594886 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8589968 - Attachment is obsolete: true
Assignee

Comment 27

4 years ago
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.
Attachment #8594887 - Attachment filename: . → destroy_bogus_attributes.patch
Attachment #8594887 - Flags: review+
Assignee

Updated

4 years ago
Attachment #8589969 - Attachment is obsolete: true
Assignee

Comment 28

4 years ago
Since this is not exploitable as far as I can tell, we should probably un-set the sec flag.
Keywords: sec-high
Assignee

Updated

4 years ago
Flags: needinfo?(docfaraday)
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/a537946a4262
https://hg.mozilla.org/mozilla-central/rev/e2009551c1be
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.