[h323plus] Possible misassumption in HandleFirstSignallingChannelPDU

[Jan Willamowius]

Hi Jose,

thanks for finding that bug!

I have committed your patch with minor changes to the CVS.

Regards,
Jan

-- 
Jan Willamowius, Founder of the GNU Gatekeeper Project
EMail : jan at 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
>