PK11SDR_Encrypt fails if not logged into token

RESOLVED FIXED in 3.4

Status

P1
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: jamie-bugzilla, Assigned: jamie-bugzilla)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Blocks: 112882
Target Milestone: --- → 3.4
(Assignee)

Comment 1

17 years ago
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

17 years ago
Jamie, does NSS 3.3.x have this problem too or is this bug
introduced in NSS 3.4?
(Assignee)

Comment 3

17 years ago
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

17 years ago
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
(Assignee)

Comment 5

17 years ago
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

17 years ago
Set priority to P1 for NSS 3.4.
Priority: -- → P1
(Assignee)

Comment 7

17 years ago
Created attachment 65335 [details] [diff] [review]
patch to login to the token before looking for the key

Comment 8

17 years ago
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

17 years ago
I'm going to close this as invalid, as per my reasons in comment 8.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → INVALID
(Assignee)

Comment 10

17 years ago
Hang on, I'll check in my patch.

Initializing the database before calling this function is necessary and is the
application's problem.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Comment 11

17 years ago
Reassigning to myself.
Assignee: relyea → nicolson
Status: REOPENED → NEW
(Assignee)

Comment 12

17 years ago
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
Status: NEW → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 13

17 years ago
OK, go ahead.

bob
You need to log in before you can comment on or make changes to this bug.