Closed Bug 92131 Opened 23 years ago Closed 23 years ago

Hole in the PKI process caused by PSM inability of presenting all certificates with the same subject name during authentication.

Categories

(Core Graveyard :: Security: UI, defect, P1)

Product:
Core Graveyard
Component:
Security: UI
Version:
1.0 Branch
Platform:
x86
Windows NT
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
psm2.2

People

(Reporter: awnuk, Assigned: inactive-mailbox)

Details

Attachments

(15 files)

Screenshot of suggested fix
7.06 KB, image/png
Details
patch / suggested fix
24.42 KB, patch
Details | Diff | Splinter Review
updated patch, resolved conflicts with current trunk
24.46 KB, patch
Details | Diff | Splinter Review
New patch as suggested
25.12 KB, patch
Details | Diff | Splinter Review
Updated patch, added purposes to displayed information
26.04 KB, patch
Details | Diff | Splinter Review
New screenshot
7.26 KB, image/png
Details
Newest incarnation of patch
27.83 KB, patch
Details | Diff | Splinter Review
Snapshot for previous patch
16.49 KB, image/gif
Details
Updated patch
27.35 KB, patch
Details | Diff | Splinter Review
manual selection dialog is w-i-d-e
13.70 KB, image/gif
Details
normal width in cert selection dialog
12.40 KB, image/gif
Details
Additional patch as described above.
16.83 KB, patch
Details | Diff | Splinter Review
To ease reviewing, same patch, ignoring space changes
7.46 KB, patch
Details | Diff | Splinter Review
Verisign cert selected
18.85 KB, image/gif
Details
Additional patch to display all certificates, allowing duplicates.
991 bytes, patch
blizzard
: superreview+
Details | Diff | Splinter Review
There is a hole in PKI process related to end user revocation or renewal, when the end user holds more then one certificate with the same subject name. Some important customers are requesting that end user can only revoke a certificate that belongs to him and he can present this certificate during authentication. Unfortunately, when the end user holds more then one certificate with the same subject name, he has no control over, which certificate is going to be revoked, because he can only select certificate nick name. Since his certificate that he really didn't want to revoke is revoked now, he might have restricted access to operations, which he would really like to do, like revoking the certificate that he really wanted to revoke from the beginning. Renewals have similar problems.
->javi P1 t->2.1 I saw the problems.
Assignee: ssaux → javi
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Target Milestone: --- → 2.1
adding nsenterprise to all P1, P2 PSM bugs with target milestone of 2.1
Keywords: nsenterprise
-> kai
Assignee: javi → kai.engert
This is a general problem, not only when revoking, but always when the user needs to decide which certificate to use. I'll add a button to the dialog that can be used to view the details of the selected certificate. In addition, the communication between caller and dialog currently uses the certificate nickname. I will check if this is unique, and if not, change the code to use a key that is unique.
Status: NEW → ASSIGNED
Another option that was discussed was having a table of all the certs in the window (much like the cert manager does) instead of just listing nicknames. That way the user can see which serial number/expiration data or what not the cert has.
I've started coding what I suggested, but I've come a to point where I want to stop for a moment and dump my mind. Javi, I'm unsure about the idea with presenting a user with something like the outliner. The user is surfing, it might be confusing if a big matrix of certificate attributes pops up. The window would have to be large, because we need to display lot's of attributes. And the user might not understand what he should do. In a large window containing a an outliner, the small explanatory message somewhere in the dialog ("please select a certificate") might not be seen immediately. This is just a feeling that I have. In addition, my impression is that certificate manager is coming up slowly. It takes 7 seconds on my 500mhz machine. I think this is mainly because of nsCertOutliner::LoadCerts, which does lots of copying and comparing etc. To come back to my suggestion from an earlier comment: While I was working on it, my intention was to pass on the dbKey of a cert, and let the dialog do the rest of querying interfaces and getting the attributes to display. But I saw that nsNSSCertificate::GetCertByDBKey doesn't use solely the dbKey as an argument, but a token identifier as well. I'm unsure about this. The design planned for it, but the implemtation ignores the token parameter. Does it mean certificates stored on tokens can't be found when I call this function? If this is true, certificates stored on tokens would not be usable for client authentication, if I used this function as a kind of callback from JS. I'm tempted to switch to a much easier design. We could extend the certificate picker dialog by a wrapping text display. Put that below the picker drop down box. Label it "Details of selected Certificate", and make space for a couple of lines. Let's create a string, that in a space saing way lists all the important attributes of the single currently selected certificate. Whenever the user selects a different certificate from the drop down list, the text display is updated immediately and displays the new details. This approach is much easier to implement. The caller creates two strings for every certificate. The current string, the non-unique nickname, which remains as it is, and the new detailed string. The string that gets returned from the dialog is the detailed string, which the caller compares on return. A conflict seems very unlikely if we include enough attributes.
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer
I implemented what I suggested, with one change, I don't return the detailed string but the index of the selected entry, which simplifies things. Please have a look at the screenshot. Is this implementation acceptable?
How does this deal with the case of 2 certificates with the same nickname being in the list? A very likely scenario with dual certs.
If two certificates have the same nickname, and both are still valid, then there will be two identical entries in the dropdown list. But clicking on each will reveal the details for each. But your question is good, maybe we really should add some additional info to the list entry, that makes it easier for the user to see that these are actually different entries. How about adding the serial number in parenthesis? I'll adjust the patch.
With dual certs, it would be more useful to show the cert's purpose (sign, encrypt) wouldn't it?
This dialog displays only a subset of all certificates the user owns - only those having a usage of SSLClient. Should we add the possible cert usages to the detailed information text?
To be more detailed, I think: If a user has multiple certificates, because one cert for every different usage is issued, then only one certificate will be shown in this dialog. And in the case where a user has multiple certificates with overlapping purposes, then the purpose information will not help much for differentiating the certs. However, displaying the cert purposes in the detailed box still sounds helpful for picking the right cert, I will add another line to the detailed box.
Attached image New screenshot —
Here's a proposal for what we put into the "Details" section of Kai's SSL-client-auth dialog: -- Issued to: Subject: $end_entity_subject Serial number: $end_entity_serial_number Valid from: $starting_date to $expriation_date Purposes: $purposes Issued by: Subject: $issuer_subject MD5 Fingerprint: $md5 SHA1 Fingerprint: $sha1 -- So I'd see this on my computer when visiting Netscape SSL sites that request my cert: -- Issued to: Subject: UID=lord, CN=Bob Lord, E=lord@netscape.com, DC=netscape, DC=com Serial number: 3D:07 Valid from: 6/12/2001 15:06:30 PM to 12/9/2001 14:06:30 PM Purposes: Client,Sign Issued by: Subject: CN=Intranet Certificate Authority, OU=AOL Technologies, O=America Online Inc, L=Mountain View, ST=CA, C=US MD5 Fingerprint: b2:f0:2b:2c:b1:66:06:aa:f1:4b:4a:20:5c:25:4d:40 SHA1 Fingerprint: e5:1a:38:41:f2:20:12:e9:f6:a3:12:9c:e1:6e:98:d4:e1:6e:98:d4 -- There are 8 available lines in the Details section of Kai's prototype. This new proposal for the layout of the text takes 9 lines, assuming we scroll to the right for long lines (like the Issuer Subject line above). However, even we reduce the number of visible lines to 6 or 7, all the important information is still "above the fold". You don't need to scroll in most cases. We don't need to show the "Stored in:" value since I believe that's preprended to the Nickname in the popup list. This model might even work to pick which certs you want to use to sign and encrypt for S/MIME. Comments?
The information Bob suggested is fine. I do think adding the details window to this dialog is a great improvement. It empowers the user.
Adding patch, review keywords. Javi, can you please review? The latest patch is based on what we discussed externally of Bugzilla. I wonder if you accept the change to nsIX509Cert.idl? In addition I added some sanity checks in nsNSSCertificate.cpp which can't hurt.
Keywords: patch, review
Make getting the expired time and issued on time a part of the nsIX509CertValidity interface, and you'll have r=javi. The nsIX509Certificate interface is starting to get cluttered. Let's avoid adding attributes/methods to it unless they're absolutely necessary. Seems that converting the times belongs in the validity interface since there's already a validity attribute with the cert.
I think you are right. While I was implementing the suggested change, I even was tempted to get rid of interface changes completely. I tried to move the date formatting code into nsNSS_SSLGetClientAuthData. But I failed. I would have been required to call createInstance for dateFormatter, and get a proxy for it, because it lives on the other thread. However, this didn't work. I never got a proxy, but always zero. I finally came to the conclusion that I either don't understand what to do, or there might a be bug in the DateFormatter factory code. As I already spent too much time on this I stopped researching it further and switched to your suggestion, extending nsIX509CertValidity.
r=javi for latest patch.
+NS_IMETHODIMP nsX509CertValidity::GetNotBeforeLocalTime(PRUnichar **aNotBeforeLocalTime) Can you please change the signature of those functions not to include negatives? For example GetNotBeforeLocalTime should be the same as GetAfterLocalTime, right? There's also a variable in there: + PR_ExplodeTime(mNotBefore, PR_LocalTimeParameters, &explodedTime); mNotBefore should be mAfter, right? Fix that and you have an sr=blizzard
I would like to object. Please see RFC 2459. The attributes of a X.509 certificate describing the validity are named "notBefore" and "notAfter". In addition, the existing interface nsIX509CertValidity already defines these attribute names. What I did was adding different representations of how you can get theses attributes from the interface. Basically I agree that avoiding negative namings is a plus, but in this case we follow the standard. If you describe a validity, using names like "issued" or "expires" is not sharp enough, because the names do not tell whether the border is included or excluded. Using the name "notAfter", this becomes clear. Your suggestion, changing "notBefore" to "after" is wrong, IMHO, because it changes the definition of the equal case. With "notBefore", the equal case is included in the range. With "after", the equal case is excluded.
OK, that's completely reasonable. You're all set.
Patch checked in.
Why is the width of the dialog not constant?
The size is not constant, because the layouter chooses at runtime. I think this is better than having a fixed size dialog, which doesn't work on some platforms. I think we should increase the height recommondation for the text box by one.
The patch I checked in was partially crap. First, it broke the build, luckily for Mac only. Calling GetProxyForObject on a nsNSSCertificate, requesting interface nsIX509Cert, doesn't work on Mac. We therefore had to check in an emergency patch tonight. To fix it correctly, and without producing a memory leak caused by the emergency checkin, I introduce an intermedia do_QueryInterface to nsIX509Cert first before getting the Proxy, and use NS_ADDREF/NS_RELEASE. I learned that nsCOMPtr should only be used for interfaces, and nsNSSCertificate doesn't have a default constructor, which nsCOMPtr does not like. Second, the code contains an error. I used the wrong variable for controlling the allocation size of the certificate array. This causes crashes. I apologize. Third, to polish the appearance, we discussed to enlarge the text box by 1 line in height.
r=javi
Attached image Verisign cert selected —
I suspect I know why some dialogs are larger than others. As you can see from this cert selection dialog, the Verisign serial number is very long. The length of the serial number probably forces the window to be wider than a cert with a shorter serial number. Also, long serial numbers like this make the pop-up element look rather nerdy or even frightening. Should we remove the serial number from the pop-up list?
I think, we only should remove it, if we agree on something else to append to the nickname.
You have some funny tab/spacing issues in that patch. Can you fix those? Other than that, sr=blizzard
Spaces & tabs cleaned up in function nsNSS_SSLGetClientAuthData. Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
I am trying to verify this on N6 2001091703 but failed. This is what I did: I got a cert (signing, ssl client only) from https://cfu:1031, then renewed the cert with the same subject name. Tried "renew" again, and I was only presented with the option to select the newer cert (the older, but still valid cert was not on the selection list). FYI, this does not impact CMS 4.2 sp2, because it will give you a selection of its own with certs matching the ssl client cert after you submitted a cert. I don't have the right permission to reopen this bug, so someone else needs to do this for me.
Reopening as requested.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
target 2.2
Target Milestone: 2.1 → 2.2
in a non-renewal case: I got two equal-dn certs (different key material) with the same dn on the software token, only one cert is presented on the selection list. in another non-renewal case: I got one cert cert on soft token, an done cert on iButton, both with same dn, but different key material, both certs are presented on the selection list.
here is how you can reproduce : 0. create a fresh profile 1. go to https://cfu:1031 2. [Enrollment][Directory] 3. uid type "test1" and password type "test1", [Submit] (you'll be asked to do key archival, just click through it and you'll eventually get two certs, one signing/ssl client cert, and one encryption cert). do 1-3 one more time to get another cert (onto your soft token) 4. go to "Manage Certificates" and see the two certs both having same dn and verified 5. go to https://cfu:1031, [Renewal] or [Revocation], then [User Certificate], [Submit] expected to see both signing certs on the cert selection list. I saw only one of them. --- do the same with one from soft token, and one from iButton, you will see both certs presented on the cert selection list.
I did was Christina described in her latest comment and was able to reproduce the problem, and I found the cause. I have the four certs (2 pairs of two certs) in my cert database. The code uses NSS function CERT_FindUserCertsByUsage to query the certs that should be shown to the user. The behaviour of this function has two parameters that influence its behaviour: - onePerCertName - validOnly Currently, the code sets "onePerCertName" to true. I guess we want all certs per cert name. The wrong behaviour results in only one cert out of my four certs to be displayed in the list. I tested to set this parameter to false. The result was: I saw two certs, with serial numbers 17 and 19. I suggest to make this change to the production version. Regarding the second parameter "validOnly". Currently this is set to false, i.e. requesting invalid, expired certs to be displayed to the user. Is this correct? Or should we display only valid certs? This is particulary confusing, as in the mode, where the browser should select the cert automatically, only valid requests are requested from the cert database. To summarize: Current code in manual prompt mode ================================== Find one cert per name. Find all certs, including invalid certs. User decides which cert to use. Current code in automatic cert select mode ========================================== Find all certs for all names, allowing duplicated. But find only valid certs. Of the above, find the first in the list with a private key. Select this cert automatically. To fix the bug: Suggested new code in manual prompt mode ======================================== Find all certs for all names, allowing duplicates. But find only valid certs. User decides which cert to use. Please comment.
Status: REOPENED → ASSIGNED
> Currently, the code sets "onePerCertName" to true. > I guess we want all certs per cert name. > Regarding the second parameter "validOnly". > Currently this is set to false, i.e. requesting invalid, > expired certs to be displayed to the user. Is this correct? > Or should we display only valid certs? I want to add, in the manual selection case, there is a comment in the source that reads: /* find all user certs that are valid and for SSL */ /* note that we are allowing expired certs in this list */ I don't understand that comment. It says, valid certs including expired certs are requested. Isn't that statement inconsistent?
sometimes you need to use an expired cert to renew itself.
Kai. so the comment in the code may be somewhat sloppy. the writer may have meant: /* find all user certs that match the criteria set by the server */ /* note that we are allowing expired certs in this list */ Christina explained to me that CA can set policies such that if your cert is less than 30 days expired, then the CA will accept is a client auth for renewal purposes. Therefore my opinion is that the code should display all the certs for which there's a private key, regardless of their expiration date. The new UI you have developed allows the user to select the most recent one. Let's make sure other people comment. Kai, let me know if this is a very small fix, in which case I can make the case to PDT.
Ok, so we have to display all the certs, as only the server can decide whether it will accept the cert or not (of course, we are filtering the list of our certs to include only those that match the CA). Now that I know what we want, it is indeed a very small fix. I will attach a patch. (Info: The current code does not verify whether there is a private key for all the displayed certs, this check is delay until after the user has selected one, but I guess that is ok.)
r=ddrinan.
Kai wrote: >Suggested new code in manual prompt mode >======================================== >Find all certs for all names, allowing duplicates. >But find only valid certs. >User decides which cert to use. Change the first line to "Find all certs appropriate for SSL client-auth for all names, allowing duplicates", and I agree. Is that what your patch does? I think so. Christina wrote: > sometimes you need to use an expired cert to renew itself. I'm of the opinion that this isn't a good idea. First, expired should mean expired, not expired for some purposes. Second, renewal should take place via some protocol like CRMF/CMMF or CMC, and ideally it should happen automatically with a minimum of user action. It should happen during the "renewal window", and the CA administrator should make sure that window is large enough for his/her user base. We shouldn't promote SSL client-auth as a good renewal mechanism. Stephane said: >Therefore my opinion is that the code should display all the certs for which >there's a private key, regardless of their expiration date. I think you meant "all certs appropriate for SSL client-auth...". Over time I'd like expired certs to become inaccessable from SSL client-auth (visible but grayed out, perhaps?), but this isn't the time to make that change.
putting it back on the PDT radar. The earlier fix checked in on 8/13/2001 was incomplete. The additional patch is necessary and is low impact. cc relyea for formal NSS team member review.
Whiteboard: PDT
Target Milestone: 2.2 → 2.1
Lord wrote: > Change the first line to "Find all certs appropriate for SSL client-auth for all > names, allowing duplicates", and I agree. Is that what your patch does? I > think so. Yes. The code is asking the database for certs with usage SSLClientCert only. > Christina wrote: > > sometimes you need to use an expired cert to renew itself. > I'm of the opinion that this isn't a good idea. First, expired should mean > expired, not expired for some purposes. Second, renewal should take place via > some protocol like CRMF/CMMF or CMC, and ideally it should happen automatically > with a minimum of user action. It should happen during the "renewal window", and > the CA administrator should make sure that window is large enough for his/her > user base. We shouldn't promote SSL client-auth as a good renewal mechanism. I can't tell which one is better, both approaches seem ok to me. If nobody objects, we can use lord's approchach, which just results in changing another parameter from "false" => "true" in the patch. > I think you meant "all certs appropriate for SSL client-auth...". Yes.
I discussed with lord, and he agreed to leave this proposed change out for now. Note to superreviewers: The previous patch is still what we want.
Waiting for Bob Relyea's comments. Since we may need additional design for this, I'm changing the targe to future.
Keywords: nsenterprise
Whiteboard: PDT
Target Milestone: 2.1 → Future
I just want to add that it seems currently revoked certs are also presented on the cert selection list (yes, a valid CRL has been imported). CRL should be checked for this matter.
cc rangansen. Rangan, check Christina's last comments about CRLs
Comment on attachment 49847 [details] [diff] [review] Additional patch to display all certificates, allowing duplicates. sr=blizzard
Attachment #49847 - Flags: superreview+
Checked in to trunk, closing bug.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
QA Contact: ckritzer → junruh
Verified on 10/12 Trunk Win98.
Keywords: nsbranch
Verified.
Status: RESOLVED → VERIFIED
Changing target to PSM 2.1
Target Milestone: Future → 2.1
kai, I don't think this was checked in onto the branch. If it wasn't then it's not fixed in 2.1 but will in 2.2, which would then be the correct target.
Target Milestone: 2.1 → 2.2
Product: PSM → Core
Version: psm2.0 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: