307081 - nsIClientAuthDialogs::ChooseCertificate should pass an nsIArray of nsIX509Certs, not strings

Status: RESOLVED FIXED

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

Description

[Simon Fraser [no longer active]]

nsIClientAuthDialogs::ChooseCertificate looks like:

  NS_IMETHOD ChooseCertificate(nsIInterfaceRequestor *ctx, const PRUnichar *cn,
const PRUnichar *organization, const PRUnichar *issuer, const PRUnichar
**certNickList, const PRUnichar **certDetailsList, PRUint32 count, PRInt32
*selectedIndex, PRBool *canceled) = 0;

which makes it hard for an implementor to do non-sucky UI.

Despite the name, the strings of 'certNickList' are not real nicknames; they are
nicknames with the serial number appended:

http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSCertificate.cpp#258

so without some string parsing, I can't even use them to look up nsIX509Certs!

Comment 1

[Kai Engert (:KaiE:)]

reassign bug owner.
mass-update-kaie-20120918

Comment 2

[:Cykesiopka]

I poked around, and it looks like making this change would have benefits even from a non-embedding perspective. In particular, it should let us clean up the client auth code a bit (something the code sorely needs).

Comment 3

[:Cykesiopka]

Attachment: Bug 307081 - Fix buggy Android implementation of nsIClientAuthDialogs::ChooseCertificate().

This fixes two issues:
1. Passing a literal 1 as the |length| argument to formatStringFromName(). This
   is obviously incorrect and should instead be the length of the given arg
   list.
2. Directly setting the |selectedIndex| outparam to a number. XPIDL outparams
   on the JS side are actually objects that wrap the true outparam value, which
   must be accessed via |.value|.

Review commit: https://reviewboard.mozilla.org/r/58384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58384/

Comment 4

[:Cykesiopka]

Attachment: Bug 307081 - Expose nsNSSCertificate.cpp GetKeyUsagesString() as the keyUsages attribute on nsIX509Cert.

This allows nsNSSCertificate::FormatUIStrings() to be reimplemented in JS, which
is a necessary step for making nsIClientAuthDialogs::ChooseCertificate() pass an
nsIArray of nsIX509Certs.

Also removes some deprecated and unused constants.

Review commit: https://reviewboard.mozilla.org/r/58386/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58386/

Comment 5

[:Cykesiopka]

Attachment: Bug 307081 - Clean up nsIClientAuthDialogs.idl and implementations.

This fixes the following in the IDL:
1. Misleading or unclear parameter names in the IDL. |cn| is practice is both
   the CN of a server cert and the port of the server, and |issuer| is the
   Organization of the issuer cert of the server cert.
2. Use of the |wstring| type. |AString| is generally preferred, and has the
   benefit of letting implementations skip null checks due to the use of
   references.
3. Using an explicit |canceled| outparam instead of just setting a return type.
   There is no need for the outparam if the return type can be used.
4. Using |long| (int32_t) for |selectedIndex|. |unsigned long| (uint32_t) is
   more logical, and paves the way for future changes.

This fixes the following in the Android implementation:
1. Lack of checks to ensure the QueryInterface() call succeeded. In practice,
   the call will always succeed, but it's good practice to check anyways.
2. Setting |pref| vars to an nsIPrefService instance initially, then later
   setting it to pref branch instance later on. This is confusing and
   unnecessary.

This fixes the following in the desktop implementation:
1. Lack of null pointer checking.
2. Trying to get a parent window ref off a context that doesn't actually support
   doing so.
3. Setting |pref| vars to an nsIPrefService instance initially, then later
   setting it to pref branch instance later on. This is confusing and
   unnecessary.
4. Abusal of the CAPS bundle.
5. Unnecessary variables.
6. Variables declared far away from where they are used.
7. Variable shadowing.
8. Style issues.
9. Lack of documentation.

This also fixes the following:
1. Lack of localisation notes.

Review commit: https://reviewboard.mozilla.org/r/58388/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58388/

Comment 6

[:Cykesiopka]

Attachment: Bug 307081 - Make nsIClientAuthDialogs::ChooseCertificate() pass an nsIArray of nsIX509Certs, not strings.

This provides implementations of ChooseCertificate() with more flexibility, and
allows callers of ChooseCertificate() to be less complex.

A portion of this work involves reimplementing
nsNSSCertificate::FormatUIStrings() in JS and improving UI strings for l10n.

Review commit: https://reviewboard.mozilla.org/r/58390/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58390/

Comment 7

[:Cykesiopka]

https://reviewboard.mozilla.org/r/58382/#review55244

keeler: Please review the security/ bits.
kats: Please review the mobile/ bits.
      It looks like you reviewed the original Android nsIClientAuthDialogs implementation, but feel free to redirect as necessary.

Thanks.

Comment 8

[:Cykesiopka]

Attachment: Not for checkin or review: Mochitest and support files to ease testing

This patch includes:
1. Two slightly different client certificates available for use by mochitests.
2. A script to generate these certs.
3. test_clientauth.html, a mochitest that allows the client auth UI to easily be triggered.

test_clientauth.html should work on desktop builds.
Theoretically it also works on Android.
However, if you're like me and couldn't get mochitests to actually run on your Android setup:
1. Build with all the attached patches (including this one) applied.
2. Do |mach package|, |mach install| and any other steps necessary to push the build to a device or emulator.
3. Run the test build via the device launcher (this ensures a profile is generated).
4. Run test_clientauth.html via |mach mochitest|.
5. Check the output to see where the temporary profile is.
6. Wait for the pk12util imports to finish (key4.db and cert9.db should be present now, with the appropriate certs imported).
7. Copy key4.db and cert9.db somewhere.
8. Kill the test.
9. Replace key4.db and cert9.db in your existing test build profile.
10. Start the test build and navigate to https://www.bennish.net/certs/ (add a cert override if you need to) or some other client auth requesting site.
11. Click the "Log in using that certificate" link.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8761029 [details]
Bug 307081 - Fix buggy Android implementation of nsIClientAuthDialogs::ChooseCertificate().

https://reviewboard.mozilla.org/r/58384/#review55364

Thanks for catching these!

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8761031 [details]
Bug 307081 - Clean up nsIClientAuthDialogs.idl and implementations.

https://reviewboard.mozilla.org/r/58388/#review55366

mobile/android changes look good to me, I didn't review the rest.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8761032 [details]
Bug 307081 - Make nsIClientAuthDialogs::ChooseCertificate() pass an nsIArray of nsIX509Certs, not strings.

https://reviewboard.mozilla.org/r/58390/#review55368

Ditto, mobile/android looks good.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8761030 [details]
Bug 307081 - Expose nsNSSCertificate.cpp GetKeyUsagesString() as the keyUsages attribute on nsIX509Cert.

https://reviewboard.mozilla.org/r/58386/#review55468

Looks good. Just the two comments.

::: security/manager/ssl/nsIX509Cert.idl:237
(Diff revision 1)
>     */
>    void getUsagesString(in boolean localOnly, out uint32_t verified, out AString usages);
>  
>    /**
> +   * A comma separated list of human readable strings representing the
> +   * certificate's key usages.

Comment nit: maybe work in "the contents of the key usage extension, if present"? (Also, is there anything to differentiate an empty key usage extension from one that isn't present at all?)

::: security/manager/ssl/nsNSSCertificate.cpp:297
(Diff revision 1)
>    const char16_t comma = ',';
>  
>    if (keyUsage & KU_DIGITAL_SIGNATURE) {
>      rv = nssComponent->GetPIPNSSBundleString("CertDumpKUSign", local);
>      if (NS_SUCCEEDED(rv)) {
>        if (!text.IsEmpty()) text.Append(comma);

Eh... technically these conditional bodies should be on their own lines and surrounded by curly braces. Going further, maybe it would be best to factor out this repeated pattern and just call a helper function 7 times. I'm not entirely sure it's worth it, but it would be a better implementation.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8761031 [details]
Bug 307081 - Clean up nsIClientAuthDialogs.idl and implementations.

https://reviewboard.mozilla.org/r/58388/#review55480

This looks good, and I believe it is as correct as the original implementation, but I don't think the original implementation is correct (see comments below). How much work would it be to remove the gotos and convert this to the regular check for error and return early style?

Also, commit message nit: maybe clarify that cn is the concatenation of the common name and port?

::: mobile/android/components/NSSDialogService.js:170
(Diff revision 1)
>        } catch (e) {
>          // pref is missing
>        }
>      }
>  
> -    let organizationString = this.formatString("clientAuthAsk.organization",
> +    let serverRequestedDetails = [

Is this eventually interpreted as HTML? It would be good to verify that this isn't an injection vector.

::: security/manager/pki/nsNSSDialogs.cpp:167
(Diff revision 1)
> -                                const char16_t* issuer,
> +                                const nsAString& organization,
> +                                const nsAString& issuerOrg,
>                                  const char16_t** certNickList,
>                                  const char16_t** certDetailsList, uint32_t count,
> -                                int32_t* selectedIndex, bool* canceled)
> +                        /*out*/ uint32_t* selectedIndex,
> +                        /*out*/ bool* success)

nit: maybe |certificateChosen| or similar instead of |success|?

::: security/manager/pki/resources/content/clientauthask.js:33
(Diff revision 1)
> -                         .getService(Components.interfaces.nsIPrefService);
> -    if (pref) {
> -      pref = pref.getBranch(null);
> -      try {
> +    try {
> -        rememberSetting =
> +      rememberSetting =
> -	  pref.getBoolPref("security.remember_cert_checkbox_default_setting");
> +        prefBranch.getBoolPref("security.remember_cert_checkbox_default_setting");

Can we just use Services.prefs.getBoolPref? (less code, and I don't think it will throw)

::: security/manager/pki/resources/content/pippki.js:13
(Diff revision 1)
>  /*
>   * These are helper functions to be included
>   * pippki UI js files.
>   */
>  
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

I'm assuming this was added so things like Cc and Ci could be used in clientauthask.js? If so, perhaps it would be best to have the declaration in that file until we fix up this file to use Cc/Ci instead of the long names.

::: security/manager/ssl/nsNSSIOLayer.cpp:2156
(Diff revision 1)
>      // find all user certs that are valid and for SSL
> -    certList.reset(CERT_FindUserCertsByUsage(CERT_GetDefaultCertDB(),
> -                                             certUsageSSLClient, false, true,
> -                                             wincx));
> +    UniqueCERTCertList certList(
> +      CERT_FindUserCertsByUsage(CERT_GetDefaultCertDB(), certUsageSSLClient,
> +                                false, true, wincx));
>      if (!certList) {
> -      goto noCert;
> +      goto loser;

How much work would it be to convert this function to the style of checking for errors and returning early? We could get rid of the gotos and simplify things a fair bit. It would mean changing more code, but I think it would be more clear that the implementation is correct in the end.

::: security/manager/ssl/nsNSSIOLayer.cpp:2228
(Diff revision 1)
>      }
>  
> -    bool canceled = false;
> -
>      if (hasRemembered) {
> -      if (rememberedDBKey.IsEmpty()) {
> +      if (!rememberedDBKey.IsEmpty()) {

This can just be |if (hasRemembered && !rememberedDBKey.IsEmpty()) {|

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

::: security/manager/ssl/nsNSSIOLayer.cpp:2425
(Diff revision 1)
>  loser:
>    if (mRV == SECSuccess) {
>      mRV = SECFailure;
>    }
>  done:
>    int error = PR_GetError();

I think there are cases in the current implementation where what this code gets from PR_GetError() has nothing to do with what the actual error was (e.g. if the user has cancelled the choose certificate dialog, I think), hence why it would be nice to restructure things to get rid of goto, etc.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8761032 [details]
Bug 307081 - Make nsIClientAuthDialogs::ChooseCertificate() pass an nsIArray of nsIX509Certs, not strings.

https://reviewboard.mozilla.org/r/58390/#review55528

This looks great. Hopefully the changes requested for the previous part won't cause this to need too much rebasing.

Comment 15

[:Cykesiopka]

https://reviewboard.mozilla.org/r/58386/#review55468

> Comment nit: maybe work in "the contents of the key usage extension, if present"? (Also, is there anything to differentiate an empty key usage extension from one that isn't present at all?)

Done. There isn't anything to differentiate between an empty key usage extension from one that isn't present at all - I've clarified this in the documentation.

> Eh... technically these conditional bodies should be on their own lines and surrounded by curly braces. Going further, maybe it would be best to factor out this repeated pattern and just call a helper function 7 times. I'm not entirely sure it's worth it, but it would be a better implementation.

It was pretty straightforward - so I just went ahead and refactored the code as suggested.

Comment 16

[:Cykesiopka]

https://reviewboard.mozilla.org/r/58388/#review55480

See my answer below.

> Is this eventually interpreted as HTML? It would be good to verify that this isn't an injection vector.

It's interpreted as HTML at the Android OS level.
It looks like the existing code is vulnerable but not exploitable.
I'll take care of this in Bug 1281661.

> Can we just use Services.prefs.getBoolPref? (less code, and I don't think it will throw)

Yes. getBoolPref() I believe does throw for non-existent prefs, but it turns out security.remember_cert_checkbox_default_setting is always supposed to be present anyways.

> I'm assuming this was added so things like Cc and Ci could be used in clientauthask.js? If so, perhaps it would be best to have the declaration in that file until we fix up this file to use Cc/Ci instead of the long names.

Yes - I was getting annoyed at how much space was being wasted by the long form.
I guess I'll convert the entire pki/ folder to the short form later.

> How much work would it be to convert this function to the style of checking for errors and returning early? We could get rid of the gotos and simplify things a fair bit. It would mean changing more code, but I think it would be more clear that the implementation is correct in the end.

It looks like doing so wouldn't be too difficult, but is somewhat tedious.
I plan to refactor this code further, so I would prefer to defer doing this for now.

> 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 filed Bug 1281665 for this.

Comment 17

[:Cykesiopka]

Comment on attachment 8761029 [details]
Bug 307081 - Fix buggy Android implementation of nsIClientAuthDialogs::ChooseCertificate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58384/diff/1-2/

Comment 18

[:Cykesiopka]

Comment on attachment 8761030 [details]
Bug 307081 - Expose nsNSSCertificate.cpp GetKeyUsagesString() as the keyUsages attribute on nsIX509Cert.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58386/diff/1-2/

Comment 19

[:Cykesiopka]

Comment on attachment 8761031 [details]
Bug 307081 - Clean up nsIClientAuthDialogs.idl and implementations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58388/diff/1-2/

Comment 20

[:Cykesiopka]

Comment on attachment 8761032 [details]
Bug 307081 - Make nsIClientAuthDialogs::ChooseCertificate() pass an nsIArray of nsIX509Certs, not strings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58390/diff/1-2/

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8761031 [details]
Bug 307081 - Clean up nsIClientAuthDialogs.idl and implementations.

https://reviewboard.mozilla.org/r/58388/#review57312

Ok - great!

Comment 22

[:Cykesiopka]

Comment on attachment 8761029 [details]
Bug 307081 - Fix buggy Android implementation of nsIClientAuthDialogs::ChooseCertificate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58384/diff/2-3/

Comment 23

[:Cykesiopka]

Comment on attachment 8761030 [details]
Bug 307081 - Expose nsNSSCertificate.cpp GetKeyUsagesString() as the keyUsages attribute on nsIX509Cert.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58386/diff/2-3/

Comment 24

[:Cykesiopka]

Comment on attachment 8761031 [details]
Bug 307081 - Clean up nsIClientAuthDialogs.idl and implementations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58388/diff/2-3/

Comment 25

[:Cykesiopka]

Comment on attachment 8761032 [details]
Bug 307081 - Make nsIClientAuthDialogs::ChooseCertificate() pass an nsIArray of nsIX509Certs, not strings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58390/diff/2-3/

Comment 26

[:Cykesiopka]

Thanks for the reviews!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63971d39528952025578c8cca0906d53cbf54559
(The WinXP opt failure seems infra related (I saw a similar failure on m-i), and the OSX 10.7 opt seems to be caused by Bug 1195477.)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f036febe1ced
Fix buggy Android implementation of nsIClientAuthDialogs::ChooseCertificate(). r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/33b793437385
Expose nsNSSCertificate.cpp GetKeyUsagesString() as the keyUsages attribute on nsIX509Cert. r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/451cf1f26cb5
Clean up nsIClientAuthDialogs.idl and implementations. r=kats,keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/34398aad1f9a
Make nsIClientAuthDialogs::ChooseCertificate() pass an nsIArray of nsIX509Certs, not strings. r=kats,keeler

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/f036febe1ced
https://hg.mozilla.org/mozilla-central/rev/33b793437385
https://hg.mozilla.org/mozilla-central/rev/451cf1f26cb5
https://hg.mozilla.org/mozilla-central/rev/34398aad1f9a

Comment 29

[:Cykesiopka]

The patches here make various IDL changes, so I'm marking addon-compat just in case (I don't have access to the addons repository, so I can't really tell if there really will be compat impact).