357025 - Support the new key object attributes in PKCS #11 v2.20, in particular CKA_ALWAYS_AUTHENTICATE

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

The following new key object attributes in PKCS #11 v2.20, Sections
10.7 - 10.10 are not supported by NSS.

Table 27, Common Key Attributes, p. 80
CKA_KEY_GEN_MECHANISM
CKA_ALLOWED_MECHANISMS

Table 28, Common Public Key Attributes, p. 81
CKA_TRUSTED
CKA_WRAP_TEMPLATE

Table 30, Common Private Key Attributes, p. 83
CKA_WRAP_WITH_TRUSTED
CKA_UNWRAP_TEMPLATE
CKA_ALWAYS_AUTHENTICATE

Table 31, Common Secret Key Attributes, pp. 85-86
CKA_CHECK_VALUE
CKA_WRAP_WITH_TRUSTED
CKA_TRUSTED
CKA_WRAP_TEMPLATE
CKA_UNWRAP_TEMPLATE

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

CKA_ALWAYS_AUTHENTICATE is the solution to the big problem described in bug
322617.  

If we had this feature, we would set this attribute on any private key 
associated with a NR cert, which would give European users the feature they
want - to be asked for their password every time they sign something with 
their NR cert - without them having to also be asked for their password 
every time they visit a site with a stored password.  

The absence of this feature, and of a solution to the problem described 
above, is leading to a rewrite of password manager that will not use SDR
for encrypting site passwords.  :(

Comment 2

[Mike Connor [:mconnor]]

Would it be possible to get this, or at least CKA_ALWAYS_AUTHENTICATE, on the radar for NSS 3.12?

Comment 3

[Robert Relyea]

There are 3 things that need to happen to get this support:

1) NSS needs to examine the CKA_ALWAYS_AUTHENTICATE flag before doing key ops and force a login in those cases.
2) Softoken needs to be able to store CKA_ALWAYS_AUTHENTICATE.
3) NSS needs to be able to set CKA_ALWAYS_AUTHENTICATE on a private key (either on creation, or afterwards).
4) PSM needs to decide what kind of key this is and set the attribute.

I have a patch for #1 I'm testing. (this will allow tokens that have the attribute set to start working immediately).

#2 I can roll in with the shared database support.

#3 if we just support setting/resetting this after the fact, it should be relatively easy.

#4 This falls back to bug 322617.

Comment 4

[Robert Relyea]

Attachment: Authenticate for CKA_ALWAYS_AUTHENTICATE private keys.

This patch treats private keys with the always authenticate bit value set and always requiring authentication. It has the same semantics as 'always authenticate' option, but only for those private keys.

NOTE: this patch does not change the behavior of calling C_Logout() on the token. It also doesn't provide a way to save value in softoken, or to set it on any token.

This patch only needs one review. Feel free to though the review off to someone else.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 268129 [details] [diff] [review]
Authenticate for CKA_ALWAYS_AUTHENTICATE private keys.

Bob and I discussed this patch by phone today.  
It seems that, for CKA_ALWAYS_AUTHENTICATE private keys, one must login
with a special user type (so as not to disturb the normal user type's 
login state).  This patch doesn't do that.  So, Bob and I agreed it's 
back to the drawing board for this patch.

Comment 6

[Robert Relyea]

taking bug so it shows up on my radar.

Comment 7

[Honza Bambas (:mayhemer)]

--> me

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: draft 1

This is preview of always-authenticate solution. It is based on Robert's patch. Has following issues:
1. Decision to set the attribute CKA_ALWAYS_AUTHENTICATE=true is made in NSS not PSM. I don't know how to get the list of imported certs in PSM code (nsPKCS12Blob::ImportFromFileHelper)
2. I tested the code using https://www.kuix.de/misc/test38/ that requires client authentication. Problem is that to find the cert and use it both invokes master password dialog. One is invoked by PK11_FindKeyByAnyCert, other by the new code as reaction to always authenticate attribute. User has to enter the password twice immediately one after other. We could solve this using a timeout?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 341471 [details] [diff] [review]
draft 1

Bob, Although Honza has not asked for review, I'd like to ask you to take an early look at this patch, and comment on any issues you see with the approach. 
I think this has the potential to be a very valuable contribution to NSS.  
If something about this proposal is fundamentally flawed, I'd like to save Honza some work by letting him know that ASAP.

Comment 10

[Honza Bambas (:mayhemer)]

One more issue: I am not sure how reading of attributes work exactly. PK11_HasAttributeSet(CKA_ALWAYS_AUTHENTICATE) returns true even on key w/o that attribute. I don't understand how this function works.

Comment 11

[Robert Relyea]

Thanks for looking at this Honza.

The problem with this patch is the semantic of CU_CONTEXT_SPECIFIC in the PKCS #11 spec.

IIRC, the call needs to happen on the session between the C_InitXXX and the C_Decrypt or C_Sign operation. (That way the PKCS #11 module knows which key is being authenticated. This means we can't just change the existing handle login call).

The big trick will be key unwrap. There is no 'C_InitXXX' function before C_UnwrapKey. The PKCS #11 group hasn't come up with a satisfactory answer (though there were discussions). The best thing to do in that case is if CKA_ALWAYS_AUTHENTICATE is set, then we need to drop to C_Decrypt rather than C_Unwrap. (or just always call handUnwrap).

The places you've found are usually just before, (or just before a call to function which does) the init call.


bob

Comment 12

[Robert Relyea]

re: CKA_ALWAYS_AUTHENTICATE... 

Hmm I believe unknown attributes should always return false. For softoken right now CKA_ALWAYS_AUTHENTICATE should be unknown.

Comment 13

[Robert Relyea]

Comment on attachment 341471 [details] [diff] [review]
draft 1

I've already talked to Honras about what is wrong with this patch. (and commented in the bug). I've given him an example of how the PKCS #11 call is supposed to work.

bob

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: Softoken part

This is here as WIP patch. 

To finish the work on always-auth (except UI things) I actually need just to find a way how to store the attribute to the legacy database. I found out that function storing the private key (seckey_encrypt_private_key @ keydb.c, http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/softoken/legacydb/keydb.c#1644) is encoding the key as ASN.1. In the template and in the structure used to store the key is NSSLOWKEYAttribute **attributes member that is null but (if I understand correctly) the ASN.1 template is able to process a data in it. Isn't it the place to store the attribute?

Or, is there a chance to move to the new database version used by Gecko?

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: Wrapper part

This is part belonging to the wrapper (slot).

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 347820 [details] [diff] [review]
Softoken part

Please see comment 14 for details on this patch.

This is not a final patch but I am stuck at the moment and don't want to go the way of try/failure.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 347820 [details] [diff] [review]
Softoken part

After studying this patch, I gather that this patch:

a) is intended only to implement the CKA_ALWAYS_AUTHENTICATE attribute in 
a way that the attribute type is known, but is always false.  Thus it is 
not intended to be a true implementation of this feature, but merely to 
provide recognition of the attribute type. and

b) only implements the attribute for legacy (cert8) databases, not for the 
new sqlite3-based cert9 DBs.  and

c) is based on the (dated) code in the Hg repository rather than on the CVS 
trunk.

Providing a false-only implementation might be an OK start, but it must
work with both old and new DBs, and must be a patch for the CVS trunk.
Honza, please let me know if you disagree with any of that analysis.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 347820 [details] [diff] [review]
Softoken part

I see that part of the softoken patch is in the next patch.  Also, it appears that the new cert9 code *may* already have some support for the new attribute type, so perhaps it is appropriate for this patch to add support only for cert8.  
I may change my review opinion of this patch when I finish reviewing the other patch.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 347821 [details] [diff] [review]
Wrapper part

I stopped reviewing this patch when I noticed that it is patching a rather old version of some of the files.  This is the problem with generating patches based on the downstream Hg repository rather than against the CVS trunk.

Please generate two new patches, one for softoken and one for the rest, and make them CVS diff -pu5 patches that apply cleanly to the CVS trunk.  Please put all the softoken patch parts together in one patch, then request review again.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: wip on cvs

This is cvs diff -u10dp on the mozilla/security trunk. It should implement the new attribute for shared database.

As I understand the legacy database code, it stores ASN.1 encoded object. For private keys I don't see a place where to store this new flag in the template. 

I set the ALWAYS_AUTH flag when importing a p12 file and the cert has non-repudiation usage flag. It's then stored to the key object using sftk_forceAttribute when handling the key object. 

When storing an attribute with sftk_forceAttribute, legacy engine ends up at lg_SetPrivateKeyAttribute with result crv = CKR_ATTRIBUTE_READ_ONLY. It only modifies the label, everything else is intact? Firefox even doesn't reach this code, sftk_isToken is false in sftk_forceAttribute, I don't understand the code from that place.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Honza, thanks for making this patch for the CVS trunk.
It's not generally necessary to increase the context size beyond the 
default value (3 lines) because bonsai's patch reader can do that for
CVS diffs.  

Since this patch changes softoken, and softoken changes must be coordinated
with FIPS evaluations, I'm marking this as a FIPS bug.

It's going to take me a while to complete this review, so I will add 
review comments in several separate bug comments. Please don't submit
another version of the patch until I indicate that 've complete a review 
pass in this bug.  Until then, consider all the comments about this patch
to be preliminary comments.  Here are some initial preliminary comments.

1. Your patch changes the signature on one or more functions that are 
explicitly exported from one or more NSS shared libraries.  So far, I have 
identified PK11_DoPassword as one such function.

We cannot change the signature of any function that is explicitly exported 
from an NSS shared library.  For each shared library, there is a .def file 
that names all the explicitly exported symbols.  If you want to know if a 
symbol is explicitly exported, you can tell by doing an MXR search for that 
symbol name in files in the "Security" tree whose names end in .def, e.g. 
to see if PK11_DoPassword is explicitly exported, do this search:
http://mxr.mozilla.org/security/search?string=PK11_DoPassword&case=on&find=%5C.def%24&tree=security

We handle this by adding a new function, rather change changing the 
existing function signature.  Often, we reimplement the old function as a 
call to the new function with suitable parameters.  We avoid using the 
suffix Ex for newly extended functions, and instead we give names that 
reflect the nature of the extension, e.g. ...WithFlags .

2. Don't add extern function declarations in .c files.  Put them in .h files.

3. Don't use // style comments, except in files that are known to be 
compiled only with compilers that accept them.  Not all of the supported
c compilers accept them. They're OK in files that are built only on Windows,
because all the compilers on Windows do accept them.  

4. Some of the new lines exceed 80 columns, I think.  Please don't.

5. Please follow the prevalent coding style of the source file being modified.
In most NSS source files, we combine else statements with the preceding one, 
e.g. 
   } else if (foo) {
or 
   } else {
rather than 
   }
   else if (foo) {
or
   }
   else 
   {

6. There is a new variable named "unauthenticated".  I'd rather that it have 
a name that says what it is, than what it's not.  So, I wonder if perhaps 
it should be named "authenticated" and have its sense inverted.  Or perhaps
it should be named "needsAuthentication", or even "alwaysAuthenticate".

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Another patch comment: In function PK11_ImportEncryptedPrivateKeyInfo, 
all the arrays of CK_ATTRIBUTE_TYPE except the new one named "usage" can 
now be declared static const.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362077 [details] [diff] [review]
wip on cvs

I'm going to ask Bob to SR this.  He can probably do it faster than I, and he may detect issues that would not occur to me. I will continue to review this.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

More patch comments:
- This patch increases the size of a local automatic array in function 
PK11_UnwrapPrivKey but makes no other change to that function.  The reason 
for that change is inobvious.  Is this a bug fix?  Is there some code 
outside of this function that requires this array to be larger?  In that 
latter case, there needs to be a source code comment explaining that.

- This patch adds code to handle Sign and Decrypt.  The Sign code also 
handles the Unwrap case.  But I see no code to handle DeriveKey operations.
It appears that a private key with the ALWAYS_AUTHENTICATE attribute could 
be used without authentication in key derivation operations.

- Is the "always authenticate" attribute only defined for private keys?
Could it also be set on secret (symmetric) keys?

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Question for Bob, Wan-Teh and Glen,
Is this bug part of the FIPS Feature complete target?

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362077 [details] [diff] [review]
wip on cvs

Please change the block comment for function PK11_HandlePasswordCheck to 
mention that this function does the check that the CKU_USER is logged in
to the token, and outputs a variable telling the caller whether to also
do a subsequent password check for the CKU_CONTEXT_SPECIFIC session.  

I'm not entirely sure that it's necessary to require both forms of login.
Bob, do you know?  Also, can you answer Kaspar's questions in comment 20?

I'm done with my review of this patch.  My review is r-.  I think Bob should
look at this patch before the next patch is submitted.  I definitely want
his SR before any patch is committed for this feature.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

In the above comment, I mentioned Kaspar, but I should have mentioned Honza.
Sorry for the confusion.  Honza, even though your patch is being picked 
over, on the whole, it seems to be pretty complete and well done.  
When these few remaining problems are resolved, I'm pretty confident that
it will be good and we'll want it for this FIPS evaluation.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to comment #24)
> More patch comments:
> - This patch adds code to handle Sign and Decrypt.  The Sign code also 
> handles the Unwrap case.  But I see no code to handle DeriveKey operations.
> It appears that a private key with the ALWAYS_AUTHENTICATE attribute could 
> be used without authentication in key derivation operations.
> 

I don't see ALWAYS_AUTHENTICATE attribute be bound to key derivation in the spec, however if you want me to do it, I can.

> - Is the "always authenticate" attribute only defined for private keys?
> Could it also be set on secret (symmetric) keys?

It is just defined for private keys, not for secret keys.

Comment 29

[Wan-Teh Chang]

CKA_ALWAYS_AUTHENTICATE being only defined for private keys and
not for secret keys could be an unintentional omission in the
spec.  We may want to ask about this on the Cryptoki mailing
list.  PKCS #11 is a very long document.  When someone amends
it, it's easy to miss a section that also needs to be updated.
In any case, it's fine to only define CKA_ALWAYS_AUTHENTICATE
for private keys for now.

As for the crypto operations CKA_ALWAYS_AUTHENTICATE is bound
to, although Table 30 says  "for each use (sign or decryt) with
the key", the text at the bottom of page 83 says "'Use' in this
case means a cryptographic operation such as sign or decrypt."
So I think CKA_ALWAYS_AUTHENTICATE should also apply to key
derivation.

By the way, do you understand the last sentence of Section 10.9
on page 84?  I don't understand what it means.  Anyway, I don't
see CKR_OPERATION_NOT_INITIALIZED in your changes to NSC_Login,
so I want to make sure you will cover that.

Comment 30

[Robert Relyea]

> I don't see ALWAYS_AUTHENTICATE attribute be bound to key derivation in the
> spec, however if you want me to do it, I can.

I believe the current PKCS #11 spec does define how you do ALWAYS_AUTHENTICATE with C_Derive.

the spec says you do the C_Login call between the _Init functions and the _Op function (that is between C_SignInit and C_Sign, C_DecryptInit and C_Decrypt). Both C_Derive and C_Unwrap do not have that ability.

bob

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Bob, did you mean to write "does NOT define how ..." ??

Comment 32

[Robert Relyea]

Yes, I hate it when I drop out little words like that...;).

bob

Comment 33

[Robert Relyea]

re comment 12: For the softoken portion, yes, it's already to late, but there is value in completing the rest of the work. Honza, go ahead and keep the softoken portion for your testing. I'll review all of them for suitability, however the softoken portion cannot be landed until after we complete our FIPS validation.

bob

Comment 34

[Robert Relyea]

Review all the non-softoken parts of the patch..

The patch will get an r-, primarily for the issue found in 5A below. as well as 4B.

Note, in general, I think the patch looks pretty good. I found no public API's changed (though a couple of internally exported APIs where changes, nothing exported in nss3.def.

I'll give a review of the softoken shortly...

---------------------------------------------

1) Changes to pk11util/pk11table.c -- looks good
2) Changes to cryptohi/keythi.h - looks good
3) Changes to cryptohi/seckey.c - looks good
4) Changes to pki11wrap/pk11akey.c -
    4A) Suggestion for the patch:
     change macro at 1263 to
    #define INPORT_KEY_USAGE_ARRAY_APPEND(source, count) \
	PR_BEGIN_MACRO \
	   PORT_Memcpy(&usage[usageCount], source, count*sizeof(CK_ATTRIBUTE_TYPE));\
	   usageCount +=count; \
	PR_END_MACRO

    4B) Required for patch:
	at line 1325, also check for PKCS #11 version > 2.20 before setting the alwaysAuthenticate attribute. We don't what the key gen to fail on older tokens. (this patch should still work with the old softoken).
        at line 1347 remove assert. usage is now an array, not a pointer.


5) Change to pk11wrap/pk11auth.c -

5A) in pk11_CheckPassword....
    line 128 is not correct. The tests between 127 and 133 is to handle the case where the token was removed and re inserted while we were prompting for the password.The removal killed all our sessions. the PK11_InitToken will reinitialize the token and get a new slot session, so slot->session at 102 is not the same as slot->session at 127. In the current code, hSession will never be CKA_INVALID_SESSION and we will always go through the retry look (and alway get the CKR_SESSION_CLOSED or CKR_SESSION_HANDLE_INVALID return). This problem can be fixed in a number of ways:  

5A-i) allow hSession to be CK_INVALID_SESSION. In that case we would use the Session handle at line 103 as

   crv = PK11_GETTAB(slot)->C_Login(
	hSession == CK_INVALID_SESSION ? slot->session: hSession, 
	usertype, .... )

 then the code at 126- 134 would look like:
   /*
    * handle the case where we are using the slot session and the token has been
    * removed and reinserted while the password prompt was on.
    */
    if (hSession == CK_INVALID_SESSION && retry++ = 0) {
	...... rest of original code stays the same ....
    } 

5A-ii) Remove the do{ ... } while (must retry), make sure PK11_MapError mapps to a unique error that says the token's been removed and kick the retry code up to the next level.

5A-iii) Implement #1 plus make sure the PK11_MapError maps to a unique error and retry in the AlwaysAuthenticate code.

I suggest 5A-i or 5A-iii (5A-ii would require  a ton of surgery and may not be possible to maintain signature compatibility of exported functions).


5B) in PK11_HandlePasswordCheck... These changes are fine (though issue in pk11_CheckPassword applies here). PK11_HandlePasswordCheck is a locally exported function (only available inside nss3.so), so it's OK to change the signature.

5C) The rest of the changes in pk11_CheckPassword is fine (though may need to be adjusted to support the pk11_CheckPassword issue). None of the modifed signatures are exported for nss3.so.

6) Changes to pk11priv.h: looks good, the exported functions are 'locally' exported and OK to change the signatures on.

7) pk11wrap/pk11obj.c -- mostly looks good a couple of nits. Also if you implement 5A-iii or 5A-ii, this code would need additional changes.

   nits: 
Restore the comment formerly at line 802 and should be at line 808. It's still valid except for the "In practice, thought,...." sentence which you can strike.


8) pk11wrap/pk11skey.c --Looks good except the implementing 5A-iii or 5A-ii, this code would need additional changes.


Additional notes on implementing 5A-iii....

The more I think about it, the more I think we probably should implement it.
The first part is identical to 5A-i, The second part, we need to return an error back to the code that is doing the actual Decrypt/Sign.

That code would now look something like....

do {
   mustRetry = 0;
   session = pk11_GetNewSession(slot, ....)
     .
     .
   crv = PK11_GETTAB(slot)->C_XXXXInit(session, ....)
     .

     .

   if (alwaysAuth) {
	rv = PK11_DoPassword(slot, CKU_CONTEXT_SPECIFIC, session, ....).
	if (rv != SECSUCCESS && PORT_GetError() == SECTokenRemoved && retry++ = 0)
		rv = PK11_TokenInit(slot...);
		if (slot->session != CKR_INVALID_SESSION) {
		   mustRetry = PR_TRUE;
		}
	}
    }
while (mustRetry);

This code block will likely be repeated a lot, you may want to create a helper function to do this (especially since C_XXXX only has 2 flavors in this case (C_SignInit and C_DecryptInit).

bob

Comment 35

[Robert Relyea]

comments on nelson's comments.

> 1. Your patch changes the signature on one or more functions that are 
> explicitly exported from one or more NSS shared libraries.  So far, I have 
> identified PK11_DoPassword as one such function.


>  do this search:
> http://mxr.mozilla.org/security/search?string=PK11_DoPassword&case=on&>find=%5C.def%24&tree=security

This returns a line in nss3.def at is commented out. Why it is even there, I don't know, but it's not exported.

Please heed all the rest of nelson's comments in any future patches. (I've yet to review Comment 21 point 6, but on the face of it, it sounds right).

softoken portion to be reviewed shortly....

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Bob, you're right!  That line IS commented out.  Don't know why I missed that.
So, PK11_DoPassword is NOT a public function (which is consistent with it 
being declared in a private header file :).  That's good news.  

So, Honza, let me ask that your next patch should (also) remove that 
commented-out line from nss3.def

Comment 37

[Honza Bambas (:mayhemer)]

According to the description in nss.def above from 

;+#CERT_DecodeCertFromPackage;

the ;+ will get removed on certain platforms but # will remain and the line will be considered as a comment? Shouldn't then be all such lines removed from that file?

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Lines that begin with ;+# are comments on all platforms.
They should be used for comments only, and not to "comment out" entry
point names.  It's pretty easy to tell which lines are comments and
which are "commented out" entry point names.  

So, in answer to comment 37, If you want to remove all the 
"commented out" entry point names, feel free, with my blessing!

Comment 39

[Robert Relyea]

Comment on attachment 362077 [details] [diff] [review]
wip on cvs


Here's the softoken portion of the review. In general it looks good, you hooked the right places and used the correct structures to store the authenticated state. I suggest just splitting this into 2 variables.

r- minus mostly for the issues in the non-softoken version.

in softoken/pkcs11.c - minor comments.
   lin 505 move the new declaration of sftk_ReturnContextByType() to pkcs11i.h
   line 1091 you want sfkt_defaultAttribute rather than sftkt_forceAttribute. Default attribute only sets the value if the user did not supply it You should default it to ckfalse.
   line 3501. We should have an 'else' close if there isn't a context than needs authentication. I suggest 2 booleans: 1) needAuthentication, set to true at the INIT if the key is a CKA_ALWAYS_AUTHENTICATE, and 2) isAuthententicated (converse of your unauthenticated) set to true in this function). we probably should look up the context while we still have the session at line 3494. Also, it looks like this change will require us to hold more pointers open longer. Feel free to adjust the error processing to handle that (change returns xxx to crv=xxxx goto done; for instance).
    line 3581 & 3534: nit. you use a switch in one case and a if () elsif() in the other for pretty much the same operation. you should probably be consistent.
    

in softoken pkcs11c.c -
     line 834 and line  2134 : There is where you would seen needAuthenticate.

     line 1176, 1236, 2164, 2255, 2314  : The if would then be if (needAuthentate && !authenticated)

    line 3596:  You need the equivalent code above in the decrypt case. (you probably didn't run into this in testing because we currently do not set CKA_ALWAYS_AUTHENTICATE on encryption keys).

 in pkcs11u.c -
      line 683: CKA_ALWAYS_AUTHENTICATE is specified as a 'changeable' attribute, so technically we should set to to SFTK_MODIFY rather than SFTK_NEVER. (even if we can't change it in the old database.

in legacydb/lgattr.c - This change looks good. The effect of this is that we can set the use the attribute in the new database, but not the old database.


bob

Comment 40

[Honza Bambas (:mayhemer)]

According to FIPS validation, is there any milestone this patch has to be ready? I've been now finishing some more priority 1.9.1 blocking things.

Comment 41

[Doug Engert]

I have run into the CKA_ALWAYS_AUTHENTICATE issue while testing OpenSC 
PKCS#11 with FIPS-201 PIV NIST-800-73-3 smart cards. Although some
original PIV cards may not have enforced this, newer ones do. Even the 
original NIST 800-73 from April 2005 Section 1.9.3 Digital Signature Key 
says: 
 "This requires cardholder participation every time the key is used for 
  digital signature generation" 

The newer cards, for example the Oberthur ID-ONE V7 with the PIV applet 
enforce this be requiring a sign operation to be proceed with a pin 
verify with no intervening operations to the card. 

If lucky, the C_Login to the card maybe the previous operation to the card
before a C_Sign is done. But then the user can only do one sign operation,
and must remove the card, so a new C_Login is forced for the next signature.

So the requirement has been around for at least 5 years now!

Comment 42

[Doug Engert]

Since no activity appears to be happening on the CKA_ALWAYS_AUTHENTICATE
issues for over 17 months, I developed a simple patch similar to the
362077 patch from comment 20 above. But it only addressed the pk11wrap
use for sign operations and is against 3.12.7. 

The main difference is that it only modifies PK11_DoPassword and 
wherever a key has CKA_ALWAYS_AUTHENTICATE, the code will call
PK11_DoPassword is called. Currently it only is checking for C_Sign.

For what is it is worth, the patch is attached, and has been used
with OpenSC for testing.

Comment 43

[Doug Engert]

Attachment: Simple patch against 3.12.7 for CKA_ALWAYS_AUTHENTICATE

Comment 44

[Doug Engert]

Attachment: Updated patch for nss-3.12.10 [checked in]

Updated verison of previous patch. 
Tested with TB 5.0, using nss-0.12.10 compiled on Solaris 10. OpenSC-pkcs11.so
used with HSPD-12 PIV card that enforces CKA_ALWAYS_AUTHENTICATE on the card.

Also needs patch with bug #613507 to avoid having a card read additional
object off card in the middle of a signing operation.

Comment 45

[Doug Engert]

(In reply to Doug Engert from comment #44)
> Created attachment 552168 [details] [diff] [review]
> Updated patch for nss-0.12.10

Typo: patch is for nss-3.12.10

> 
> Updated verison of previous patch. 
> Tested with TB 5.0, using nss-0.12.10 compiled on Solaris 10.
> OpenSC-pkcs11.so
> used with HSPD-12 PIV card that enforces CKA_ALWAYS_AUTHENTICATE on the card.
> 
> Also needs patch with bug #613507 to avoid having a card read additional
> object off card in the middle of a signing operation.

Comment 46

[Wan-Teh Chang]

Comment on attachment 552168 [details] [diff] [review]
Updated patch for nss-3.12.10 [checked in]

Bob, please review this patch.

In security/nss/lib/pk11wrap/pk11priv.h:

>-SECStatus PK11_DoPassword(PK11SlotInfo *slot, PRBool loadCerts, void *wincx);
>+SECStatus PK11_DoPassword(PK11SlotInfo *slot, PRBool loadCerts, void *wincx,
>+				PRBool contextSpecific);

PK11_DoPassword is declared in an exported header (pk11priv.h), but
PK11_DoPassword is not exported from libnss3.so; it seems to be commented
out in nss.def:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/nss/nss.def&rev=1.215&mark=450#450

So I think it is fine to change the function prototype of PK11_DoPassword.

Comment 47

[Robert Relyea]

> So I think it is fine to change the function prototype of PK11_DoPassword.

Yes, there was quite a discussion about that earlier in the bug. We should remove the commented out entry. PK11_DoPassword is indeed a private function.

Comment 48

[Robert Relyea]

Comment on attachment 552168 [details] [diff] [review]
Updated patch for nss-3.12.10 [checked in]

r+ such as it is. Note: there is still an issue with making sure the Unwrap case works....

bob

Comment 49

[Kai Engert (:KaiE:)]

Comment on attachment 552168 [details] [diff] [review]
Updated patch for nss-3.12.10 [checked in]

Checking in cryptohi/keythi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keythi.h,v  <--  keythi.h
new revision: 1.17; previous revision: 1.16
done
Checking in cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.67; previous revision: 1.66
done
Checking in pk11wrap/pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.16; previous revision: 1.15
done
Checking in pk11wrap/pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.25; previous revision: 1.24
done
Checking in pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.18; previous revision: 1.17
done

Comment 50

[Kai Engert (:KaiE:)]

For tracking purposes, I propose to resolve this bug as fixed, and file a follow up bug for any remaining work.

Comment 51

[Robert Relyea]

This patch lead to a double lock one some tokens. Also, to be safe, we should login on the same session we are doing the signing.

Comment 52

[Robert Relyea]

Attachment: Fix locking issue. Also authenticate on the correct session [checked in]

Incremental patch on top of the previous patch.

Comment 53

[Kai Engert (:KaiE:)]

Comment on attachment 677824 [details] [diff] [review]
Fix locking issue. Also authenticate on the correct session [checked in]

You're lucky that PK11_DoPassword is commented out in nss.def :)

r=kaie

Comment 54

[Kai Engert (:KaiE:)]

Comment on attachment 677824 [details] [diff] [review]
Fix locking issue. Also authenticate on the correct session [checked in]

You had asked for checkin-needed on this bug earlier, so I've checking in the additional patch for you, too.

Checking in lib/cryptohi/keythi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keythi.h,v  <--  keythi.h
new revision: 1.18; previous revision: 1.17
done
Checking in lib/cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.69; previous revision: 1.68
done
Checking in lib/pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.37; previous revision: 1.36
done
Checking in lib/pk11wrap/pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.17; previous revision: 1.16
done
Checking in lib/pk11wrap/pk11merge.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11merge.c,v  <--  pk11merge.c
new revision: 1.11; previous revision: 1.10
done
Checking in lib/pk11wrap/pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.28; previous revision: 1.27
done
Checking in lib/pk11wrap/pk11priv.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11priv.h,v  <--  pk11priv.h
new revision: 1.20; previous revision: 1.19
done

Comment 55

[Kai Engert (:KaiE:)]

Wan-Teh is probably unhappy, because we're now unable to correctly track the issue using the target milestone field.

For the record, initial support added with NSS 3.14
Locking issues fixed in NSS 3.14.1

I'm updating the target to 3.14.1 to indicate the version that has all the fixes.

I'm resolving this bug as fixed, let's handle followup work or regressions in a new bug.

Comment 56

[Wan-Teh Chang]

(In reply to Kai Engert (:kaie) from comment #55)
> Wan-Teh is probably unhappy, because we're now unable to correctly track the
> issue using the target milestone field.

Yes, the double lock problem reported in comment 51 should have been
filed as a separate bug that was introduced in NSS 3.14 :-)

Comment 57

[J.K.Umeboshi]

OK, the PIN request does not locks anymore (I'm using FF20).
But now Firefox is prevented to quit.

After the second PIN request has bee shown, also if you click on cancel, when you close Firefox the process still be alive in background, also generating some activity on the CPU.

There is also an user experience issue here: the second PIN request is misleading, is identical to the request of the MAIN PIN.
Usually smart cards requiring the submit of the PIN before each signature have a secondary PIN having a different value from the main one.

Comment 58

[Doug Engert]

(In reply to Giuseppe Amato from comment #57)
> OK, the PIN request does not locks anymore (I'm using FF20).
But now Firefox
> is prevented to quit. After the second PIN request has bee shown, also if
> you click on cancel, when you close Firefox the process still be alive in
> background, also generating some activity on the CPU.


Sounds like a seperate problem.

> There is also an user
> experience issue here: the second PIN request is misleading, is identical to
> the request of the MAIN PIN.
Usually smart cards requiring the submit of the
> PIN before each signature have a secondary PIN having a different value from
> the main one.


The cards on which this modification was tested (PIV) used the same PIN
to unlock the card for authentication as well as to allow a signature operation.

FireFox has always had a problem with multiple pins. OpenSC had some work around to 
only expose one pin to FireFox.

IMHO,If you cards requires multiple PINs, then this should be a seperate bug report.

A PKCS#11 trace (or use the OpenSC pkcs11-spy) would show if NSS and the PKCS#11 
library are providing any information aboiunt multiple PINS.