All users were logged out of Bugzilla on October 13th, 2018

Webrtc in NACK mode resends packets and SRTP rejects re-encoding a packet with the same sequence number

RESOLVED FIXED in mozilla21

Status

()

P1
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

17 Branch
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webrtc][blocking-webrtc+][qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Webrtc in NACK mode resends packets and in our implementation, with SRTP external to webrtc, SRTP rejects re-encoding a packet with the same sequence number to avoid key reuse.

In this case, it's actually safe since we're re-encoding the exact same data.

I'll attach a patch to allow resend of packets - note that you still can't implement after-SRTP-decrypt artificial drops without flipping a config (since SRTP also protects against re-reception of the same packet).

Better solutions would be

a) use the external_encryption_ setting in webrtc (which would make it store and resend already-encrypted packets)
b) Modify the API to allow being told it's a resend and flagging it to SRTP (which involves API changes there)
c) Modify SRTP to store the auth hash and compare to the duplicate before rejecting, under the assumption it's not a two-time use if the hashes match

We do not need to do any of these before release, as we don't resend with different data.
(Assignee)

Comment 1

6 years ago
Created attachment 704270 [details] [diff] [review]
Disable SRTP replay protection on outbound packets
(Assignee)

Comment 2

6 years ago
Created attachment 704271 [details] [diff] [review]
Disable SRTP replay protection on outbound packets

correct patch without debugs
(Assignee)

Updated

6 years ago
Attachment #704270 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=d94adfcca2b2
Priority: -- → P1
(Assignee)

Updated

6 years ago
Attachment #704271 - Flags: review?(ekr)
(Assignee)

Comment 4

6 years ago
Created attachment 704277 [details] [diff] [review]
Match SRTP policy values to enable NACK mode in webrtc

We found how Chrome avoids this problem - the set a policy bit to allow retransmission.  Match their settings, for now.  No more security risk than the original patch; same impact.
(Assignee)

Comment 5

6 years ago
Created attachment 704278 [details] [diff] [review]
Match SRTP policy values to enable NACK mode in webrtc
(Assignee)

Updated

6 years ago
Attachment #704277 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Comment on attachment 704278 [details] [diff] [review]
Match SRTP policy values to enable NACK mode in webrtc

 https://tbpl.mozilla.org/?tree=Try&rev=2403e2900f26

This *should* work no differently (except a longer window for NACK) than the previous patch.

We'll revisit to limit the time period for NACK and window size
Attachment #704278 - Flags: review?(ekr)

Comment 7

6 years ago
Comment on attachment 704278 [details] [diff] [review]
Match SRTP policy values to enable NACK mode in webrtc

Review of attachment 704278 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but please mark this for revisiting by derf or someone else who understands video and RTP.
Attachment #704278 - Flags: review?(ekr) → review+
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72b61011720

Argh.  Forgot to land this when the RTCP fixes landed
Target Milestone: --- → mozilla21
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/0b1325001df4
To make sure it gets into next nightly
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+][webrtc-uplift][qa-]
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift][qa-] → [webrtc][blocking-webrtc+][qa-]
(Assignee)

Updated

5 years ago
Attachment #704271 - Flags: review?(ekr)
You need to log in before you can comment on or make changes to this bug.