119500 - PKCS#11 CKF_PROTECTED_AUTHENTICATION_PATH token flag not supported

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Stephane Saux]

PSM conterpart to bug 110062

Comment 1

[Stephane Saux]

add dependency.

Comment 2

[Stephane Saux]

Changes to PSM are in a patch published to 110062.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Patch created from code in 110062

This is the PSM portion of the code from bug 110062. I didn't do anything else
but fixing the conflicts, make it compile, and fixed a compiler warning.

Comment 4

[Kai Engert (:KaiE:)]

Trying to understand what this is all about, I found the following explanation:

>>CKF_PROTECTED_AUTHENTICATION_PATH
If it is set, that means that there is some way for the user to login without
sending a PIN through the Cryptoki library itself.<<

Trying to summarize, I think that means:
- our PK11 routines must give feedback to the user.
- NSS triggers a callback, and we display a message on screen, that the user
should do the login.
- NSS internally blocks on some code in the PKCS#11 library, and waits for the
login even to complete.
- As soon as the login completes, or an error is triggered, we must quit that
information box.
- As control must reside inside NSS/PKCS#11, the information must be shown
modeless, and that is why you created a separate thread.

Comment 5

[Petr Kostka]

Hi, I am sorry I was not able to cooperate on this bug for last 3 months 
because I was assigned to a time critical project by my management. I think I 
will return to PKI stuff and thus work on this within next one or two weeks.

Your understanding of my patch is right but I think Bob went another way 
without a callback to PSM. I have not seen his solution yet. If it is as he 
proposed earlier, than we will have to check for the 
CKF_PROTECTED_AUTHENTICATION_PATH flag BEFORE calling authentication stuff of 
NSS. If set, modeless dialog with message like "Do your protected logon now" 
must be shown before calling the NSS logon function and hiden after the logon 
function returns.

Petr

Comment 6

[Robert Relyea]

Peter's correct. My basic change to his code was not to add a new callback, but
provide enough information with the old call back so you know that you are doing
protected pin path authentication. The NSS changes was to export the function
you call to complete the autentication, and to correct NSS to not try to pass a
password down to the token in the protected pin path case.

I'll try to catch you guys on IRC just now.

bob

Comment 7

[Petr Kostka]

Bob, are your changes visible somewhere already? I could not find it using 
LXR. Could you point me to the right source code? I thought it would be in 
pk11wrap/pk11slot.c.

Unfortunatelly I am behind firewall that allows me only http, so no CVS, no 
IRC :(

Comment 8

[Petr Kostka]

Attachment: CVS patch on existing files

Comment 9

[Petr Kostka]

Attachment: Non-CVS patch containing new files

Finally I managed to modify my gui stuff for handling Protected Auth Path
login. Now the PK11PasswordPrompt() in case of PAP does not pop up dialog for
collecting password. Instead it shows a modal uncancellable (PKCS #11 login
operation is not cancellable) dialog and calls PK11_CheckUserPassword() with
null password string from dialogs thread. That makes the device to do its own
login. Then PK11PasswordPrompt() returns with special values (-1, -2, -3)
casted to char *. I believe these are not usable pointers on any system and
they map to SECSuccess, SECWouldBlock and SECFailure. PK11_DoPassword() than
knows how to handle these return values properly.

During my tests I also discovered following bug in pki3hack.c in function
nssToken_UpdateTrustForCerts() leading to crash in PK11_DoPassword(). In case
of non internal and non HW slot nssToken_LoadCerts() called in
PK11_DoPassword() leaves the slot->nssToken->certList as null pointer.
nssToken_UpdateTrustForCerts() which is called immediatelly after
nssToken_LoadCerts() does not check the pointer and tries to dereference it. I
bundled the fix with my PAP patch files.

I created two patch files: 119500cvs for existing files created via cvs and
119500newfiles for new files as I cannot check in new files to cvs.

The changes were tested on Wintel only, with slot/tokens both requiring and not
supporting PAP.

Comment 10

[R.Roesel]

Is there anybody working on this bug ? 

Comment 11

[John G. Myers]

*** Bug 206844 has been marked as a duplicate of this bug. ***

Comment 12

[Priit Randla]

Well, I see last activity has been over two years ago. Petr Kostka made patch
for the problem, 
but, as I'm currently using such a token, I can only say it seemingly didn't
made it into official tree? Currently Mozilla 1.7.3 doesn't say anything about
pinpad, asks for pin, ignores it and then blocks and allows the user to enter
his/her pin using pinpad. It creates a lot of confusion and even I myself tend
to forget the pinpad, enter the pin and stare the screen in confusion until my
cardreader with pinpad timeouts. 
So I'd like to ask if some knowledgeable person would please do 'something'
about it? Those pinpad equipped readers aren't exotic anymore...

Comment 13

[Robert Relyea]

Sigh, The problem is there has been close to zero real activity on PSM (the
portion of mozilla the interfaces with the security library) in almost 2 years.
I had thought Petr's changes were already checked into Mozilla, but obviously it
hasn't.

The good news is there are rumors we may have some PSM resources in the near future.

bob

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 282437 has been marked as a duplicate of this bug. ***

Comment 15

[Priit Randla]

And another year has gone... It seems life is too short to wait for a fix; I
have to start digging Mozilla's codebase to do something about it... 

Comment 16

[Wan-Teh Chang]

Thank you. I look forward to your contribution.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 78905 [details] [diff] [review]
CVS patch on existing files

This bug doesn't need sppear to need someone to start over and write a new 
patch from scratch.  There's a 3.5 year old patch attached to this bug.  
It needs a review, not more coding. (Maybe a negative review will establish
that it needs more coding, but that review hasn't happened in3.5 years).

Bob or Kai, will you review these patches?

Comment 18

[Robert Relyea]

Comment on attachment 78905 [details] [diff] [review]
CVS patch on existing files

Unless the PSM authentication stuff has changed significantly, this patch is
99% there.

The main problem is casting a set of ints (which are really SECStatus values)
to char * to return them from the password callback.

There are a couple of ways to fix this:
1) move the looping logic up to User space. We pass a special string value to
tell NSS that the token is already authenticated (No need to call
checkPassword). This special string value is only checked for protectedAuthPath
tokens.

2) keep the loop in NSS and have 2 special string values passed from the
password handler: one for retry the other for 'already authenticated'.

The mapping to the existing code is:
SECFailure -> pw = NULL
SECWouldBlock -> pw = "Retry"
SECSuccess -> pw = "auth"

The second part of the NSS patch is a minor nit. NSS already handles the
protectedAuthPatch case by setting pw and len to NULL. The code to set len only
if pw set is a bit of paranoia which is good for protectedAuth apps, but not
good for non-protectedAuth tokens (it would theoretically be possible to to
crash int the PKCS #11 module if we pass a NULL password to a non-protectedAuth
token. Probably something like:

    if (slot->protectedAuthPath) {
	len = 0;
	pw = NULL;
    } else if (pw == NULL) {
	PORT_SetError(SEC_ERROR_INVALID_ARGS);
       return SECFailure;
   } else {
       len = PORT_Strlen(pw);
   }

Comment 19

[Robert Relyea]

Attachment: NSS changes for PROTECTED_AUTH_PATH

This patch addresses the NSS changes needed to implement this patch. I used the
second method described in my comments since it has a lower impact on the PSM
portion of the patch to interoperate.

This patch also allows NULL passwords to be passed to the authentication
functions if the token is a protected pin path token, as well as allowing a
'dummy' string. It does check if applications pass a NULL pointer to a
non-protected pin path device. This semantic will allow existing the ability to
limp along with protected pin path, while accepting the current proposed PSM
patch without requiring a 'dummy' password to these function.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196941 [details] [diff] [review]
NSS changes for PROTECTED_AUTH_PATH

I think the code is right, but I found a number of confusing things about the
comments and the newly defined symbols.  I was able to finally figure it out,
but I think the comments need to make it clearer for the next guy.

>+#define PK11_PW_TRY		""      /* Default: a prompt has been presented
>+					 * to the user, initiate a C_Login
>+					 * to authenticate the token */

This value is never used anywhere (in NSS).  So, I suggest that rather 
than defining it, you simply use a comment that says "any other value
(than AUTH or RETRY) will cause NSS to ignore it and call C_Login,
and retry (or not) based on the outcome of that login attempt."


>@@ -545,7 +560,26 @@
>      *	(3) the password was successful.
>      */
>     while ((password = pk11_GetPassword(slot, attempt, wincx)) != NULL) {
>+	/* if the token has a protectedAuthPath, the application may have
>+         * already issued the C_Login.

I interpret that statement to mean that the application may have called C_Login
before this while loop begins, before the function that holds this while loop
was called.  I suspect you don't mean that.  I think you mean to say something
like
"When pk11_GetPassword calls the application's password callback function, some 
applications will call C_Login to attempt to manage their own authentication."  

>+         * already issued the C_Login. In this the application will tell
						^ case ??
>+	 * us what the results were in the password value (retry or the
>+         * authentication was successful). 

I was confused by that last sentence.  I think you meant something like this: 
"In that case, we need the application to tell us whether to stop here because 
the login succeeded, or to call it again (without calling C_Login ourselves)
so that it can retry the failed authentication. If the application tells us 
neither, we will call C_Login as usual, and retry (or not) based on that
outcome.

	      Applications that don't know about
>+         * protectedPinAuth will return a password, which we will ignore and
>+         * trigger the token to 'authenticate' itself anyway. Hopefully the
>+	 * blinking display on the reader, or the flashing light under the
>+         * thumbprint reader will attract the user's attention */

Comment 21

[Robert Relyea]

Attachment: Incorporate nelson's comments

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197735 [details] [diff] [review]
Incorporate nelson's comments




>+#define PK11_PW_RETRY		"RETRY"	/* an failed attempt to authenticate
>+					 * has already been made, just retry
>+					 * the operation */
>+#define PK11_PW_AUTHENTICATED	"AUTH"  /* a successful attempt to authenticate
>+					 * has completed. Continue without
>+					 * another call to C_Login */
>+/* any value works here, this value is to applications can easily document

"any value works here"?  what does that mean?  Is "here" AUTHENTICATED or
is "here" TRY?	And what does "works" mean?  These comments say the application
should be able to easily document what it's trying to do, but don't tell the
application programmer anything about how "any value" is interepreted by NSS
and what NSS does with it.  

>+ * what it's trying to do */
>+#define PK11_PW_TRY		""      /* Default: a prompt has been presented
>+					 * to the user, initiate a C_Login
>+					 * to authenticate the token */
>+

I suggested: a comment that says "any other value (than AUTH or RETRY) will 
cause NSS to ignore it and call C_Login, and retry (or not) based on the 
outcome of that login attempt."

Comment 23

[Robert Relyea]

Attachment: Tweek the comment once again

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 197870 [details] [diff] [review]
Tweek the comment once again

I think this comment still leaves a lot of questions unanswered, but it's
better than before, and I'm not going to hold up the patch (whose logic is
correct) over this comment any more.

Comment 25

[Robert Relyea]

Comment on attachment 197870 [details] [diff] [review]
Tweek the comment once again

This is targetted for 3.10.2, so it needs 2 reviews.

Comment 26

[Robert Relyea]

Checking in pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.4; previous revision: 1.3
done
Checking in secmodt.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v  <--  secmodt.h
new revision: 1.29; previous revision: 1.28
done

Checked into 3.11, 3.10.2 waiting on the second review.

Comment 27

[Wan-Teh Chang]

Comment on attachment 197870 [details] [diff] [review]
Tweek the comment once again

Bob, I have some questions about this patch.

The most important one first.  In pk11auth.c, you have:

>+	if (slot->protectedAuthPath) {
>+	    /* application tried to authenticate and failed. it wants to try
>+	     * again, continue looping */
>+	    if (strcmp(password, PK11_PW_RETRY) == 0) {
>+		rv = SECWouldBlock;
>+		PORT_Free(password);
>+		break;

The comment says "continue looping", but the code is "break".
I think the code is wrong.

The rest of my questions are all unimportant.

In pk11auth.c, you have three code fragments that look
like this:

>     if (slot->protectedAuthPath) {
> 	len = 0;
> 	pw = NULL;
>+    } else if (pw == NULL) {
>+	PORT_SetError(SEC_ERROR_INVALID_ARGS);
>+	return SECFailure;
>+    } else {
>+	len = PORT_Strlen(pw);
>     }

This means it is fine to pass a NULL pw to the check-password
functions if slot->protectedAuthPath is true.  But this contradicts
your comment in secmodt.h that the password callback function
for a protected pin path slot should return a non-null password
other than "RETRY" and "AUTH" to force NSS to call the check-password
function.  This implies that a NULL pw should also be an invalid
argument for a protectedAuthPath slot.	(Note that PK11_DoPassword
never passes a NULL pw to pk11_CheckPassword.)

>+	/* if the token has a protectedAuthPath, the application may have
>+         * already issued the C_Login as part of it's pk11_GetPassword call.

"it's" => "its"

>+	 * Applications that don't know about protectedPinAuth will return a

Did you mean protectedAuthPath?

>+	     /* applicaton tried to authenticate and succeeded we're done */
>+	     } else if (strcmp(password, PK11_PW_AUTHENTICATED) == 0) {

Since this follows a "break" (or rather "continue") statement,
the "else" is not necessary.

In secmodt.h, you have:

>+/* All other non-null values mean that that NSS could call C_Login to force
>+ * the authentication. The following define is to add applications in 
>+ * documenting that is what it's trying to do */
>+#define PK11_PW_TRY		""      /* Default: a prompt has been presented
>+					 * to the user, initiate a C_Login
>+					 * to authenticate the token */

The value of PK11_PW_TRY could be any non-null string other
than "RETRY" and "AUTH".  You chose an empty string "".  This
has the unfortunate effect of making me wonder, does Bob mean
"non-null values" or "non-empty values"?  You can avoid this
potential confusion by defining PK11_PW_TRY as "TRY".

Comment 28

[Wan-Teh Chang]

Comment on attachment 197870 [details] [diff] [review]
Tweek the comment once again

One more:

>+ * the authentication. The following define is to add applications in 

I think "add" should be "aid".

Comment 29

[Robert Relyea]

Attachment: Fix continue bug in retry and minor suggestions from wtc.

Comment 30

[Robert Relyea]

Comment on attachment 197870 [details] [diff] [review]
Tweek the comment once again

My new patch applies on top of this patch.

Comment 31

[Wan-Teh Chang]

Comment on attachment 197920 [details] [diff] [review]
Fix continue bug in retry and minor suggestions from wtc.

r=wtc on this patch and "Tweek the comment once again" (attachment 197870 [details] [diff] [review]).

Comment 32

[Wan-Teh Chang]

The fix is in the NSS trunk (NSS 3.11) and NSS_3_10_BRANCH
(3.10.2).

Comment 33

[Robert Relyea]

I'm reopening this bug and assigning it to PSM. Let's see if we can finally get this in. The NSS changes are (or should be) complete.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Really changing to PSM

Comment 35

[Kai Engert (:KaiE:)]

Attachment: PSM Patch v3

I worked on this a bit.

I took the PSM code portions from attachment 78905 [details] [diff] [review] and attachment 78906 [details] [diff] [review] and applied them to today's trunk.

I had to tweak the code, because the patch had bitrotted.

I hope I correctly changed the code in the switch below
SECStatus rv = protectedAuthRunnable->GetResult();

We'll need to adjust the license blocks.

This code compiles, but how could we test it?

Comment 37

[Kai Engert (:KaiE:)]

Comment on attachment 262728 [details] [diff] [review]
PSM Patch v3

Please also see comment 35.

Comment 38

[Robert Relyea]

Comment on attachment 262728 [details] [diff] [review]
PSM Patch v3

r=rrelyea

I do have some comments. One is a semantic one, the othere are style.

The semantic comment:
The following code appears to not allow cancel. It would be nice to have a cancel button which kills the login thread in the protected auth dialog. Such a feature should not hold up this patch, however.

Style:

I have 2 style comments:

1) DisplayProtectedAuth takes a tokenName which is unused. It's caller always passes null. Token Name is passed as a part of the runnable. We should probably remove the parameter from the function call.

2) the new code in the password call back looks big enough and seperable enough to really be it's own function. 

Neither change is required for checkin.

bob

Comment 39

[Kai Engert (:KaiE:)]

I also did a review of the patch, a kind of super-review.

a) @Petr: Are you willing to donate your contributed code under the usual MPL/GPL/LGPL triple license?

b) we must use the correct license boilerplates.

c) I removed the dummy "Bla Bla" string in the xul window.

d) I did some whitespace cleanup.

e) Is it correct to say the following?

 * The Original Code is mozilla.org code.
 *
 * The Initial Developer of the Original Code is
 * Petr Kostka.
 * Portions created by the Initial Developer are Copyright (C) 2007
 * the Initial Developer. All Rights Reserved.

Comment 40

[Kai Engert (:KaiE:)]

Does it make sense to land this patch, although Bob and I are unable to test it?

Once we check it in, are people on the cc list of this bug willing to test using the nightly experimental builds of Firefox?

Comment 41

[Kai Engert (:KaiE:)]

Attachment: PSM Patch v4

This patch addresses Bob's style change requests and has the other changes I mentioned in my previous 2 comments.

I'll give Petr a chance to veto the triple-license before I go ahead an check this in, but note this is a requirement to accept your contribution.

Comment 42

[Fabian Pache]

Is this going to make it into a Firefox/Thunderbird release anytime soon?

I am currently implementing the pkcs11 component of a biometric token. The proposal to display a notice to the user when a token with CKF_PROTECTED_AUTHENTICATION_PATH is present is far better than the current state and I would be very happy to see this in the very near future 

In my particular case I could do without any notification to the user, since we need a proprietary dialog anyway. This dialog is displayed as soon as C_Login( *, *, NULL, 0 ) is called. 

Current behaviour: CKF_PROTECTED_AUTHENTICATION_PATH is ignored (largely)
Patched behaviour: display a notice if CKF_PROTECTED_AUTHENTICATION_PATH
Alternative: create a settings-file parameter to display nothing at all or display the default notice

When creating and formulating the notice for CKF_PROTECTED_AUTHENTICATION_PATH please keep in mind that the PKCS#11 module
a) might want to present a interface of its own
b) can request any number of things, besides a simple secure PINpad. Think retinal scanner, thumbprint scanner, voice recognition,

I would be happy to test this feature with a nightly build

Comment 43

[Kai Engert (:KaiE:)]

Petr, you didn't answer my question.

As you had contributed the code by attaching it, I conclude the answer to my questions in comment 39 is "yes".

This patch disappeared on the radar, but it's reviewed, so we should probably try to check it in.

But I see, it "bitrotted". It no longer applies and I must merge it to the latest trunk.

Unfortunately we are late in the release cycle :-/
That means we will need to request mozilla's drivers approval to get it in.

Comment 44

[Kai Engert (:KaiE:)]

Attachment: PSM Patch v5

Merged the patch to the trunk.

This patch has r=rrelyea and sr=kengert

Requesting approval.

Comment 45

[Reed Loden [:reed]]

Just a quick note to say that freeze for b2 is tomorrow, so please get this in. :)

Comment 46

[Kai Engert (:KaiE:)]

Attachment: Addon Patch v1

We can not check in PSM Patch v5 as is.
It breaks the build.

Reason: The patch changes an existing interface. Code that currently implements the existing interface is required to implement the interface in total. As they currently don't implement the new function "DisplayProtectedAuth", the build fails.

I ran into a failure when compiling Firefox, because it builds some embedding code. I suspect the same build failure would happen with Camino.

Here is a patch that might help, but I'm not sure if it's sufficient.
The patch works for me when building Firefox on Linux.
The patch might work on Mac for Camino, but I have not tested that.

Comment 47

[Kai Engert (:KaiE:)]

Comment on attachment 291389 [details] [diff] [review]
Addon Patch v1

Requesting mento's review for camino.

Requesting Benjamin's review for embedding.

Comment 48

[Fabian Pache]

Like I mentioned before, the notice for the protected auth method is not required. The true error of the core is to display a password request dialog. It is not about the wrong message. The message itself is just eyecandy and usually made redundant by authentication method specific behaviour.
Of course I understand the urge of frontend developers to inform the user about everything the product is doing. Please keep in mind that middleware developers might prefer the most unobstrusive way 

Comment 49

[Mark Mentovai]

Comment on attachment 291389 [details] [diff] [review]
Addon Patch v1

>Index: mozilla/camino/src/browser/SecurityDialogs.mm

>+NS_IMETHODIMP
>+SecurityDialogs::DisplayProtectedAuth(nsIInterfaceRequestor *ctx, nsIProtectedAuthThread *runnable)

Above this, there should be a comment showing the idl method declaration that this implements:

/* void displayProtectedAuth(in nsIInterfaceRequestor ctx, in nsIProtectedAuthThread runnable); */

because that's how the rest of the file is formatted.

Looks good.

Comment 50

[Kai Engert (:KaiE:)]

I checked in both patches, plus the change requested by Mark.
marking fixed.

Comment 51

[Kai Engert (:KaiE:)]

Petr, thanks a lot for your contribution!

Comment 52

[Kai Engert (:KaiE:)]

Bug 443284 reports a crash related to this new feature, and has questions in bug 443284 comment 5 and bug 443284 comment 7.