118679 - PK11SDR_Encrypt fails if not logged into token

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jamie Nicolson]

An empty keyid is passed in, meaning the default keyid should be used. PK11SDR_Encrypt calls PK11_FindFixedKey to lookup the default key. If we are not logged into the token, this call will not find the key, but it won't fail either--it will silently succeed. Thinking the key does not exist yet, PK11SDR_Encrypt then tries to generate the key. When it goes to store the new key in the database, it conflicts with the existing key and the whole thing fails.

It seems like we should call PK11_Authenticate before calling PK11_FindFixedKey.

Comment 1

[Jamie Nicolson]

REPOST OF ORIGINAL COMMENTS (hopefully with appropriate linebreaks?):

An empty keyid is passed in, meaning the default keyid should be used.
PK11SDR_Encrypt calls PK11_FindFixedKey to lookup the default key. If we are not
logged into the token, this call will not find the key, but it won't fail
either--it will silently succeed. Thinking the key does not exist yet,
PK11SDR_Encrypt then tries to generate the key. When it goes to store the new
key in the database, it conflicts with the existing key and the whole thing fails.

It seems like we should call PK11_Authenticate before calling PK11_FindFixedKey.

Comment 2

[Wan-Teh Chang]

Jamie, does NSS 3.3.x have this problem too or is this bug
introduced in NSS 3.4?

Comment 3

[Jamie Nicolson]

I assume it has always been there, and was never before encountered because the
SDR  functions had never been called before the token was logged in.

Comment 4

[Robert Relyea]

In fact the test program explicitly logs in.

This is actually a feature enhancement -- except for the fact it should probably
return an error instead of silently failing. (This is always the problem with
search interfaces -- if the object isn't found, do you return an error [maybe
it's not an error if the object isn't there] -- but what if the object wasn't
found because of a different error, and how can you tell.

bob

Comment 5

[Jamie Nicolson]

This is a bug in PK11_SDREncrypt. It should call PK11_Authenticate before
PK11_FindFixedKey. I agree there may be times when FindFixedKey should silently
fail if the token is not logged in, but this isn't one of those times. That's
why the bug is in SDREncrypt and not FindFixedKey. If you are using the
SecretDecoderRing you always want to be logged in. In fact SDREncrypt DOES force
a login when it generates the key, just not when it looks for it. This
inconsistency is what caused the behavior described above.

Comment 6

[Wan-Teh Chang]

Set priority to P1 for NSS 3.4.

Comment 7

[Jamie Nicolson]

Attachment: patch to login to the token before looking for the key

Comment 8

[Robert Relyea]

OK, I know why this is an issue.

The application that calls PK11SDR_Encrypt needs to do more. It needs to make
sure the internal token has been initialized with a password. The exact code to
do this is application dependent. Here's the version from sdrtest.c

      /* sigh, initialize the key database */
      slot = PK11_GetInternalKeySlot();
      if (slot && PK11_NeedUserInit(slot)) {
        rv = SECU_ChangePW(slot, "", 0);
        if (rv != SECSuccess) {
            SECU_PrintError(program_name, "Failed to initialize slot \"%s\"",
                                    PK11_GetSlotName(slot));
            return SECFailure;
        }
        PK11_FreeSlot(slot);
      }

This code can't go into PK11SDR_Encrypt because it needs to prompt for the
password (in the normal case) before the SECU_ChangePW() call. It's the same
code you need to run before doing a keygen.

Adding the PK11_Authenticate() doesn't hurt... and if you know you are dealing
with an initialized database (somehow) already, then go ahead and check in your
patch, but be sure to test this function as the first operation on a new profile
to make sure you have the proper initialization checks in.

bob

Comment 9

[Robert Relyea]

I'm going to close this as invalid, as per my reasons in comment 8.

Comment 10

[Jamie Nicolson]

Hang on, I'll check in my patch.

Initializing the database before calling this function is necessary and is the
application's problem.

Comment 11

[Jamie Nicolson]

Reassigning to myself.

Comment 12

[Jamie Nicolson]

Fixed on trunk for NSS 3.4.

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11sdr.c,v  <--  pk11sdr.c
new revision: 1.8; previous revision: 1.7

Comment 13

[Robert Relyea]

OK, go ahead.

bob