823338 - Minor bug in media/mtransport/transportlayerdtls.cpp

Status: RESOLVED FIXED

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

Description

[Nickolai Zeldovich]

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.

Comment 1

[Nickolai Zeldovich]

Attachment: Patch for TransportLayerDtls::GetClientAuthDataHook()

Comment 2

[Eric Rescorla (:ekr)]

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

lgtm

Comment 3

[Eric Rescorla (:ekr)]

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...

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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

[Eric Rescorla (:ekr)]

    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;
        }

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

So, I take it this should be RESOLVED-INVALID?

Comment 7

[Eric Rescorla (:ekr)]

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.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Sorry, assigning to the person who has the patch up for review.   Just waiting on feedback from bsmith

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

This should be uplifted to aurora if WebRTC is enabled in Aurora.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Just use the checkin-needed keyword in the future please.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b26ac05ab0b0