109770 - Obtaining a certificate from Verisign does not work

Status: VERIFIED FIXED

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

Description

[Kai Engert [:KaiE:]]

I tested with version 094 and with the current trunk. Version 094 works as
expected. The current trunk does not.

To reproduce:

- Go to https://digitalid.verisign.com/client/class1Netscape.htm
- Apply for a test certificate
  (hint: if you already have a test certificate, use a different first/last
name, append a number for example)
  Fill first, last name, e-mail address, challenge phrase, select test-drive,
submit.
- wait for the e-mail from Verisign to arrive, this can take a while.
- make sure you still use the same profile and browser version you used in the
initial steps
- as instructed by the e-mail go to
https://digitalid.verisign.com/enrollment/nspickup.htm and paste your digital ID
pin you received in the mail
- click submit
- you will be presented a page that informs you "the certificate has been
installed" (or similar text)
- go to certificate manager.

Expected result: A new certificate from Verisign is listed
Actual result: No new certificate has been received

As this works with 094, it shouldn't be a server side problem.

Could somebody please test to confirm what I see?Thanks.

Comment 1

[Kai Engert [:KaiE:]]

I'd like to find out at which point the problem starts. Either storing the
generated private key does not work, or downloading the certificate does not work.

I tried to use "certutil -K -k all" from NSS to list all keys in the certificate
database, but that seems to work only for keys already having a certificate.

If nobody has an idea, I'll have to debug what goes wrong.

Comment 2

[Stephane Saux]

I was able to get a cert from testca.netscape.com
This may have to do with keygen.

Comment 3

[Kai Engert [:KaiE:]]

Using testca works for me, too. Something must be different between verisign and
testca.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

The reason, why it didn't work for me, was influenced by the time Verisign
needs to issue the certifiate. I had stopped my browser session and restarted.
When I tried to download my cert, I had not yet logged in to the token. It is
password protected.

So, if you want to reproduce my initial problem, quit the browser completely,
and try to download the cert without having logged in to the token.

During import, NSS function PK11_KeyForCertExists is called. It iterates over
tokens, and calls PK11_Authenticate for each.
It is expected that this call prompts the user to log in to the token. However,
this does not happen. No password prompt is shown.
This problem is always reproducable on my machine. I can fix it by logging in
to the token first.

PK11PasswordPrompt expects the context object to be an nsIInterfaceRequestor,
and tries to QueryInterface nsIPrompt on it.
However, that does not succeed.

Solution
========
The bug is in method PSMContentDownloaderContext::GetInterface, which returns
an undefined result code in the success case.
I'll attach a simple patch which fixes the problem for me.

Question
========
While my patch fixes the problem, I wonder why there are two classes in PSM
that do exactly the same. Besides from this bug, I couldn't find any difference
between classes PSMContentDownloaderContext and PipUIContext. Should I create a
patch to remove one class and reduce code bloat?

Comment 5

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Alternative fix

Javi, do you think we should prefer this fix? It removes class
PSMContentDownloaderContext which I think is not necessary.

Comment 7

[Javier Delgadillo]

Comment on attachment 57510 [details] [diff] [review]
Suggested fix

r=javi

Comment 8

[Kai Engert [:KaiE:]]

Javi, before I go and request sr=, what do you think about the alternative patch?

Comment 9

[Javier Delgadillo]

Comment on attachment 57512 [details] [diff] [review]
Alternative fix

r=javi

Comment 10

[Javier Delgadillo]

Comment on attachment 57510 [details] [diff] [review]
Suggested fix

marking patch as obsolete

Comment 11

[Christopher Blizzard (:blizzard)]

Comment on attachment 57512 [details] [diff] [review]
Alternative fix

sr=blizzard

Comment 12

[Kai Engert [:KaiE:]]

checked in => fixed

Comment 13

[John Unruh]

Verified fixed.