Closed Bug 1303867 Opened 3 years ago Closed 3 years ago

Missing DTLS alert on PeerConnection.close()

Categories

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

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file)

According to a report on dev-media Fx since version 48 no longer sends a DTLS alert message when PeerConnection.close() gets called.

Nils needs to provide the bug which caused this behavior.
^ see description
backlog: --- → webrtc/webaudio+
Rank: 29
Flags: needinfo?(drno)
I'm pretty sure bug 1279146 causes this new behavior. We should check if we can find a better way to have DTLS send a desired DTLS alert on shutdown, but not have it send DTLS packets on already freed sockets.
Depends on: CVE-2016-5258
Flags: needinfo?(drno)
Assignee: nobody → drno
Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

https://reviewboard.mozilla.org/r/132258/#review135156

::: media/mtransport/transportlayerdtls.cpp:373
(Diff revision 1)
>    TransportLayerReserved,
>    TransportLayerReserved
>  };
>  
>  TransportLayerDtls::~TransportLayerDtls() {
> +  ssl_fd_ = nullptr;

So this patch appears to do absolutely nothing.  The destructor of the UniquePRFileDesc should be called either way when the TransportLayerDtls instance is destroyed.  But your concern is ordering, and that's fair.  Note that packet loss will cause the alert to be lost in a few cases anyway.

I think that you need a comment here explaining that the close alert would be otherwise suppressed by disabling the `nspr_io_adapter_` (if that is indeed what is happening).
Attachment #8860254 - Flags: review?(martin.thomson)
Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

https://reviewboard.mozilla.org/r/132258/#review135156

> So this patch appears to do absolutely nothing.  The destructor of the UniquePRFileDesc should be called either way when the TransportLayerDtls instance is destroyed.  But your concern is ordering, and that's fair.  Note that packet loss will cause the alert to be lost in a few cases anyway.
> 
> I think that you need a comment here explaining that the close alert would be otherwise suppressed by disabling the `nspr_io_adapter_` (if that is indeed what is happening).

Yes this is only about order of destroying so the DTLS alert can still get out. I know it can get lost. But people are complaining that Firefox used to send out the alerts, and now it doesn't any more.

I guess the other option would be instead of disabling the nspr_io_adapter completely, only block reads but still allow write operations from the destructor. But as that would add even more code I decided against it.
Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

https://reviewboard.mozilla.org/r/132258/#review135530

::: media/mtransport/transportlayerdtls.cpp:373
(Diff revision 2)
>    TransportLayerReserved,
>    TransportLayerReserved
>  };
>  
>  TransportLayerDtls::~TransportLayerDtls() {
> +  // Destroy the NSS instance first so it can still send out an alert before 

trailing space

::: media/mtransport/transportlayerdtls.cpp:374
(Diff revision 2)
>    TransportLayerReserved
>  };
>  
>  TransportLayerDtls::~TransportLayerDtls() {
> +  // Destroy the NSS instance first so it can still send out an alert before 
> +  // we disable the nspr_io_adapter.

missing trailing _
Attachment #8860254 - Flags: review?(martin.thomson) → review+
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/1979070698f1
destroy SSL FD to send out DTLS allert on close. r=mt
https://hg.mozilla.org/mozilla-central/rev/1979070698f1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.