783618 - Add support for verifying DTLS fingerprints to mtransport

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Attachment: First patch

This code adds checking of the peer certificate

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Revised patch

Comment 2

[Timothy B. Terriberry (:derf)]

Comment on attachment 652845 [details] [diff] [review]
Revised patch

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

f+ with a few minor nits.

::: dom/media/PeerConnection.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +

ekr said on IRC he would fix the whitespace issues before comitting, so I will ignore them at his request.

::: media/mtransport/test/transport_unittests.cpp
@@ +139,5 @@
> +    nsresult res = dtls_->SetVerificationAllowAll();
> +    ASSERT_TRUE(NS_SUCCEEDED(res));    
> +  }
> +  void SetDtlsPeer(TransportTestPeer *peer, bool damage) {
> +    unsigned char fingerprint_to_set[64];

Should this also be kMaxDigestLength instead of 64?

@@ +303,5 @@
>    std::vector<mozilla::RefPtr<NrIceMediaStream> > streams_;
>    std::map<std::string, std::vector<std::string> > candidates_;
>    TransportTestPeer *peer_;
>    bool gathering_complete_;
>    unsigned char fingerprint_[100];

Should this also be kMaxDigestLength instead of 100?

::: media/mtransport/transportlayerdtls.cpp
@@ +771,2 @@
>  // This always returns true, but stores the certificate for
>  // later use.

This comment is now wrong, isn't it?

@@ +784,5 @@
> +                                                  PRBool isServer) {
> +  CERTCertificate *peer_cert = NULL;
> +  peer_cert = SSL_PeerCertificate(fd);  
> +
> +  PR_ASSERT(verification_mode_ != VERIFY_UNSET);

Should also ASSERT that peer_cert_ is NULL (if not, we'll leak it on success below, or leave it pointing to a stale certificate on failure).

@@ +787,5 @@
> +
> +  PR_ASSERT(verification_mode_ != VERIFY_UNSET);
> +  switch (verification_mode_) {
> +    case VERIFY_UNSET:
> +      return SECFailure;  // Can't happen

If it ever did happen, this would leak peer_cert, wouldn't it? Can't we just omit this case entirely? We'll return SECFailure by default.

Comment 3

[Eric Rescorla (:ekr)]

Attachment: Revised patch (2)