Skip to content

AmRtpStream::recvDtmfPacket: reject telephone-event packets with unexpected size - #552

Merged
hecko merged 2 commits into
masterfrom
hecko/amrtpstream-recvdtmf-size-guard
Sep 13, 2026
Merged

hecko merged 2 commits into
masterfrom
hecko/amrtpstream-recvdtmf-size-guard

Conversation

@hecko

@hecko hecko commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Bug

AmRtpStream::recvDtmfPacket (core/AmRtpStream.cpp) casts the RTP payload straight to dtmf_payload_t* and reads all of its fields without checking that the packet actually carries a full payload:

void AmRtpStream::recvDtmfPacket(AmRtpPacket* p) {
  if (p->payload == getLocalTelephoneEventPT()) {
    dtmf_payload_t* dpl = (dtmf_payload_t*)p->getData();
    DBG(... dpl->event, dpl->e, dpl->r, dpl->volume, ntohs(dpl->duration) ...);
    if (session)
      session->postDtmfEvent(new AmRtpDtmfEvent(dpl, getLocalTelephoneEventRate(), p->timestamp));
  }
}

dtmf_payload_t is 4 bytes (event, flags/volume byte, 2-byte duration). A telephone-event RTP packet carrying only 1–3 payload bytes still passes the non-empty-payload filter in receive() (if(!rp->getDataSize())), so the cast reads dpl->duration (and possibly volume) past the received data. The result is a bogus AmRtpDtmfEvent with garbage volume/duration posted to the session. The payload type and length are attacker-controlled, so a remote peer can inject spurious DTMF signaling.

Fix

Reject telephone-event packets whose data size is not exactly sizeof(dtmf_payload_t):

if (p->getDataSize() != sizeof(dtmf_payload_t))
  return;

Validation

  • Confirmed the unchecked cast is present in current master and that the only size filter before it (receive()) merely rejects a zero-length payload, not short ones.
  • The guard only adds an early return for malformed packets; well-formed 4-byte telephone-event payloads are unaffected.

Acknowledgement

Backport of the fix from the yeti-switch/sems fork, commit a15aa819152f10f24fcfb35f975ef025ba5fa3f7 ("AmRtpStream::recvDtmfPacket: ignore telephone-event packets with unexpected size"). Thanks to the yeti-switch maintainers.


Generated by Claude Code

…pected size

recvDtmfPacket casts the RTP payload to dtmf_payload_t* and reads
event/e/r/volume/duration without checking that the packet actually
carries a full 4-byte telephone-event payload. A truncated (1-3 byte)
telephone-event packet passes the non-empty-payload filter in receive()
and makes the cast read past the received data, producing bogus DTMF
events from stale buffer bytes.

Reject telephone-event packets whose data size is not exactly
sizeof(dtmf_payload_t). Backport of yeti-switch/sems a15aa819.
Copilot AI review requested due to automatic review settings July 10, 2026 03:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens DTMF (RFC 2833/telephone-event) handling in AmRtpStream::recvDtmfPacket by validating the RTP payload length before casting it to dtmf_payload_t, preventing reads past the received payload and avoiding posting malformed DTMF events.

Changes:

  • Add a guard to reject telephone-event RTP packets whose payload size is not exactly sizeof(dtmf_payload_t).
  • Document why the size check is required (prevents bogus event/volume/duration).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread core/AmRtpStream.cpp
Comment on lines +858 to +860
// a telephone-event payload is exactly sizeof(dtmf_payload_t) bytes; a
// truncated packet would make the cast below read past the received data
// and yield bogus event/volume/duration values.
Add a test_rtpstream suite for the previous commit. A test subclass of
AmRtpStream sets the negotiated telephone-event payload type directly,
so no SDP or socket is involved, and feeds packets built with
AmRtpPacket::compile_raw() and parse() to recvDtmfPacket(). The stream
posts to an AmSession whose onDtmf() records the keys its DTMF detector
reports; a key is reported once the next one arrives.

- Full 4 byte telephone-event payloads for keys 4 and 5 report key 4.
- A packet for key 4 whose payload is 0 to 3 bytes long is ignored:
  full events for keys 5 and 6 that follow it report key 5 only. Before
  that packet, the same AmRtpPacket carries a full key 4 event, so the
  bytes the short payload lacks still spell it. Without the previous
  commit those stale bytes were decoded, and keys 4 and 5 were reported.

Payloads longer than 4 bytes are not covered. RFC 4733 section 2.5.1.5
lets a sender pack contiguous events into one packet, so whether those
should be dropped or have their first event used is left open here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hecko
hecko merged commit fbfac89 into master Sep 13, 2026
24 of 26 checks passed
@hecko
hecko deleted the hecko/amrtpstream-recvdtmf-size-guard branch September 13, 2026 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants