824960 - WebRTC use-after-free crash [@mozilla::DataChannelConnection::SendOpenAckMessage]

Status: RESOLVED DUPLICATE of bug 821071

(Core :: WebRTC: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

alloc: ./media/mtransport/third_party/nICEr/src/ice/ice_parser.c:221

    /* Assume v4 for now */
    if(r=nr_ip4_port_to_transport_addr(ntohl(addr),port,IPPROTO_UDP,&cand->addr))
      ABORT(r);


free: ./media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:420

    r_log(NR_LOG_STUN,LOG_DEBUG,"STUN-CLIENT(%s): Received(my_addr=%s,peer_addr=%s)",ctx->label,ctx->my_addr.as_string,peer_addr->as_string);


re-use: ./netwerk/sctp/datachannel/DataChannel.cpp:755

DataChannelConnection::SendOpenAckMessage(uint16_t streamOut)
{
  struct rtcweb_datachannel_ack ack;

  memset(&ack, 0, sizeof(struct rtcweb_datachannel_ack));
[...]


Tested with m-c changeset: 117036:f5ed2691d901

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

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

Ok, I'm very confused here.  Are you *sure* the lines identified are correct?  Maybe you had the wrong rev? 

As best I can tell, there's no allocation where it says (r is an int), there's no free where it says (let alone of an object from the alloc location), and the re-use location is a routine that memsets a stack buffer to 0.  And there's no way DataChannelConnection::anything is called from AudioDeviceMac::RecordingIsAvailable()

    #0 0x1053ed58b in mozilla::DataChannelConnection::SendOpenAckMessage DataChannel.cpp:755
    #1 0x108b4bd38 in webrtc::AudioDeviceMac::RecordingIsAvailable audio_device_mac.cc:1262


I believe something got flagged.  I can't see how this set of locations could possibly be the source of it.

Comment 3

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

This is also not a possible pairing:
    #7 0x108b5609a in webrtc::AudioDeviceMac::outConverterProc audio_device_mac.cc:2786
    #8 0x10506086c in nsSocketTransportService::ClosePrivateConnections nsSocketTransportService2.cpp:895

This is impossible:
    #4 0x108bd6957 in nr_ice_peer_candidate_from_attribute ice_parser.c:221
    #5 0x10777a34e in nsZipWriter::AddEntryChannel nsZipWriter.cpp:434


Basically the entire stack is fubarred.

Comment 4

[Christoph Diehl [:posidron]]

Yea, I was wondering while putting the source in the bug that this does not cover the logic of a UAF crash but forgot to re-check it because it was filed during my holidays. I am sorry for that.

I think we should ignore that bug or mark it as invalid right now if there is no useable information in the call stack at all. I haven't seen that crash signature again.

Comment 5

[Christoph Diehl [:posidron]]

Oh, wait there was a testcase in this bug. Let me re-check that again in more detail with that specific revision. I will do this tomorrow, have right now no available disk space left.

Comment 6

[Christoph Diehl [:posidron]]

Attachment: callstack - lsm_open_rx

It might be a case where the symbolizer produced wrong output. Although, the crash signature that a use-after-free happened, is true. Wired, till now I was only able to reproduce a heap-overflow condition which is already known and stack-overflow condition which might be a false positive (see new attachment). 

I have also updated ASan to r171858 btw.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Christoph, is the attached testcase still valid?

Comment 8

[Christoph Diehl [:posidron]]

Yes but only for the stack-overflow condition. Ethan will need to verify that the callstack is not a false positive. If it is a false positive this bug can be marked as invalid otherwise we should adjust the name of the bug.

Comment 9

[Ethan Hugg [:ehugg]]

This one is a bit confusing.  The error says the stack is this

  This frame has 5 object(s):
    [32, 36) 'port_allocated'
    [96, 100) 'sdpmode'
    [160, 168) 'candidates'
    [224, 228) 'candidate_ct'
    [288, 296) 'default_addr'

The offending line is this:

          if (!status) {

And from the code, the top of the stack should look more like this:

          char **candidates;
          int candidate_ct;
          char *default_addr;
          short status;

So, it's as if 'status' got lost as the top of the stack due to something in the vcmRxAllocICE call perhaps?

I'm unable to reproduce this on my OSX ASan build with our without gdb, and I am unsure how to verify that the callstack is not a false positive.  Any guidance or more stackdumps would be helpful.

Comment 10

[Christoph Diehl [:posidron]]

The callstack looks a bit like of bug 821071 except that it calls fsmdef_ev_setremotedesc instead of fsmdef_ev_createoffer.

Comment 11

[Ethan Hugg [:ehugg]]

OK, perhaps I was just confused because I was on the wrong changeset which had Adam's fix in it already.  Although the fix for bug 821071 was pushed 12/31 and the callstack here was posted 1/10 so it should have had that patch in it, correct?  If not it would make sense to declare this one a duplicate of 821071 if it cannot be reproduced with the latest.

Comment 12

[Christoph Diehl [:posidron]]

This bug got found with m-c changeset: 117036:f5ed2691d901
The bug 821071 got found with m-i changeset: 115815:ec65853affc8

Comment 13

[Ethan Hugg [:ehugg]]

Bug 821071 was fixed in m-c changeset 117269:6a15f30fdfb3 on Dec 31st. after the report of this bug.