Closed
Bug 307081
Opened 19 years ago
Closed 8 years ago
nsIClientAuthDialogs::ChooseCertificate should pass an nsIArray of nsIX509Certs, not strings
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: sfraser_bugs, Assigned: Cykesiopka)
References
Details
(Keywords: addon-compat, Whiteboard: [kerh-ehz][psm-assigned])
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
kats
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
keeler
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kats
:
review+
keeler
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
keeler
:
review+
|
Details |
14.59 KB,
patch
|
Details | Diff | Splinter Review |
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!
Updated•19 years ago
|
Whiteboard: [kerh-ehz]
Updated•17 years ago
|
QA Contact: psm
Assignee | ||
Comment 2•8 years ago
|
||
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).
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Whiteboard: [kerh-ehz] → [kerh-ehz][psm-assigned]
Assignee | ||
Comment 3•8 years ago
|
||
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/
Attachment #8761029 -
Flags: review?(bugmail.mozilla)
Attachment #8761030 -
Flags: review?(dkeeler)
Attachment #8761031 -
Flags: review?(dkeeler)
Attachment #8761031 -
Flags: review?(bugmail.mozilla)
Attachment #8761032 -
Flags: review?(dkeeler)
Attachment #8761032 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 4•8 years ago
|
||
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/
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
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/
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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!
Attachment #8761029 -
Flags: review?(bugmail.mozilla) → review+
Comment 10•8 years ago
|
||
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.
Attachment #8761031 -
Flags: review?(bugmail.mozilla) → review+
Comment 11•8 years ago
|
||
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.
Attachment #8761032 -
Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8761030 -
Flags: review?(dkeeler) → review+
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.
Attachment #8761031 -
Flags: review?(dkeeler)
Attachment #8761032 -
Flags: review?(dkeeler) → review+
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.
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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.
Assignee | ||
Comment 17•8 years ago
|
||
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/
Attachment #8761031 -
Flags: review?(dkeeler)
Attachment #8761032 -
Flags: review+
Assignee | ||
Comment 18•8 years ago
|
||
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/
Assignee | ||
Comment 19•8 years ago
|
||
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/
Assignee | ||
Comment 20•8 years ago
|
||
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/
Attachment #8761031 -
Flags: review?(dkeeler) → review+
Comment on attachment 8761031 [details] Bug 307081 - Clean up nsIClientAuthDialogs.idl and implementations. https://reviewboard.mozilla.org/r/58388/#review57312 Ok - great!
Assignee | ||
Comment 22•8 years ago
|
||
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/
Assignee | ||
Comment 23•8 years ago
|
||
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/
Assignee | ||
Comment 24•8 years ago
|
||
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/
Assignee | ||
Comment 25•8 years ago
|
||
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/
Assignee | ||
Comment 26•8 years ago
|
||
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.)
Keywords: checkin-needed
Priority: -- → P1
Comment 27•8 years ago
|
||
Pushed by cbook@mozilla.com: 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
Keywords: checkin-needed
Comment 28•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 29•8 years ago
|
||
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).
Keywords: addon-compat
You need to log in
before you can comment on or make changes to this bug.
Description
•