Closed Bug 783618 Opened 12 years ago Closed 12 years ago

Add support for verifying DTLS fingerprints to mtransport

Categories

(Core :: WebRTC: Networking, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ekr, Unassigned)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch First patch (obsolete) — Splinter Review
This code adds checking of the peer certificate
Attachment #652836 - Attachment is patch: true
Attached patch Revised patch (obsolete) — Splinter Review
Attachment #652836 - Attachment is obsolete: true
Attachment #652845 - Flags: feedback?(tterribe)
Attachment #652845 - Attachment is patch: true
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.
Attachment #652845 - Flags: feedback?(tterribe) → feedback+
Attachment #652845 - Attachment is obsolete: true
Attachment #652867 - Attachment is patch: true
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: