Closed Bug 1281665 Opened 3 years ago Closed 3 years ago

nsIClientAuthDialogs.chooseCertificate() should not use the CN of a server cert for the user to examine

Categories

(Core :: Security: PSM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

(David Keeler [:keeler] (use needinfo?) from Bug 307081 comment #13)
> ::: security/manager/ssl/nsNSSIOLayer.cpp:2298
> (Diff revision 1)
> >        }
> >  
> >        NS_ASSERTION(nicknames->numnicknames == NumberOfCerts, "nicknames->numnicknames != NumberOfCerts");
> >  
> >        // Get CN and O of the subject and O of the issuer
> >        UniquePORTString ccn(CERT_GetCommonName(&mServerCert->subject));
> 
> This should be addressed in a follow-up, but using the common name of the
> server certificate is probably not correct. If a cert has a cn of
> "example.com" but a san containing "example.com" and "example.org",
> connecting to "example.org" and being presented with a dialog talking about
> "example.com" would be confusing.
I might work on this at some later point, so P2 for now I guess.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
chooseCertificate() currently uses a concatenation of the Common Name of the
server cert and the port of the server to allow the user to identify the server
requesting client authentication. Unfortunately, this approach is flawed, since
it doesn't take into account things like SAN entries, which might be very
different from the CN.

Using the hostname instead avoids this problem. If we've reached the point of
asking the user to choose a client cert, the server cert has already been
verified to chain up to a trust anchor, or an exception has been added for it.
As such, the hostname is guaranteed to be correct.

Review commit: https://reviewboard.mozilla.org/r/66852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66852/
Attachment #8774376 - Flags: review?(dkeeler)
Attachment #8774376 - Flags: review?(bugmail)
Comment on attachment 8774376 [details]
Bug 1281665 - Change nsIClientAuthDialogs.chooseCertificate() to use hostname instead of CN.

https://reviewboard.mozilla.org/r/66852/#review63640

I don't think I'm qualified to review the main part of this change. The syntax changes you're making in NSSDialogService.js seem unnecessary but r+ for that if there's a legitimate reason for it.

::: mobile/android/components/NSSDialogService.js:33
(Diff revision 1)
>     * any other context.
>     *
>     * @param {String} input The input to interpret as a plain string.
>     * @returns {String} The escaped input.
>     */
> -  escapeHTML: function(input) {
> +  escapeHTML(input) {

Is there any particular reason you're changing this? It seems unrelated to the main change you're making here.
Comment on attachment 8774376 [details]
Bug 1281665 - Change nsIClientAuthDialogs.chooseCertificate() to use hostname instead of CN.

https://reviewboard.mozilla.org/r/66852/#review63690

I think this is a good change to make. That said, when I was testing this out, I got a bit confused by what the dialog was even trying to tell me in that section. It would be nice if we could somehow reuse the site identity block to indicate to the user who/what is asking them to identify themselves, but I think that would require significant architectural changes (which would be good, but that's something for future work if we even want to contintinue to invest in client certificates). Note also bug 1289186, which indicates that we might not actually know that the certificate is valid for the hostname when we're showing the client auth dialog (luckily, it doesn't appear to matter since Firefox closes the connection before sending the client certificate).

::: security/manager/ssl/nsIClientAuthDialogs.idl:31
(Diff revision 1)
>     * @param selectedIndex Index of the cert in |certList| that the user chose.
>     *                      Ignored if the return value is false.
>     * @return true if a certificate was chosen. false if the user canceled.
>     */
>    boolean chooseCertificate(in nsIInterfaceRequestor ctx,
> -                            in AString cnAndPort,
> +                            in AUTF8String hostname,

I think it would be most consistent to have the string types in this function be all the same (which would mean that the UTF-8 -> UTF-16 conversions can all happen at the same time).
Attachment #8774376 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/66852/#review63640

Ah, thanks anyways!

> Is there any particular reason you're changing this? It seems unrelated to the main change you're making here.

No significant reason - IMO the code just looks better using the newer ES6 syntax.
I'll revert to the previous style.
https://reviewboard.mozilla.org/r/66852/#review63690

Thanks!

> require significant architectural changes
Yeah, doesn't seem worth it at the moment.

> bug 1289186
Oops - looks like the server I originally used to test my theory on doesn't exhibit the bug for some reason.
I'll update the commit message as appropriate.

> I think it would be most consistent to have the string types in this function be all the same (which would mean that the UTF-8 -> UTF-16 conversions can all happen at the same time).

Sounds good.
Comment on attachment 8774376 [details]
Bug 1281665 - Change nsIClientAuthDialogs.chooseCertificate() to use hostname instead of CN.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66852/diff/1-2/
keeler: Could you review the parts under mobile/ as well if you haven't already? Thanks.
Flags: needinfo?(dkeeler)
Yep - LGTM.
Flags: needinfo?(dkeeler)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fefec564929
Change nsIClientAuthDialogs.chooseCertificate() to use hostname instead of CN. r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2fefec564929
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.