diff --git a/context.go b/context.go index ca6dd87..903d3a9 100644 --- a/context.go +++ b/context.go @@ -308,67 +308,79 @@ func (s *srtpSSRCState) updateRolloverCount(sequenceNumber uint16, difference in } } -func (c *Context) getSRTPSSRCState(ssrc uint32) *srtpSSRCState { - s, ok := c.srtpSSRCStates[ssrc] +func (c *Context) getSRTPSSRCState(ssrc uint32, keepNew bool) (*srtpSSRCState, bool) { + state, ok := c.srtpSSRCStates[ssrc] if ok { - return s + return state, true } - s = &srtpSSRCState{ + state = &srtpSSRCState{ ssrc: ssrc, replayDetector: c.newSRTPReplayDetector(), } - c.srtpSSRCStates[ssrc] = s + if keepNew { + c.srtpSSRCStates[ssrc] = state + } - return s + return state, false } -func (c *Context) getSRTCPSSRCState(ssrc uint32) *srtcpSSRCState { - s, ok := c.srtcpSSRCStates[ssrc] +func (c *Context) getSRTCPSSRCState(ssrc uint32, keepNew bool) (*srtcpSSRCState, bool) { + state, ok := c.srtcpSSRCStates[ssrc] if ok { - return s + return state, true } - s = &srtcpSSRCState{ + state = &srtcpSSRCState{ ssrc: ssrc, replayDetector: c.newSRTCPReplayDetector(), } - c.srtcpSSRCStates[ssrc] = s + if keepNew { + c.srtcpSSRCStates[ssrc] = state + } + + return state, false +} + +func (c *Context) setSRTPSSRCState(state *srtpSSRCState) { + c.srtpSSRCStates[state.ssrc] = state +} - return s +func (c *Context) setSRTCPSSRCState(state *srtcpSSRCState) { + c.srtcpSSRCStates[state.ssrc] = state } // ROC returns SRTP rollover counter value of specified SSRC. func (c *Context) ROC(ssrc uint32) (uint32, bool) { - s, ok := c.srtpSSRCStates[ssrc] + state, ok := c.srtpSSRCStates[ssrc] if !ok { return 0, false } - return uint32(s.index >> 16), true //nolint:gosec // G115 + return uint32(state.index >> 16), true //nolint:gosec // G115 } // SetROC sets SRTP rollover counter value of specified SSRC. func (c *Context) SetROC(ssrc uint32, roc uint32) { - s := c.getSRTPSSRCState(ssrc) - s.index = uint64(roc) << 16 - s.rolloverHasProcessed = false + state, _ := c.getSRTPSSRCState(ssrc, true) + state.index = uint64(roc) << 16 + state.rolloverHasProcessed = false } // Index returns SRTCP index value of specified SSRC. func (c *Context) Index(ssrc uint32) (uint32, bool) { - s, ok := c.srtcpSSRCStates[ssrc] + state, ok := c.srtcpSSRCStates[ssrc] if !ok { return 0, false } - return s.srtcpIndex, true + return state.srtcpIndex, true } // SetIndex sets SRTCP index value of specified SSRC. func (c *Context) SetIndex(ssrc uint32, index uint32) { - s := c.getSRTCPSSRCState(ssrc) - s.srtcpIndex = index % (maxSRTCPIndex + 1) + state, _ := c.getSRTCPSSRCState(ssrc, true) + state.srtcpIndex = index % (maxSRTCPIndex + 1) } //nolint:cyclop diff --git a/session_srtcp_test.go b/session_srtcp_test.go index 820e220..178fd6e 100644 --- a/session_srtcp_test.go +++ b/session_srtcp_test.go @@ -306,6 +306,56 @@ func getSenderSSRC(t *testing.T, stream *ReadStreamSRTCP) (ssrc uint32, err erro return pli.SenderSSRC, nil } +func TestSessionSRTCPFailedAuthDoesNotGrowStreams(t *testing.T) { + lim := test.TimeOut(time.Second * 10) + defer lim.Stop() + + report := test.CheckRoutines(t) + defer report() + + const ( + badMediaSSRC = uint32(0xDEADBEEF) + goodMediaSSRC = uint32(0x12345678) + ) + + aSession, bSession := buildSessionSRTCPPair(t) + + // Encrypt a PLI for badMediaSSRC with the correct key, then corrupt its auth tag + // so that bSession's remote context rejects it without creating a stream entry. + badEncrypted, err := encryptSRTCP(aSession.session.localContext, + &rtcp.PictureLossIndication{MediaSSRC: badMediaSSRC}) + assert.NoError(t, err) + badEncrypted[len(badEncrypted)-1] ^= 0xFF // corrupt last byte of auth tag + + _, err = aSession.session.nextConn.Write(badEncrypted) + assert.NoError(t, err) + + // Write a valid PLI for goodMediaSSRC so we can synchronize via AcceptStream. + // The read goroutine processes packets sequentially, so when AcceptStream + // returns the bad packet has already been handled. + goodEncrypted, err := encryptSRTCP(aSession.session.localContext, + &rtcp.PictureLossIndication{MediaSSRC: goodMediaSSRC}) + assert.NoError(t, err) + + _, err = aSession.session.nextConn.Write(goodEncrypted) + assert.NoError(t, err) + + _, receivedSSRC, err := bSession.AcceptStream() + assert.NoError(t, err) + assert.Equal(t, goodMediaSSRC, receivedSSRC, "AcceptStream must return the valid SSRC") + + bSession.session.readStreamsLock.Lock() + streamCount := len(bSession.session.readStreams) + _, hasBadMediaSSRC := bSession.session.readStreams[badMediaSSRC] + bSession.session.readStreamsLock.Unlock() + + assert.Equal(t, 1, streamCount, "readStreams must not grow after failed authentication") + assert.False(t, hasBadMediaSSRC, "readStreams must not contain the SSRC from the rejected packet") + + assert.NoError(t, aSession.Close()) + assert.NoError(t, bSession.Close()) +} + func encryptSRTCP(context *Context, pkt rtcp.Packet) ([]byte, error) { decryptedRaw, err := pkt.Marshal() if err != nil { diff --git a/session_srtp.go b/session_srtp.go index 203852d..7b35b69 100644 --- a/session_srtp.go +++ b/session_srtp.go @@ -189,6 +189,16 @@ func (s *SessionSRTP) decrypt(buf []byte) error { return err } + // Decrypt and authenticate the packet before using the SSRC from the header. + // The SSRC field is part of the unauthenticated RTP header, so it must not be + // used to allocate per-SSRC state (stream, replay detector, etc.) until after + // the auth tag has been verified. Doing so before authentication would allow an + // unauthenticated peer to exhaust memory by spoofing arbitrary SSRCs. + decrypted, err := s.remoteContext.decryptRTP(buf, buf, header, headerLen) + if err != nil { + return err + } + r, isNew := s.session.getOrCreateReadStream(header.SSRC, s, newReadStreamSRTP) if r == nil { return nil // Session has been closed @@ -204,11 +214,6 @@ func (s *SessionSRTP) decrypt(buf []byte) error { return errFailedTypeAssertion } - decrypted, err := s.remoteContext.decryptRTP(buf, buf, header, headerLen) - if err != nil { - return err - } - _, err = readStream.write(decrypted) if err != nil { return err diff --git a/session_srtp_test.go b/session_srtp_test.go index dbca950..cdeb29b 100644 --- a/session_srtp_test.go +++ b/session_srtp_test.go @@ -349,6 +349,62 @@ func assertPayloadSRTP( return hdr.SequenceNumber, nil } +func TestSessionSRTPFailedAuthDoesNotGrowStreams(t *testing.T) { + lim := test.TimeOut(time.Second * 5) + defer lim.Stop() + + report := test.CheckRoutines(t) + defer report() + + const ( + badSSRC = uint32(0xDEADBEEF) + goodSSRC = uint32(0x12345678) + ) + + aSession, bSession := buildSessionSRTPPair(t) + + // Encrypt a packet for badSSRC with the correct key, then corrupt its auth tag + // so that bSession's remote context rejects it without creating a stream entry. + badPkt := &rtp.Packet{ + Payload: []byte{0x00, 0x01, 0x02, 0x03}, + Header: rtp.Header{SSRC: badSSRC, SequenceNumber: 1}, + } + badEncrypted, err := encryptSRTP(aSession.session.localContext, badPkt) + assert.NoError(t, err) + badEncrypted[len(badEncrypted)-1] ^= 0xFF // corrupt last byte of auth tag + + _, err = aSession.session.nextConn.Write(badEncrypted) + assert.NoError(t, err) + + // Write a valid packet for goodSSRC so we can synchronize via AcceptStream. + // The read goroutine processes packets sequentially, so when AcceptStream + // returns the bad packet has already been handled. + goodPkt := &rtp.Packet{ + Payload: []byte{0x00, 0x01, 0x02, 0x03}, + Header: rtp.Header{SSRC: goodSSRC, SequenceNumber: 1}, + } + goodEncrypted, err := encryptSRTP(aSession.session.localContext, goodPkt) + assert.NoError(t, err) + + _, err = aSession.session.nextConn.Write(goodEncrypted) + assert.NoError(t, err) + + _, receivedSSRC, err := bSession.AcceptStream() + assert.NoError(t, err) + assert.Equal(t, goodSSRC, receivedSSRC, "AcceptStream must return the valid SSRC") + + bSession.session.readStreamsLock.Lock() + streamCount := len(bSession.session.readStreams) + _, hasBadSSRC := bSession.session.readStreams[badSSRC] + bSession.session.readStreamsLock.Unlock() + + assert.Equal(t, 1, streamCount, "readStreams must not grow after failed authentication") + assert.False(t, hasBadSSRC, "readStreams must not contain the SSRC from the rejected packet") + + assert.NoError(t, aSession.Close()) + assert.NoError(t, bSession.Close()) +} + func encryptSRTP(context *Context, pkt *rtp.Packet) ([]byte, error) { decryptedRaw, err := pkt.Marshal() if err != nil { diff --git a/srtcp.go b/srtcp.go index 5874f0d..496fb19 100644 --- a/srtcp.go +++ b/srtcp.go @@ -47,8 +47,18 @@ func (c *Context) decryptRTCP(dst, encrypted []byte) ([]byte, error) { index := c.cipher.getRTCPIndex(encrypted) ssrc := binary.BigEndian.Uint32(encrypted[4:]) - s := c.getSRTCPSSRCState(ssrc) - markAsValid, ok := s.replayDetector.Check(uint64(index)) + // The SSRC is read from the unauthenticated RTCP header at this point. + // getSRTCPSSRCState is called in read-only mode so that no new map entry is + // inserted until after the auth tag has been verified. The state is committed + // to the map by setSRTCPSSRCState only after markAsValid() succeeds below. + ssrcState, existingState := c.getSRTCPSSRCState(ssrc, false) + + // The replay check is intentionally performed before authentication. + // Rejecting already-seen sequence numbers here avoids the CPU cost of + // AES decryption and HMAC/GCM verification on flooded duplicate packets. + // Safety relies on the replay detector only committing the index as "seen" + // when markAsValid() is explicitly called after successful authentication. + markAsValid, ok := ssrcState.replayDetector.Check(uint64(index)) if !ok { return nil, &duplicatedError{Proto: "srtcp", SSRC: ssrc, Index: index} } @@ -70,6 +80,10 @@ func (c *Context) decryptRTCP(dst, encrypted []byte) ([]byte, error) { markAsValid() + if !existingState { + c.setSRTCPSSRCState(ssrcState) + } + return out, nil } @@ -92,7 +106,7 @@ func (c *Context) encryptRTCP(dst, decrypted []byte) ([]byte, error) { } ssrc := binary.BigEndian.Uint32(decrypted[4:]) - ssrcState := c.getSRTCPSSRCState(ssrc) + ssrcState, _ := c.getSRTCPSSRCState(ssrc, true) if ssrcState.srtcpIndex >= maxSRTCPIndex { // ... when 2^48 SRTP packets or 2^31 SRTCP packets have been secured with the same key diff --git a/srtcp_test.go b/srtcp_test.go index 1fa92e2..cae6626 100644 --- a/srtcp_test.go +++ b/srtcp_test.go @@ -703,3 +703,37 @@ func TestRTCPSwitchMKI(t *testing.T) { //nolint:cyclop }) } } + +func TestSRTCPFailedAuthDoesNotGrowSSRCMap(t *testing.T) { + testSRTCPFailedAuthDoesNotGrowSSRCMap := func(t *testing.T, profile ProtectionProfile) { + t.Helper() + + encryptCtx, err := buildTestContext(profile) + assert.NoError(t, err) + + // Use all-zero key and salt so the authentication tag will never match. + keyLen, err := profile.KeyLen() + assert.NoError(t, err) + + saltLen, err := profile.SaltLen() + assert.NoError(t, err) + + decryptCtx, err := CreateContext(make([]byte, keyLen), make([]byte, saltLen), profile) + assert.NoError(t, err) + + pktRaw, err := rtcp.Marshal([]rtcp.Packet{&rtcp.PictureLossIndication{MediaSSRC: 0xDEADBEEF}}) + assert.NoError(t, err) + + encrypted, err := encryptCtx.EncryptRTCP(nil, pktRaw, nil) + assert.NoError(t, err) + + _, err = decryptCtx.DecryptRTCP(nil, encrypted, nil) + assert.Error(t, err, "DecryptRTCP must fail when key does not match") + + assert.Empty(t, decryptCtx.srtcpSSRCStates, + "SRTCP SSRC state map must not grow after failed authentication") + } + + t.Run("CTR", func(t *testing.T) { testSRTCPFailedAuthDoesNotGrowSSRCMap(t, profileCTR) }) + t.Run("GCM", func(t *testing.T) { testSRTCPFailedAuthDoesNotGrowSSRCMap(t, profileGCM) }) +} diff --git a/srtp.go b/srtp.go index 9c611ab..e5591e9 100644 --- a/srtp.go +++ b/srtp.go @@ -20,6 +20,7 @@ Simplified structure of SRTP Packets: - Auth Tag - used by non-AEAD profiles only. When RCC is used with AEAD profiles, the ROC is sent here. */ +// nolint:cyclop func (c *Context) decryptRTP(dst, ciphertext []byte, header *rtp.Header, headerLen int) ([]byte, error) { authTagLen, err := c.cipher.AuthTagRTPLen() if err != nil { @@ -39,7 +40,11 @@ func (c *Context) decryptRTP(dst, ciphertext []byte, header *rtp.Header, headerL return nil, fmt.Errorf("%w: %d", errTooShortRTP, len(ciphertext)) } - ssrcState := c.getSRTPSSRCState(header.SSRC) + // The SSRC in the RTP header is unauthenticated at this point. getSRTPSSRCState is + // called in read-only mode (existingState tracks whether it was pre-existing) so that + // no new map entry is inserted until after the auth tag has been verified. The state + // is committed to the map by setSRTPSSRCState only after markAsValid() succeeds below. + ssrcState, existingState := c.getSRTPSSRCState(header.SSRC, false) var roc uint32 var diff int64 @@ -55,6 +60,11 @@ func (c *Context) decryptRTP(dst, ciphertext []byte, header *rtp.Header, headerL diff = int64(ssrcState.index) - int64(index) //nolint:gosec } + // The replay check is intentionally performed before authentication. + // Rejecting already-seen sequence numbers here avoids the CPU cost of + // AES decryption and HMAC/GCM verification on flooded duplicate packets. + // Safety relies on the replay detector only committing the index as "seen" + // when markAsValid() is explicitly called after successful authentication. markAsValid, ok := ssrcState.replayDetector.Check(index) if !ok { return nil, &duplicatedError{ @@ -87,6 +97,10 @@ func (c *Context) decryptRTP(dst, ciphertext []byte, header *rtp.Header, headerL markAsValid() ssrcState.updateRolloverCount(header.SequenceNumber, diff, hasRocInPacket, roc) + if !existingState { + c.setSRTPSSRCState(ssrcState) + } + return dst, nil } @@ -134,8 +148,8 @@ func (c *Context) encryptRTP(dst []byte, header *rtp.Header, headerLen int, plai return nil, errUnsupportedHeaderExtension } - s := c.getSRTPSSRCState(header.SSRC) - roc, diff, ovf := s.nextRolloverCount(header.SequenceNumber) + ssrcState, _ := c.getSRTPSSRCState(header.SSRC, true) + roc, diff, ovf := ssrcState.nextRolloverCount(header.SequenceNumber) if ovf { // ... when 2^48 SRTP packets or 2^31 SRTCP packets have been secured with the same key // (whichever occurs before), the key management MUST be called to provide new master key(s) @@ -143,7 +157,7 @@ func (c *Context) encryptRTP(dst []byte, header *rtp.Header, headerLen int, plai // https://www.rfc-editor.org/rfc/rfc3711#section-9.2 return nil, errExceededMaxPackets } - s.updateRolloverCount(header.SequenceNumber, diff, false, 0) + ssrcState.updateRolloverCount(header.SequenceNumber, diff, false, 0) rocInPacket := c.rccMode != RCCModeNone && header.SequenceNumber%c.rocTransmitRate == 0 diff --git a/srtp_test.go b/srtp_test.go index 95c0cb6..9177f92 100644 --- a/srtp_test.go +++ b/srtp_test.go @@ -909,3 +909,42 @@ func TestRTPSwitchMKI(t *testing.T) { //nolint:cyclop assert.NoError(t, err) } } + +func TestSRTPFailedAuthDoesNotGrowSSRCMap(t *testing.T) { + testSRTPFailedAuthDoesNotGrowSSRCMap := func(t *testing.T, profile ProtectionProfile) { + t.Helper() + + encryptCtx, err := buildTestContext(profile) + assert.NoError(t, err) + + // Use all-zero key and salt so the authentication tag will never match. + keyLen, err := profile.KeyLen() + assert.NoError(t, err) + + saltLen, err := profile.SaltLen() + assert.NoError(t, err) + + decryptCtx, err := CreateContext(make([]byte, keyLen), make([]byte, saltLen), profile) + assert.NoError(t, err) + + pkt := &rtp.Packet{ + Payload: rtpTestCaseDecrypted(), + Header: rtp.Header{SequenceNumber: 5000, SSRC: 0xDEADBEEF}, + } + + pktRaw, err := pkt.Marshal() + assert.NoError(t, err) + + encrypted, err := encryptCtx.EncryptRTP(nil, pktRaw, nil) + assert.NoError(t, err) + + _, err = decryptCtx.DecryptRTP(nil, encrypted, nil) + assert.Error(t, err, "DecryptRTP must fail when key does not match") + + assert.Empty(t, decryptCtx.srtpSSRCStates, + "SRTP SSRC state map must not grow after failed authentication") + } + + t.Run("CTR", func(t *testing.T) { testSRTPFailedAuthDoesNotGrowSSRCMap(t, profileCTR) }) + t.Run("GCM", func(t *testing.T) { testSRTPFailedAuthDoesNotGrowSSRCMap(t, profileGCM) }) +}