857256 - WebRTC TURN use-after-free crash [@nr_turn_client_process_response]

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

The crash occurred while running the IceConnectTest.TestConnectTurn test.

I am not sure how much this is related to bug 856382 (which is not reproducible anymore) or because I am running the unit test without a TURN server.


alloc: ./media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:304

int nr_turn_client_ctx_create(char *label, nr_socket *sock,
                              char *username, Data *password,
                              nr_transport_addr *addr,
                              nr_turn_client_ctx **ctxp)
{
[...]
  if(!(ctx=RCALLOC(sizeof(nr_turn_client_ctx))))
    ABORT(R_NO_MEMORY);


free: ./media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:347

int
nr_turn_client_ctx_destroy(nr_turn_client_ctx **ctxp)
{
  nr_turn_client_ctx *ctx;

  if(!ctxp || !*ctxp)
    return(0);

  ctx=*ctxp;
  *ctxp = 0;

  nr_turn_client_cancel(ctx);

  RFREE(ctx);


re-use: ./media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c:532

int nr_turn_client_process_response(nr_turn_client_ctx *ctx,
                                    UCHAR *msg, int len,
                                    nr_transport_addr *turn_server_addr)
{
  int r, _status;
  nr_turn_stun_ctx *sc1;

  switch (ctx->state) { 



Tested with m-i changeset: 126927:9fe83576713e and the applied patch of bug 786235.

Comment 1

[Eric Rescorla (:ekr)]

What do you mean "without a TURN server". What is it pointed at?

Comment 2

[Christoph Diehl [:posidron]]

std::string g_turn_server("172.16.131.149");

I was referring to your comment in the meeting where you said, that a TURN server will be required in order to run the TURN unit-tests but perhaps I misunderstood.

Comment 3

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

I'll note this would only rate as a security bug once the patch being tested lands, and then only if the problem hasn't already been resolved or made moot by changes to the patch.  I'm not sure if it should be sec-critical at this point, but it certainly would be if it lands as-id.

Comment 4

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

(In reply to Randell Jesup [:jesup] from comment #3)
> I'll note this would only rate as a security bug once the patch being tested
> lands

FYI, by "rate" I meant "rate sec-critical severity"

Comment 5

[Christoph Diehl [:posidron]]

This seems to be fixed with the updated patch https://bugzilla.mozilla.org/show_bug.cgi?id=786235#c18

Comment 6

[Jason Smith [:jsmith]]

The tracking flags were set incorrectly here - they should all be cleared since we haven't even landed the patch that generates the bug. No need to track for any Firefox release until there's code in the tree.

Comment 7

[Daniel Veditz [:dveditz]]

I disagree: there is a desire to get this feature into Firefox 23, and possibly uplift to 22, so we need to track this bug to make sure it's fixed should that happen.

Comment 8

[Jason Smith [:jsmith]]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> I disagree: there is a desire to get this feature into Firefox 23, and
> possibly uplift to 22, so we need to track this bug to make sure it's fixed
> should that happen.

Let's take this to a side email thread.

Comment 9

[Tracy Walker [:tracy]]

this crash is no longer appearing in report lists for 22 nor 23

Comment 10

[Alex Keybl [:akeybl]]

Bug 786235 suggests that FF22 should be explicitly unaffected, and FF23 sounds like it's fixed.

Comment 11

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

We have no intention to enable TURN for FF22.  TURN will be on for FF23

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this affected on FF23?

Comment 13

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

per comment 5, I'll mark FF23 fixed even though it never landed in the tree in the broken state.  Feel free to change to unaffected if that fits the process better.

Comment 14

[Jason Smith [:jsmith]]

Sounds like we've got our answer to comment 12's question in comment 13.

Comment 15

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #13)
> per comment 5, I'll mark FF23 fixed even though it never landed in the tree
> in the broken state.  Feel free to change to unaffected if that fits the
> process better.

Let's actually do unaffected in this case - that will work with our security QA's bug process for tracking bugs before landing vs. post landing.