1535868 - Negotiating DTLS without SRTP extension results in random crashes

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Comment 1

[Nils Ohlmeier [:drno]]

Marked this as sec bug, because crashes appear all over the place, e.g.:

https://crash-stats.mozilla.com/report/index/31b24d3a-df2e-4e9a-961b-13edd0190315
https://crash-stats.mozilla.com/report/index/22e4740e-7c30-4f83-9777-b5ad80190315
https://crash-stats.mozilla.com/report/index/0abfcb1a-e347-44f8-8653-be97b0190315
https://crash-stats.mozilla.com/report/index/d321fd13-e67b-40d2-b7eb-f5ec20190315
https://crash-stats.mozilla.com/report/index/169e2d63-d3ad-4190-b1aa-425730190315

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: Bug 1535868: don't send SRTP when not negotiated. r?bwc

Comment 3

[Nils Ohlmeier [:drno]]

The problem appears to be that when we fail to negotiate the use_srtp extension the transportlayersrtp state gets set to error. But that error doesn't get propagated through the stack, as transportlayerdtls reports success. And transportlayersrtp only refuses to receive when being in an error state but keeps trying to send.

Proper handling in the sense that DTLS should fail if the upper layers need it, but can success if use_srtp is not needed because of a data channel only connection should be done in bug 1520692.

Comment 4

[Byron Campen [:bwc]]

All of the crashes in comment 1 are nullptr crashes as far as I can tell.

Comment 5

[Andrew McCreight [:mccr8]]

I'm marking this as audit because they all look like null derefs, but adjust as needed.

Comment 6

[Nils Ohlmeier [:drno]]

I double checked the code in transportlayersrtp and I'm pretty sure this is the only corner case which wasn't covered yet. So I'm opening this bug now for the public.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/74d361ea2fe6
don't send SRTP when not negotiated. r=bwc

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/74d361ea2fe6

Comment 9

[Pascal Chevrel (relman team) -> :pascalc]

Nils, do you think that this is a fix we should uplift to 67 beta?

Comment 10

[Pascal Chevrel (relman team) -> :pascalc]

(In reply to Pascal Chevrel:pascalc from comment #9)

Nils, do you think that this is a fix we should uplift to 67 beta?

Nils, ping :)

Comment 11

[Nils Ohlmeier [:drno]]

Sorry for the late reply.

As endpoints which try to negotiate something like this are pretty rare I think it's fine to let this ride the trains.