search

voipmonitor / sniffer

VoIPmonitor sniffer sources
226 stars 105 forks source link

keep stun and dtls packets intact in rtpheader mode #67

Closed kritarthh closed 4 years ago

kritarthh commented 4 years ago

When savertp = header is used, stun and dtls packets are also truncated and they become of no use. So I have added checks to exclude these packets based on https://tools.ietf.org/html/rfc5764#section-5.1.2

voipmonitor commented 4 years ago

Hi, the patch does not work in a way we want. Can you please attach pcap file with stun and dtls? Why truncated dtls packet is of no use? Purpose of savertp = header is that you are not able to decrypt and not able to listen to a call

kritarthh commented 4 years ago

Please find the pcaps showing the behaviour both before and after this patch. pcaps.zip

voipmonitor commented 4 years ago

this part:

} else if((this->flags & FLAG_SAVERTP) || this->isfax || record_dtmf || packetS->isStun() || packetS->isDtls()) { save_packet(this, packetS, TYPE_RTP, forceVirtualUdp); } should be:

} else if((this->flags & FLAG_SAVERTP) || this->isfax || record_dtmf ||) { save_packet(this, packetS, TYPE_RTP, forceVirtualUdp); }

as if SAVERTP is not enabled you should not store STUN or DTLS at all?

kritarthh commented 4 years ago

Okay, I have missed savertp = no case. In this you would not want to store any rtp, dtls or stun. But for savertp = header case, we want to store dtls and stun as it is. Something like this should work: stun_dtls.patch.txt

voipmonitor commented 4 years ago

but this will not work if you will have savertp = yes, it will work only for savertp = header

On Wed, Jun 10, 2020 at 6:45 PM Kritarth notifications@github.com wrote:

Okay, I have missed savertp = no case. In this you would not want to store any rtp, dtls or stun. But for savertp = header case, we want to store dtls and stun as it is. Something like this should work: stun_dtls.patch.txt https://github.com/voipmonitor/sniffer/files/4760002/stun_dtls.patch.txt

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/voipmonitor/sniffer/pull/67#issuecomment-642130017, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVX7J3Q4CRKGX2EX4THMHLRV62DFANCNFSM4NTHLFYA .

-- Best regards Martin Vit

voipmonitor commented 4 years ago

have you tested all three options - savertp = no, savertp = header and savertp = yes if they behave correctly for STUN and DTLS now?

On Thu, Jun 11, 2020 at 2:35 PM Kritarth notifications@github.com wrote:

@kritarthh commented on this pull request.

In calltable.cpp https://github.com/voipmonitor/sniffer/pull/67#discussion_r438749253:

if(enable_save_packet) {

if((this->silencerecording || (this->flags & FLAG_SAVERTPHEADER)) && !this->isfax && !record_dtmf) {

  • if(packetS->datalen_() >= RTP_FIXED_HEADERLEN &&
  • packetS->headerpt->caplen > (unsigned)(packetS->datalen() - RTP_FIXED_HEADERLEN)) {
  • if(packetS->isStun() || packetS->isDtls()) {
  • save_packet(this, packetS, TYPE_RTP, forceVirtualUdp);
  • } else if(packetS->datalen_() >= RTP_FIXED_HEADERLEN &&
  • packetS->headerpt->caplen > (unsigned)(packetS->datalen() - RTP_FIXED_HEADERLEN)) { unsigned int tmp_u32 = packetS->header_pt->caplen; packetS->header_pt->caplen = min(packetS->headerpt->caplen - (packetS->datalen() - RTP_FIXEDHEADERLEN), packetS->dataoffset() + RTP_FIXED_HEADERLEN);

I have moved STUN and DTLS checks inside the savertp = header case, so this makes sure that savertp = yes case conditions are not altered. Also savertp = no does not even reach this flow.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/voipmonitor/sniffer/pull/67#pullrequestreview-428863565, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVX7J6R4GM2OENYADDLMATRWDFQXANCNFSM4NTHLFYA .

-- Best regards Martin Vit

kritarthh commented 4 years ago

yes, i have tested with savertp = yes,no,header no - no rtp, no stun, no dtls yes - rtp, stun, dtls header - stun, dtls, only rtp header

voipmonitor commented 4 years ago

thank you, code merged to the develop branch