Your environment.

• Version: v1.5.2
• Browser: Chrome Version 95.0.4638.54 (Official Build) (x86_64)
• Other Information

my code: https://github.com/madawei2699/free4chat/blob/3867ee89cb44ecd3efb581598621decadbf151a1/backend/src/user.go#L102-L131

What did you do?

when the user disconnect unexpectedly, the webrtc peer connections close, then the memory panic has occurred.

What did you expect?

can normally close the peer connections

What happened?

My app crashed!

error stack:

2021/11/08 10:05:36 websocket: close 1006 (abnormal closure): unexpected EOF
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1339a42]

goroutine 64 [running]:
github.com/pion/srtp.(*ReadStreamSRTCP).Close(0x0, 0x0, 0x0)
	~/go/pkg/mod/github.com/pion/[email protected]/stream_srtcp.go:66 +0x42
github.com/pion/webrtc/v2.(*RTPReceiver).Stop(0xc0001394a0, 0x0, 0x0)
	~/go/pkg/mod/github.com/pion/webrtc/[email protected]/rtpreceiver.go:139 +0x105
github.com/pion/webrtc/v2.(*RTPTransceiver).Stop(0xc0002084c0, 0x4d, 0x0)
	~/go/pkg/mod/github.com/pion/webrtc/[email protected]/rtptransceiver.go:55 +0x17b
github.com/pion/webrtc/v2.(*PeerConnection).Close(0xc00008bb00, 0x39, 0x0)
	~/go/pkg/mod/github.com/pion/webrtc/[email protected]/peerconnection.go:1355 +0xdb
main.(*User).readPump.func1(0xc000226460)
	/tmp/free4chat/backend/src/user.go:105 +0x33
main.(*User).readPump(0xc000226460)
	/tmp/free4chat/backend/src/user.go:131 +0x3aa
created by main.serveWs
	/tmp/free4chat/backend/src/user.go:639 +0x98a

On line 14, out is truncated to length tailOffset:
https://github.com/pions/srtp/blob/bc58b55b1a0d101267701ae3ea00412cb77bc918/srtcp.go#L14

However, on line 21, access of out[tailOffset:] is attempted, which results in an out-of-bounds panic:
https://github.com/pions/srtp/blob/bc58b55b1a0d101267701ae3ea00412cb77bc918/srtcp.go#L21

Believe line 21 should be:

srtcpIndexBuffer := encrypted[tailOffset : tailOffset+srtcpIndexSize]

pion/rtp
// Header represents an RTP packet header
type Header struct {
Version uint8
Padding bool
Extension bool
Marker bool
PayloadType uint8
SequenceNumber uint16
Timestamp uint32
SSRC uint32
CSRC []uint32
ExtensionProfile uint16
Extensions []Extension
}
github.com/pion/srtp
../../../../srtp/session_srtp.go:144:9: assignment mismatch: 1 variable but h.Unmarshal returns 2 values
../../../../srtp/srtp.go:37:9: assignment mismatch: 1 variable but header.Unmarshal returns 2 values
../../../../srtp/srtp.go:52:9: assignment mismatch: 1 variable but header.Unmarshal returns 2 values
../../../../srtp/srtp.go:56:51: header.PayloadOffset undefined (type *rtp.Header has no field or method PayloadOffset)
../../../../srtp/srtp_cipher_aead_aes_gcm.go:99:13: header.PayloadOffset undefined (type *rtp.Header has no field or method PayloadOffset)
../../../../srtp/srtp_cipher_aead_aes_gcm.go:104:18: header.PayloadOffset undefined (type *rtp.Header has no field or method PayloadOffset)
../../../../srtp/srtp_cipher_aes_cm_hmac_sha1.go:121:30: header.PayloadOffset undefined (type *rtp.Header has no field or method PayloadOffset)
../../../../srtp/srtp_cipher_aes_cm_hmac_sha1.go:126:32: header.PayloadOffset undefined (type *rtp.Header has no field or method PayloadOffset)

There is an error with this repository's Renovate configuration that needs to be fixed. As a precaution, Renovate will stop PRs until it is resolved.

Error type: Cannot find preset's package (github>whitesource/merge-confidence:beta)

where is srtp demo

current:

EncryptRTP(dst []byte, plaintext []byte, header *rtp.Header) ([]byte, error)

proposed:

EncryptRTP(dst []byte, packet *rtp.Packet) ([]byte, error)

pros:

1. Simpler API when callers use *rtp.Packet.
2. Avoids an extra Unmarshal for callers when use rtp.Packet. For example: WriteRTP.
3. Avoids a memcpy of the payload prior to encryption.
4. Removes the implicit assumption that cap(dst) == cap(plaintext) will avoid an allocation.

cons:

1. Worse API when callers use byte slices. Requires Unmarshal, but net performance is still the same/better.
2. Removes the option to fill the packet headers after sending. I can't think of a case when this would be practical.

This issue is a reminder for me to revisit these objects (but If anyone wants to write some tests before i get to it, feel free!)

Summary

Please make ProtectionProfile methods public

Motivation

I am implementing SRTP support for my RTSP Server. I need to send key management parameters using MIKEY protocol to RTSP client. They include lengths of key, salt, etc.

Describe alternatives you've considered

I added constants for these values to my code. Calling ProtectionProfile methods to get these values would result in cleaner code.

Your environment.

• Version: master
• Browser: include version
• Other Information - stacktraces, related issues, suggestions how to fix, links for us to have context

What did you do?

What did you expect?

What happened?

func (c *Context) decryptRTCP(dst, encrypted []byte) ([]byte, error) {
	out := allocateIfMismatch(dst, encrypted)

	authTagLen, err := c.cipher.rtcpAuthTagLen()
	if err != nil {
		return nil, err
	}
	aeadAuthTagLen, err := c.cipher.aeadAuthTagLen()
	if err != nil {
		return nil, err
	}
	tailOffset := len(encrypted) - (authTagLen + srtcpIndexSize)

	if tailOffset < aeadAuthTagLen {
		return nil, fmt.Errorf("%w: %d", errTooShortRTCP, len(encrypted))
	} else if isEncrypted := encrypted[tailOffset] >> 7; isEncrypted == 0 {
		return out, nil
	}

	index := c.cipher.getRTCPIndex(encrypted)
	ssrc := binary.BigEndian.Uint32(encrypted[4:])

	s := c.getSRTCPSSRCState(ssrc)
	markAsValid, ok := s.replayDetector.Check(uint64(index))
	if !ok {
		return nil, &duplicatedError{Proto: "srtcp", SSRC: ssrc, Index: index}
	}

	out, err = c.cipher.decryptRTCP(out, encrypted, index, ssrc)
	if err != nil {
		return nil, err
	}

	markAsValid()
	return out, nil
}

srtcp data length min length should be 8 + aeadAuthTagLen + authTagLen + srtcpIndexSize
8 is rtcp header(4 bytes + 4 bytes ssrc)

assume a srtcp packet is 4 + aeadAuthTagLen + authTagLen + srtcpIndexSize, then

ssrc := binary.BigEndian.Uint32(encrypted[4:])

will get a wrong value. so need check if len(encrypted) >= min length in decryptRTCP function

Your environment.

• Version: v1.1.2/b2f6a7fc
• Other Information:

I added some extra logging to help figure out why this message was appearing:

--- a/vendor/github.com/pions/transport/packetio/buffer.go
+++ b/vendor/github.com/pions/transport/packetio/buffer.go
@@ -1,9 +1,10 @@
 package packetio

 import (
-       "errors"
        "io"
        "sync"
+
+       "github.com/pkg/errors"
 )

 // ErrFull is returned when the buffer has hit the configured limits.
@@ -57,7 +58,7 @@ func (b *Buffer) Write(packet []byte) (n int, err error) {
        // Check if there is available capacity
        if b.limitSize != 0 && b.size+len(packet) > b.limitSize {
                b.mutex.Unlock()
-               return 0, ErrFull
+               return 0, errors.Wrap(ErrFull, "Buffer capacity reached in Write()")
        }

--- a/vendor/github.com/pions/srtp/session.go
+++ b/vendor/github.com/pions/srtp/session.go
@@ -5,6 +5,8 @@ import (
        "io"
        "net"
        "sync"
+
+       "github.com/pkg/errors"
 )

 type streamSession interface {
@@ -92,6 +94,10 @@ func (s *session) close() error {
        return nil
 }

+type stackTracer interface {
+       StackTrace() errors.StackTrace
+}
+
 func (s *session) start(localMasterKey, localMasterSalt, remoteMasterKey, remoteMasterSalt []byte, profile ProtectionProfile, child streamSession) error {
        var err error
        s.localContext, err = CreateContext(localMasterKey, localMasterSalt, profile)
@@ -127,6 +133,11 @@ func (s *session) start(localMasterKey, localMasterSalt, remoteMasterKey, remote

                        if err = child.decrypt(b[:i]); err != nil {
                                fmt.Println(err)
+                               if err, ok := err.(stackTracer); ok {
+                                       for _, f := range err.StackTrace() {
+                                               fmt.Printf("%+s:%d\n", f, f)
+                                       }
+                               }
                        }
                }
        }()

This gave me the following stack trace on the console (one of zillions, once they started appearing):

Buffer capacity reached in Write(): full buffer
savant-go/vendor/github.com/pions/transport/packetio.(*Buffer).Write
	/Users/mike.macdonald/go/src/savant-go/vendor/github.com/pions/transport/packetio/buffer.go:61
savant-go/vendor/github.com/pions/srtp.(*ReadStreamSRTCP).write
	/Users/mike.macdonald/go/src/savant-go/vendor/github.com/pions/srtp/stream_srtcp.go:28
savant-go/vendor/github.com/pions/srtp.(*SessionSRTCP).decrypt
	/Users/mike.macdonald/go/src/savant-go/vendor/github.com/pions/srtp/session_srtcp.go:129
savant-go/vendor/github.com/pions/srtp.(*session).start.func1
	/Users/mike.macdonald/go/src/savant-go/vendor/github.com/pions/srtp/session.go:134
runtime.goexit
	/usr/local/Cellar/go/1.11.5/libexec/src/runtime/asm_amd64.s:1333

What did you do?

Testing multiple video streams. Have not noticed this issue before, but I added a new high-bandwidth stream source (2-4Mbps) and this seemed to be enough to put it over the edge.

Once I started seeing the errors in the console log, I also observed that the video streams became choppy, dropping from 30fps down to single-digits and in some cases freezing altogether until restarted.

What did you expect?

That not to happen.

What happened?

It happened.

Encrypt version of #69

Currently readStreams can't be removed at runtime. Add .Close() to ReadStreams so we can tear them down at anytime

OpenWriteStream should not return global stream since roll over counter of each encryption context should be managed for each SSRC.

-func (s *SessionSRTP) OpenWriteStream() (*WriteStreamSRTP, error) {
+func (s *SessionSRTP) OpenWriteStream(SSRC uint32) (*WriteStreamSRTP, error) {

I am working on a benchmark to show RTSP -> WebRTC fanout performance. I want to land this first before getting numbers :)

This is the difference when using libsrtp, so we should see a pretty big improvement for Pion as well! This is due to be able to use hardware acceleration (Auth+Encrypt) can all be done in one step now.

-- aes_cm_128_hmac_sha1_32
./a.out  1.83s user 0.01s system 99% cpu 1.843 total
./a.out  1.84s user 0.01s system 99% cpu 1.856 total
./a.out  1.83s user 0.00s system 99% cpu 1.837 total
./a.out  1.82s user 0.01s system 99% cpu 1.831 total
./a.out  2.04s user 0.01s system 99% cpu 2.065 total
./a.out  1.82s user 0.01s system 99% cpu 1.834 total
./a.out  1.84s user 0.01s system 99% cpu 1.857 total
./a.out  1.83s user 0.00s system 99% cpu 1.843 total
./a.out  1.85s user 0.01s system 99% cpu 1.860 total
./a.out  1.83s user 0.01s system 99% cpu 1.839 total

-- aes_gcm_128_16
./a.out  0.76s user 0.00s system 99% cpu 0.772 total
./a.out  0.81s user 0.00s system 99% cpu 0.814 total
./a.out  0.75s user 0.00s system 99% cpu 0.762 total
./a.out  0.75s user 0.01s system 99% cpu 0.760 total
./a.out  0.76s user 0.00s system 99% cpu 0.765 total
./a.out  0.76s user 0.00s system 99% cpu 0.765 total
./a.out  0.79s user 0.01s system 99% cpu 0.803 total
./a.out  0.77s user 0.01s system 99% cpu 0.780 total
./a.out  0.75s user 0.00s system 99% cpu 0.762 total
./a.out  0.79s user 0.01s system 99% cpu 0.796 total

This issue provides visibility into Renovate updates and their statuses. Learn more

This repository currently has no open or pending branches.

• Check this box to trigger a request for Renovate to run again on this repository

srtp.Context is shared among multiple SSRCs, but roll over counter and replay detector must not be shared.
Shared ROC causes decryption failure when the difference of actual ROCs is more than 1.
(Packet rate of each stream can be different.)
And, replay detector causes false positive detection.

ref: pion/webrtc#1095 (comment)

The code does not currently verify that the authentication tag matches. This allows a middle-man to send RTCP packets on behalf of the peer. An ISP could send a BYE packet, for example.

Summary

• Expose SetReadDeadline from ReadStreamSRTP and ReadStreamSRTCP.
• Expose SetWriteDeadline from WriteStreamSRTP and WriteStreamSRTCP

Motivation

To make the packet I/O of pion/webrtc cancelable without closing peer-connection, including RTP and RTCP packets.

Additional context

I've spent some time to trace this package, and below is an implementation plan. Please kindly correct me if it's wrong.

• expose SetReadDeadline for ReadStreamSRTP via ReadStreamSRTP.buffer.
• expose SetReadDeadline for ReadStreamSRTCP via ReadStreamSRTCP.buffer.
• expose SetWriteDeadline for WriteStreamSRTP via session.nextConn.
• expose SetWriteDeadline for WriteStreamSRTCP via session.nextConn.

What I didn't investigate is the way to test the implementation, feel free to give me some advise/reference if any.

Your environment.

• Version: aa6cbe8 or 1.3.1
• Browser: Firefox

Other Information

pcap-logs.zip contains:

1. firefox-rtcp-from-log.pcap: obtained using this method
2. firefox-rtcp-received-tcpdump-from-srtp.pcap: actual received packets, failed example, correlates to the first file
3. chrome-rtcp-received-tcpdump-from-srtp.pcap: working example, but taken with the replay detection commented out. But Chrome does not fail even with replay detector turned on.

The last two files contain duplicate (sent and received) packets, they can be filtered out using the frame.p2p_dir == sent filter. These logs were obtained by modifying session_srtcp.go:

$ git diff
diff --git a/session_srtcp.go b/session_srtcp.go
index 3df39c5..2ecfe87 100644
--- a/session_srtcp.go
+++ b/session_srtcp.go
@@ -11,6 +11,17 @@ import (

 const defaultSessionSRTCPReplayProtectionWindow = 64

+var udpConn *net.UDPConn
+
+func init() {
+       fmt.Println("-- srtp udp --")
+       var err error
+       udpConn, err = net.ListenUDP("udp", &net.UDPAddr{IP: net.IP{127, 0, 0, 1}, Port: 5678})
+       if err != nil {
+               panic(err)
+       }
+}
+
 // SessionSRTCP implements io.ReadWriteCloser and provides a bi-directional SRTCP session
 // SRTCP itself does not have a design like this, but it is common in most applications
 // for local/remote to each have their own keying material. This provides those patterns
@@ -145,9 +156,15 @@ func destinationSSRC(pkts []rtcp.Packet) []uint32 {
 func (s *SessionSRTCP) decrypt(buf []byte) error {
        decrypted, err := s.remoteContext.DecryptRTCP(buf, buf, nil)
        if err != nil {
+               fmt.Println("Error decrypting RTCP:", err)
                return err
        }

+       _, err = udpConn.WriteTo(decrypted, &net.UDPAddr{IP: net.IP{127, 0, 0, 1}, Port: 5000})
+       if err != nil {
+               fmt.Println("Error writing to UDP 127.0.0.1:5000", err)
+       }
+
        pkt, err := rtcp.Unmarshal(decrypted)
        if err != nil {
                return err

What did you do?

Error is reproducible using https://github.com/peer-calls/firefox-pli-bug

What did you expect?

I expected to receive a PLI packet for 2nd track in Firefox.

What happened?

The PLI packets were filtered out because of this line:

Possibly related to #60.

Currently we never clear these maps, if we add/remove tracks they SSRCes could just become leaks.

When we implement re-offers/re-answers make sure these get cleaned.

Currently these tests fail and we just logf the result.

here and here

It's valid to pass multiple RTCP packets in the same UDP packet. The UDP packet is decrypted correctly, but both rtcp.Unmarshal and ReadRTCP can only return a single packet. rtcp.Unmarshal will incorrectly assume the extra concatenated packets to be an extension, improperly ignoring the length header.

Your environment.

• Version: v2.0.1
• Browser: chrome 87.0.4280.141
• Other Information - stacktraces, related issues, suggestions how to fix, links for us to have context

What did you do?

I use pion/webrtc to establish a connection with webrtc on the browser, and then when the browser returns rtcp information (such as nack), pion/webrtc will return an unpacking error message. After following up, it was found that the error came from the decryption verification of srtcp (srtcp.go:18). The established srtcp uses gcm encryption. I found that the SRTP_AES128_CM_HMAC_SHA1_80 encryption method has a generateSrtcpAuthTag call (srtp_cipher_aes_cm_hmac_sha1.go:138), but SRTP_AEAD_AES_128_GCM does not. Is this the cause of the problem? After I switched the encryption method of srtcp to SRTP_AES128_CM_HMAC_SHA1_80, this problem disappeared.
--From google translation

Your environment.

• Version: v2.0.12
• Browser: include version
• Other Information - stacktraces, related issues, suggestions how to fix, links for us to have context

What did you do?

What did you expect?

What happened?

// AcceptStream returns a stream to handle RTCP for a single SSRC
func (s *SessionSRTP) AcceptStream() (*ReadStreamSRTP, uint32, error) {
	stream, ok := <-s.newStream
	if !ok {
		return nil, 0, errStreamAlreadyClosed
	}

	readStream, ok := stream.(*ReadStreamSRTP)
	if !ok {
		return nil, 0, errFailedTypeAssertion
	}

	return readStream, stream.GetSSRC(), nil
}

func (s *session) start(localMasterKey, localMasterSalt, remoteMasterKey, remoteMasterSalt []byte, profile ProtectionProfile, child streamSession) error {
	var err error
	s.localContext, err = CreateContext(localMasterKey, localMasterSalt, profile, s.localOptions...)
	if err != nil {
		return err
	}

	s.remoteContext, err = CreateContext(remoteMasterKey, remoteMasterSalt, profile, s.remoteOptions...)
	if err != nil {
		return err
	}

	go func() {
		defer func() {
			close(s.newStream)

			s.readStreamsLock.Lock()
			s.readStreamsClosed = true
			s.readStreamsLock.Unlock()
			close(s.closed)
		}()

		b := make([]byte, 8192)
		for {
			var i int
			i, err = s.nextConn.Read(b)
			if err != nil {
				if !errors.Is(err, io.EOF) {
					s.log.Error(err.Error())
				}
				return
			}

			if err = child.decrypt(b[:i]); err != nil {
				s.log.Info(err.Error())
			}
		}
	}()

	close(s.started)

	return nil
}

When calling AcceptStream(), it waits for a new Stream to come in via stream, ok := <-s.newStream. However, if there is an error on the sender side or a network problem preventing packets from being properly transmitted, s.nextConn.Read(b) will not be able to proceed in session.start(). This will cause AcceptStream() to wait indefinitely.

To prevent situations where AcceptStream() cannot proceed, SetReadDeadline() should be set on nextConn.