Minor bug in media/mtransport/transportlayerdtls.cpp

RESOLVED FIXED in mozilla25

Status

()

Core
WebRTC: Networking
P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Nickolai Zeldovich, Assigned: Nickolai Zeldovich)

Tracking

Trunk
mozilla25
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC] [blocking-webrtc-])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.40 Safari/537.17

Steps to reproduce:

I looked for various bugs in the Mozilla source code.


Actual results:

I found a minor issue in media/mtransport/transportlayerdtls.cpp, where TransportLayerDtls::GetClientAuthDataHook() incorrectly checks for out-of-memory conditions.  I've attached a fairly obvious patch.
(Assignee)

Comment 1

4 years ago
Created attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
QA Contact: jsmith
Attachment #694171 - Flags: review?(ekr)

Comment 2

4 years ago
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

lgtm
Attachment #694171 - Flags: review?(ekr) → review+

Comment 3

4 years ago
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

I think these actually may not be able to fail, because they just increment a refct...
Attachment #694171 - Flags: feedback?(bsmith)
(In reply to Eric Rescorla (:ekr) from comment #3)
> I think these actually may not be able to fail, because they just increment
> a refct...

True for CERT_DupCertificate but not for SECKEY_CopyPrivateKey. However, I bet libbsl itself sanity checks the result. (Worth checking, to see if Nickolai qualifies for the bug bounty.)

Thanks for the patch, Nickolai!.

Comment 5

4 years ago
    The answer appears to be that it checks it.

    /* check what the callback function returned */
        if ((!ss->ssl3.clientCertificate) || (!ss->ssl3.clientPrivateKey)) {
            /* we are missing either the key or cert */
            if (ss->ssl3.clientCertificate) {
                /* got a cert, but no key - free it */
                CERT_DestroyCertificate(ss->ssl3.clientCertificate);
                ss->ssl3.clientCertificate = NULL;
            }
            if (ss->ssl3.clientPrivateKey) {
                /* got a key, but no cert - free it */
                SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
                ss->ssl3.clientPrivateKey = NULL;
            }
            goto send_no_certificate;
        }
So, I take it this should be RESOLVED-INVALID?
Flags: needinfo?(bsmith)

Comment 7

4 years ago
Well, it's still a defect, since we're not correctly returning an error when we
should. And this patch looked correct when I looked at it before.

Updated

4 years ago
Assignee: nobody → ekr
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bsmith)
Priority: -- → P3
Whiteboard: [WebRTC] [blocking-webrtc-]
Sorry, assigning to the person who has the patch up for review.   Just waiting on feedback from bsmith
Assignee: ekr → kolya
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

This should be uplifted to aurora if WebRTC is enabled in Aurora.
Attachment #694171 - Flags: feedback?(bsmith) → feedback+
Attachment #694171 - Attachment is patch: true

Updated

4 years ago
Attachment #694171 - Flags: checkin?(ekr)
Comment on attachment 694171 [details] [diff] [review]
Patch for TransportLayerDtls::GetClientAuthDataHook()

Just use the checkin-needed keyword in the future please.
Attachment #694171 - Flags: checkin?(ekr) → checkin+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b26ac05ab0b0

Thanks for the patch, Nickolai! One request, for future patches, please make sure that you have Mercurial configured to generate patches will all the necessary commit information. It makes life much easier for those landing on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
https://hg.mozilla.org/mozilla-central/rev/b26ac05ab0b0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.