Closed
Bug 783618
Opened 12 years ago
Closed 12 years ago
Add support for verifying DTLS fingerprints to mtransport
Categories
(Core :: WebRTC: Networking, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ekr, Unassigned)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
15.78 KB,
patch
|
Details | Diff | Splinter Review |
This code adds checking of the peer certificate
Reporter | ||
Updated•12 years ago
|
Attachment #652836 -
Attachment is patch: true
Reporter | ||
Comment 1•12 years ago
|
||
Attachment #652836 -
Attachment is obsolete: true
Attachment #652845 -
Flags: feedback?(tterribe)
Updated•12 years ago
|
Attachment #652845 -
Attachment is patch: true
Comment 2•12 years ago
|
||
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+
Reporter | ||
Comment 3•12 years ago
|
||
Attachment #652845 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Attachment #652867 -
Attachment is patch: true
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•