87361 - Incorrect error during "Backup All"

Status: VERIFIED FIXED

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

Description

[Terry Hayes]

The "Backup All" function in the certificate manager generates an error which
reads "Failed to restore the PKCS#12 file for unknown reasons".  This is an
inappropriate error message for a backup operation.

I get this error when I attempt to do a "Backup All" when the only certificates
I have are on an external token such as an iButton.  The operation appears to
select all available certificates (there are 2 on the iButton), requests the
password for the built in database, and requests a password for the new PKCS 12
file.  At that point it fails with the incorrect message.

Comment 1

[Bob Lord]

Target 2.1

Comment 2

[Stephane Saux]

P1

Comment 3

[(Don't use, old account) - Kai Engert]

I can't test because I don't have crypto a token, but I think you see the
message for the following reasons:

- nsPKCS12Blob::ExportToFile makes no attempt to backup the certificates from
your crypto token
- the loop iterating over all selected certificates does not find anything to
back up.
- the code trying to write the backup file does not succeed, because the file
would be empty
- the code falls through to a default error message, which is currently the one
you see

I'm suggesting the following fix:
- Use a different default message for backup and restore situations
- If the backup loop detects that a certificate from a crypto hardware is
selected, it shows an informational display to user, informing that it's not
possible to do that.
- If only certificates from crypo tokens were selected, do not make an attempt
to create the backup file
I'll attach a fix for that.

Comment 4

[(Don't use, old account) - Kai Engert]

Attachment: Suggested fix

Comment 5

[(Don't use, old account) - Kai Engert]

Sean, can you please review the language?

David, can you please review the patch?

Who could test this patch, as I don't have the required hardware?

Comment 6

[Sean Cotter]

Not sure if I understand this completely. Currently help says that you can't
back up smart card certs with the Backup All button, but you can back them up
one at a time. Is that not true? In other words, is the problem that you can't
back up hardware tokens at all, or only that you can't back them up with Backup All?

>PKCS12UnknownErrBackup=Failed to backup the PKCS#12 file for unknown reasons.

"backup" in the above should be two words: "back up"

For this next one, my suggestions are valid only if it's true that you can't
back up hardware tokens at all. If in fact you can back them up one at a time,
then we need further changes (in help as well as the alert).

>+PKCS12InfoNoSmartcardBackup=Please note: It is not possible to backup
certificates from a hardware security token.

"Please note" seems overly polite for an alert
"token" is "security device" elsewhere in the interface. And even "hardware
security device" is probably mystifying to most users.

Better:
+PKCS12InfoNoSmartcardBackup=It is not possible to back up certificates from a
hardware security device such as a smart card.

Comment 7

[(Don't use, old account) - Kai Engert]

Sean, thanks for your wording suggestions, I will create an updated patch.

The current implementation is indeed different from what you found in the help
text. My understanding is: You can not backup certificates from any smart card.

The underlying implementation (contained in nsPKCS12Blob::ExportToFile) receives
a list of all the certificates that were selected. It's called no matter if one
or multiples certificates were selected (backup all = all certificates are
selected).

Arriving at the code that does the backup, for each certificate a check is made
whether it is on the internal token or not. If it is not, it get's silently
ignored.

The code I'm referencing to is:

    // We can only successfully export certs that are on 
    // internal token.  Most, if not all, smart card vendors
    // won't let you extract the private key (in any way
    // shape or form) from the card.  So let's punt if 
    // the cert is not in the internal db.
    if (nssCert->slot && !PK11_IsInternal(nssCert->slot)) {
      CERT_DestroyCertificate(nssCert);
      continue;
    }

CC'ing David and Javier. Would you like to verify that I'm telling the truth
with above statements?

Comment 8

[Javier Delgadillo]

kaie is right.  We silently ignore any certs that were selected for back-up and
are on smart cards.

Comment 9

[(Don't use, old account) - Kai Engert]

As discussed with Sean, we are changing the text to "Failed to create the PKCS
#12 backup file for unknown reasons.".

In addition, changing text from "PKCS#12" to "PKCS #12" all over PSM was
suggested, both in the new text we add and in the existing text. This will make
PSM consistent, because all other strings in Mozilla use the version with space.

A seperate bug would have made most sense, but the only affected file is the one
we are fixing with this bug. To avoid clashes of different patches I'm fixing
this minor issue within the current bug.

Comment 10

[(Don't use, old account) - Kai Engert]

Attachment: Updated patch

Comment 11

[(Don't use, old account) - Kai Engert]

Javi, can you please review?

Comment 12

[Javier Delgadillo]

The only potential issue with this is the popping up of an alert while trying to
back up (iff one of the selected certs is on a smart card).  Ot adds another
click to the process.  May hamper usability slightly, but I think the solution
is OK.

r=javi

Comment 13

[Stephane Saux]

add patch, review keywords

Comment 14

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 15

[John Unruh]

Mass assigning QA to ckritzer.

Comment 16

[Javier Delgadillo]

Checked in patch for Kai.

Comment 17

[Stephane Saux]

keywords->verifyme

Comment 18

[ckritzer (gone)]

This is blocked by 95873.

Comment 19

[Stephane Saux]

Using 09/19/2001 Win 0.9.4 branch build, I verified that the correct message
appears.
Note 95873 is solved.

Sean, has the help issue been resolved?
When you click on backup All and some of your certs are on a hardware token, you
first get an alert that says the you cannot backup certs that are on a hardware
token, then a info dialog telling you that your certs have been backed up.
That's because all your internal token certs WILL be backed up.

Although the bug should be marked as verified as the approved patch is indeed
there, I'm wondering whether the language should say something like:

"can't backup cets on hardware token, so only the certs in the software token
will be backed up."

I'm also going to try to hit backup all when all the certs are on a hardware token.

Comment 20

[Stephane Saux]

Works fine when all the certs are on a hardware token. The behavior is that you
get the first warning, and no success dialog. No file is created.

Comment 21

[Sean Cotter]

The help issue has been resolved.

I agree, based on your description, that the alert message could be improved. It
would need a bit of investigation to determine exactly what it should say.
Stephane, do you want to open a new bug about that issue, for the 2.2 timeframe?

Comment 22

[Stephane Saux]

created 100820 as per Sean's suggestion.

Comment 23

[John Unruh]

Verified with an iButton on Win98 9/21 build.