Stephane Saux Reporter
	Description • 23 years ago

PSM conterpart to bug 110062

	Stephane Saux Reporter
	Comment 1 • 23 years ago

add dependency.

	Stephane Saux Reporter
	Comment 2 • 23 years ago

Changes to PSM are in a patch published to 110062.

	Kai Engert (:KaiE:) Assignee
	Comment 3 • 23 years ago

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.

	Kai Engert (:KaiE:) Assignee
	Comment 4 • 23 years ago

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.

	Petr Kostka
	Comment 5 • 23 years ago

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

	Robert Relyea
	Comment 6 • 23 years ago

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

	Petr Kostka
	Comment 7 • 23 years ago

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 :(

	Petr Kostka
	Comment 9 • 22 years ago

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.

	R.Roesel
	Comment 10 • 21 years ago

Is there anybody working on this bug ? 

	John G. Myers
	Comment 11 • 21 years ago

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

	Priit Randla
	Comment 12 • 20 years ago

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...

	Robert Relyea
	Comment 13 • 20 years ago

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

	Nelson Bolyard (seldom reads bugmail)
	Comment 14 • 20 years ago

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

	Priit Randla
	Comment 15 • 19 years ago

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... 

	Wan-Teh Chang
	Comment 16 • 19 years ago

Thank you. I look forward to your contribution.

	Nelson Bolyard (seldom reads bugmail)
	Comment 17 • 19 years ago

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?

	Robert Relyea
	Comment 18 • 19 years ago

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);
   }

	Robert Relyea
	Comment 19 • 19 years ago

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.

	Nelson Bolyard (seldom reads bugmail)
	Comment 20 • 19 years ago

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 */

	Nelson Bolyard (seldom reads bugmail)
	Comment 22 • 19 years ago

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."

	Nelson Bolyard (seldom reads bugmail)
	Comment 24 • 19 years ago

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.

	Robert Relyea
	Comment 25 • 19 years ago

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

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

	Robert Relyea
	Comment 26 • 19 years ago

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.

	Wan-Teh Chang
	Comment 27 • 19 years ago

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".

	Wan-Teh Chang
	Comment 28 • 19 years ago

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".

	Robert Relyea
	Comment 30 • 19 years ago

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

My new patch applies on top of this patch.

	Wan-Teh Chang
	Comment 31 • 19 years ago

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]).

	Wan-Teh Chang
	Comment 32 • 19 years ago

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

	Robert Relyea
	Comment 33 • 17 years ago

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.

	Nelson Bolyard (seldom reads bugmail)
	Comment 34 • 17 years ago

Really changing to PSM

	Kai Engert (:KaiE:) Assignee
	Comment 35 • 17 years ago

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?

	Kai Engert (:KaiE:) Assignee
	Comment 37 • 17 years ago

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

Please also see comment 35.

	Robert Relyea
	Comment 38 • 17 years ago

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

	Kai Engert (:KaiE:) Assignee
	Comment 39 • 17 years ago

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.

	Kai Engert (:KaiE:) Assignee
	Comment 40 • 17 years ago

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?

	Kai Engert (:KaiE:) Assignee
	Comment 41 • 17 years ago

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.

	Fabian Pache
	Comment 42 • 17 years ago

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

	Kai Engert (:KaiE:) Assignee
	Comment 43 • 17 years ago

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.

	Kai Engert (:KaiE:) Assignee
	Comment 44 • 17 years ago

Merged the patch to the trunk.

This patch has r=rrelyea and sr=kengert

Requesting approval.

	Reed Loden [:reed]
	Comment 45 • 17 years ago

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

	Kai Engert (:KaiE:) Assignee
	Comment 46 • 17 years ago

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.

	Kai Engert (:KaiE:) Assignee
	Comment 47 • 17 years ago

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

Requesting mento's review for camino.

Requesting Benjamin's review for embedding.

	Fabian Pache
	Comment 48 • 17 years ago

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 

	Mark Mentovai
	Comment 49 • 17 years ago

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.

	Kai Engert (:KaiE:) Assignee
	Comment 50 • 17 years ago

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

	Kai Engert (:KaiE:) Assignee
	Comment 51 • 17 years ago

Petr, thanks a lot for your contribution!

	Kai Engert (:KaiE:) Assignee
	Comment 52 • 16 years ago

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