110062 - PKCS#11 CKF_PROTECTED_AUTHENTICATION_PATH token flag not supported

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Petr Kostka]

NSS always asks user for a PIN to authorize to the token, even though the 
CKF_PROTECTED_AUTHENTICATION_PATH flag was specified in token's CK_TOKEN_INFO 
structure. 

Correct behaviour would be to check for this flag. If it is set, a modeless 
message like "Please authenticate to your token. Authentication method depends 
on the type of your token and/or slot." or some thing like this should pop up 
before (and during) a call to module's C_Login() (with pPin = NULL_PTR). 

Current implementation is really annoying for use with PKCS #11 libraries 
written for biometric devices or PIN-less cards.

Comment 1

[Wan-Teh Chang]

Assigned the bug to Bob.

Comment 2

[Robert Relyea]

Actually this is a combo PSM/NSS bug. PSM controls all the UI, and should have
enough information to say that this is a protected pin path token. NSS needs to
call C_Login with a '0' when it returns, then make a second call to tell PSM to
popdown the dialog.

I'm not sure there is a way to make all this work inside mozilla (that's always
been the problem in Communicator).

<rant>
Anyway this is a long standing bug that needs some UI developer at the next
level up to help solve. (It's been about 4 years now that we've known about
protected pin path but haven't been able to use it effectively:().
</rant>

bob

Comment 3

[Petr Kostka]

Attachment: Initial patch proposal (without new files)

I tried to fix this bug. My comments follow:

Cooperation between NSS and PSM (providing GUI) is similar to that of getting a
password, i.e. via a callback to 

PSM.

A new GUI callback to PSM was added: PK11ProtectedAuth(PK11SlotInfo* slot,
void* arg) and is initialized in NSS from 

manager nsNSSComponent::InitializeNSS) by calling new NSS public method
PK11_SetProtectedAuthFunc(). If this call is 

not done before calling PK11_Authenticate(), no GUI callback is called and we
simply call pk11_CheckPassword() with 

NULL password. 

If the callback is set, NSS calls it from PK11_Authenticate. I creates a modal
dialog that creates new thread that 

calls PK11_CheckUserPassword(NULL). The dialog cannot be cancelled as C_Login()
is uninterruptible from code. This 

GUI stuff was written using KeygenHandler as a template.

I had to extend nsITokenDialogs interface instead of adding a new one because
XPCOM impl macros support only up to 

10 interfaces and I really did not want to touch XPCOM stuff because of this
limitation.

New files:
./security/manager/ssl/public/nsIProtectedAuthThread.idl
./security/manager/ssl/src/nsProtectedAuthThread.cpp
./security/manager/ssl/src/nsProtectedAuthThread.h
./security/manager/pki/resources/content/protectedAuth.js
./security/managet/pki/resources/content/protectedAuth.xul
I was not able to generate cvs diff for them so I generated common diff file.

I admit it is a pretty big fix and it will require thorough review.

Petr

Comment 4

[Petr Kostka]

Attachment: Five new files (not diffed via CVS)

Comment 5

[Robert Relyea]

Thanks Petr!

I do have a couple of questions.... how do you make the dialog go away after
login? I don't see any NSS callbacks that cause that to happen.

On the NSS side, I prefer simply using the existing callbacks and adjusting our
calls to C_Login accordingly. Attached are my diffs, though like yours they are
missing the callback to popdown the popup after C_Login returns.

bob

Comment 6

[Robert Relyea]

Attachment: NSS protected pin path changes.

Comment 7

[Wan-Teh Chang]

Petr,

Since this is a combo PSM/NSS bug and the fix seems to
require more PSM changes than NSS changes, we should
either convert this into a PSM bug or open a PSM bug
for the PSM portion of the bug.

Comment 8

[Petr Kostka]

Bob,

there are two ways PK11_Authenticate() function works (after my change):

1) The callback WAS initialized in NSS: callback to PSM is called. The 
PK11ProtectedAuth() PSM callback creates a modal uninterruptible window that 
creates new thread from JavaScript window.OnLoad(). Then the callback function 
joins the new thread and waits until the window is closed. The thread (see 
nsProtectedAuthThread.cpp) calls PK11_CheckUserPassword() with NULL password 
which actually calls C_Login(). When this is done, it closes the window and 
ends itself (the thread). Now callback function is resumed as the thread ends 
and returns to NSS.

So it is NOT neccessary to have two callbacks, one for popping up the dialog 
and the second closing it. The PSM code is very similar to the one that 
handles key generation dialog.

2) The callback WAS NOT initialized in NSS: then pk11_CheckPassword() with 
NULL password is performed. No callback to PSM.

The advantage of this implementation is that it is up to PSM (or any other NSS 
client code) to initialize the callback. If it is not initialized it just 
calls C_Login() with null password without any GUI stuff. If the client wants 
to handle this, it can.

Your proposed change is perfectly OK with me. But when I proposed this little 
change to you in Hamburg this June, you said there must be some GUI notifying 
the user that he has to authenticate to the token. ANd you were right. If it 
is the case of token with certificates on private part, than during say 
openning of the PSM, it would just hang up waiting for the C_Login() 
completition and the user would not know what is going on.

Comment 9

[Petr Kostka]

Wan-Teh,

it is OK with me if we convert this bug into a PSM bug. But I would rather not 
divide it into two bugs as the changes on the NSS side are pretty small and 
have to be coordinated with changes on PSM side.

Comment 10

[Robert Relyea]

OK, I still prefer not to have a new callback type, but I like your idea of
calling PK11_CheckUserPassword() from inside the callback itself. That keeps any
create thread calls out of NSS proper.

Having the UI code is a great help! There shouldn't be a problem splitting this
bug and coordinating. Bugzilla has tools for that, and most of the PSM team sits
withing 30 feet of me;). Putting a separate bug allows them to manage who is
supposed to work with me on this (and puts it on their radar).

bob

Comment 11

[Petr Kostka]

I see some few problems with avoiding the callback. 

1) Instead of simply calling PK11_Authenticate() you always have to check 
whether given token has protected authentication and behave accordingly, i.e. 
either pop up some GUI and call PK11_CheckUserPassword() or PK11_Authenticate
() from a side thread or call PK11_Authenticate() directly.

2) I found almost 50 occurences of PK11_Authenticate() at various places in 
nss, psm and jss. I do not believe somebody is going to change all those 
places to behave correctly with tokens that have 
CKF_PROTECTED_AUTHENTICATION_PATH flag. And there must be lots of these calls 
in non-mozilla projects using NSS. By providing the clients with possibility 
to set up a single callback, writters of those apps have life much easier.

3) Unfortunatelly PK11_Authenticate()'s second parameter is loadCerts which 
prevents us from calling PK11_CheckUserPassword() on client side as 
replacement to PK11_Authenticate() unless PK11_CheckUserPassword() is called 
from the callback initiated by PK11_Authenticate(). But this can be solved by 
changing current PK11_Authentication()'s implementation by not calling 
PK11_DoPassword() in every case but only when needed and by moving certificate 
loading code from PK11_DoPassword() to PK11_Authenticate().

Well, I presented my arguments but the decision is yours. And I must admit I 
do not like such callbacks at all :) (GetPassword's as well). But what can we 
do? Maybe this can be solved more nicely in NSS 4 :)

And just a little last comment: I have seen a few of PORT_Strlen(pPassword) 
calls in pk11slot.c without checking for null pointer even in public NSS 
functions. PORT_Strlen() crashes on those occasions (at least on my Wintel 
machine) ;(

So, please think about it a would you manage the splitting of the bug into 
PSM/NSS parts so that I have somebody to communicate about the GUI stuff with. 
Thanx.

Petr

Comment 12

[Robert Relyea]

I think you misunderstood. I didn't want to add a *new* callback. The plan is
simply use the old one, Applications need to call
PK11_ProtectedAuthenticationPath(slot) to determine what it needs to do. The
application still calls PK11_CheckUserPass. It would return something a defined
value to indicated 'C_Login' has already been called.

Applications that don't check PK11_ProtectedAuthenticationPath() will still put
up UI and collect a Password which will be ignored. There is still an issue with
applications that try to do unattended restarts (like servers), but I think
unattended restart and ProtectedAuthenticationPath are mutually exclusive anyway.

bob.

Comment 13

[Wan-Teh Chang]

Bob, could you set a target milestone for this bug?

Comment 14

[Robert Relyea]

We need a PSM version of this bug as well.

bob

Comment 15

[Wan-Teh Chang]

Bob, what is the status of this bug?

Comment 16

[Wan-Teh Chang]

Set priority to P1 so that it shows up on Bob's radar screen.

Comment 17

[Robert Relyea]

OK, the NSS changes I described above have been checked in for a while and will
show up when NSS 3.4 lands. Kaie, we need to talk about the changes to the PSM
patch to use the NSS patches I made. They should be relatively minor (most of
Petr's work is in getting the UI right).

I'll close this wen NSS 3.4 lands.

bob

Comment 18

[Wan-Teh Chang]

NSS 3.4 has landed.  Lowered the priority to P2 for
the remaining PSM work.

Bob, I'll let you decide whether you want to close
this bug and open a new PSM bug or simply change
the product to PSM.

Comment 19

[Kai Engert [:KaiE:]]

I think I'm ready to talk. I've read this bug, and understand most of it, but I
don't yet get the idea what changes have to be made to Petr's UI code.

Let's talk about it in bug 119500, or find me on IRC.

Comment 20

[Wan-Teh Chang]

Set priority back to P1.

Bob, you thought you checked in your patch, but
you haven't :-)

Also, you need to export the new function from nss.def.

Comment 21

[Robert Relyea]

Attachment: Rebuild patch against the currrent tree.

Sigh, I thought I had found the changes in the top of tree, but for the life of
me I can't see them now. I've reapplied the patch and checking it in now.

Comment 22

[Wan-Teh Chang]

The patch has been checked into the tip and the NSS_CLIENT_TAG
of NSS.  Marked the bug fixed.