Re: [h323plus] Possible misassumption in HandleFirstSignallingChannelPDU

-- Jan Willamowius, Founder of the GNU Gatekeeper Project EMail : jan@willamowius.de H.323 Support: https://www.willamowius.com/ Relaxed Communications GmbH Frahmredder 91 22393 Hamburg Geschäftsführer: Jan Willamowius HRB 125261 (Amtsgericht Hamburg) USt-IdNr: DE286003584 Jose Uceda wrote: > In our software that we mentioned a few days ago, in addition to the > lockups, we observed a problem when we apparently received an incoming > call, but CreateConnection is called with a valid PDU that is not a > Setup Message one. > > It happens when the incoming call comes from a Cisco gateway, and the > call hangs up at a point where the gateway has sent the Admission > Request to the gatekeeper, receives its corresponding admission confirm, > but has not sent yet the setup message to our software / destination > endpoint. > > In these circumstances we saw that the gateway has opened the signaling > socket with our endpoint, but the first packet it sends is a Release > Complete because the call was hung before sending the setup. > > > PDU trace from our Gatekeeper: > > 2017-05-27 14:46:28.853 5d37 INFO pdu.call.a.ARQ.recv : Received from ip$x.x.x.x:54432. > 2017-05-27 14:46:28.855 5d37 INFO pdu.call.a.ACF.send : Sending to ip$x.x.x.x:54432. > 2017-05-27 14:46:28.865 5d37 INFO pdu.call.d.DRQ.recv : Received from ip$x.x.x.x:54432. > 2017-05-27 14:46:28.866 5d37 INFO pdu.call.d.DCF.send : Sending to ip$x.x.x.x:54432. > > See the time-stamps, showing that the ACF and DRQ are closely followed. > We receive the call at our endpoint, in the CreateConnection function at > the same time point as the DRQ / DCF. > > In our software, in some cases we return NULL if we cannot handle the > incoming connection (not enough setup data, e.g.), and if we are running > an executable compiled in Debug mode, an assert fails on the following > operator: > > H225_H323_UU_PDU_h323_message_body :: operator const H225_Setup_UUIE > & () const > #endif > { > #ifndef PASN_LEANANDMEAN > PAssert (PIsDescendant (PAssertNULL (choice), H225_Setup_UUIE), > PInvalidCast); > #endif > Return * (H225_Setup_UUIE *) choice; > } > > > It is caused by the following line (transports.cxx: 1126): > > H225_Setup_UUIE & setup = pdu.m_h323_uu_pdu.m_h323_message_body; > > This code assumes that the initial PDU is a Setup one without checking > it, when it may not necessarily be. That can cause a DoS in a software > that has core dump enabled when an assert occurs, or if there is some > code in HandleFirstSignallingChannelPDU / OnIncomingConnection and > sub-calls that trust and handles the PDU object assuming it is a Setup > message. > > I've made the following patch on the HandleFirstSignallingChannelPDU > function, which verifies that the PDU is a Setup message, otherwise it > returns FALSE (same behavior as if it had not been able to read a valid > q931 pdu), since we think it does not make much sense to call > OnIncomingConnection if the initial PDU is not a Setup one: > > -------------------------------------------------------------------------------- > > --- src/transports.cxx.orig 2015-11-24 18:51:06.385755223 +0100 > +++ src/transports.cxx 2017-05-29 11:15:20.777411444 +0200 > @@ -1106,7 +1106,13 @@ > return FALSE; > } > > - unsigned callReference = pdu.GetQ931().GetCallReference(); > + const Q931 &firstQ931PDU = pdu.GetQ931(); > + if (firstQ931PDU.GetMessageType() != Q931::SetupMsg) { > + PTRACE(1, "H225\tFirst pdu is not a setup one, connection not > started."); > + return FALSE; > + } > + > + unsigned callReference = firstQ931PDU.GetCallReference(); > PTRACE(3, "H225\tIncoming call, first PDU: callReference=" << > callReference); > > // Get a new (or old) connection from the endpoint > > -------------------------------------------------------------------------------- > > A place where we could also put the patch is in the > HandleSignallingSocket function, which apparently is only used from > HandleFirstSignallingChannelPDU, but as it has a more generic name and > may give rise to doubts of its behavior. > > > Regards, > > Jose Uceda >