613496 - NSS softtoken ECC support is incomplete when used with other PKCS#11 modules

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Doug Engert]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; InfoPath.2)
Build Identifier: NSS-3.12.7 

I am testing Thunderbird with OpenSC PKCS#11 using ECDSA. The smart card
being used is an Oberthur ID-ONE V7 with a FIPS-201 PIV NIST 800-73-3
applet. 
  
(This is the first of 3 related bug reports on NSS to get this working.)
 
I found a number of errors with the NSS EC code:

The soft token, does not advertise it ECDSA mechanism, thus if a verify
operation in needed on a certificate signature, NSS will try to 
use the OpenSC pkcs11 module. It does this by trying to create a session 
public key object in the token, which is not supported.  The soft token 
code has the ability to do the verify and will with the attached changes. 
 
cmssigninfo.c, secsign.c and p7decode.c will only work with if 
NSS_ECC_MORE_THAN_SUITE_B is defined. The attached patch changes the 
#ifdefs to test for NSS_ENABLE_ECC so ECC keys can be used in "Basic ECC"
mode too. 

The attached patch adds:
 To secmodt.h: 
    #define SECMOD_EC_FLAG     0x00040000L
 pk11pars.h:
    SECMOD_ARG_ENTRY(EC,SECMOD_EC_FLAG),
 pkcs11slot.c:
    #ifdef NSS_ENABLE_ECC
        { "ECDSA", SECMOD_EC_FLAG, CKM_ECDSA },
        /* TODO add ECDH */
    # endif
 ecl_curve.h:
   Remove: 
    #ifdef NSS_ECC_MORE_THAN_SUITE_B
    #error This source file is for Basic ECC only .
    #endif

The ECC code appears to work with or without
NSS_ECC_MORE_THAN_SUIT_B  as the NIST-800-73 only requires 2 
named curves be supported, which are part of the "Basic ECC"

Testing was done on Solaris 10, using Thunderbird-3.1.4 with
separately compiled nss-3.12.7. 

Reproducible: Always

Steps to Reproduce:
1. Use a smartcard with ECDSA to sign e-mail
2. 
3.
Actual Results:  
Would not work. 


See attached patch to nss-3.12.7. Looking at the online source, these
changes still apply.

Comment 1

[Doug Engert]

Attachment: Patches to let softtoken handed ECC, and get sign to work in "Basic ECC" mode

Comment 2

[Doug Engert]

Also see related bug:
 357025 - the need for CKA_ALWAYS_AUTHENTICATE
 613507 - Cause NSS to not need to read additional objects when 
          searching for only a certificate

Comment 3

[David Cooper]

I have also come across the problem of ECDSA signature verification failing if a PIV Card is present in a card reader and is activated.  This problem occurs for me even with the above patch installed since NSS is using a value for slotFlags from a pre-existing pkcs11.txt file that does not list ECDSA.

The issue can be reproduced as follows:
1. Start up Thunderbird or Firefox with a smart card that supports the CKM_ECDSA mechanism, but that does not support signature verification (e.g., it can create ECDSA signatures but not verify them).
2. Open Preferences and click the "View Certificates" button under the "Encryption" tab of the "Advanced" tab.
3. Enter PIN when requested to activate the smart card.
4. Attempt to view a certificate that has been signed using ECDSA. (signature verification fails).
5. Remove smart card from reader and attempt to view certificate again. (signature verification succeeds).

The problem is that since "ECDSA" does not appear in slotList, a new list of available slots is generated each time PK11_GetBestSlotMultiple is called and the slot for the smart card will be the first slot in this list.  PK11_GetBestSlotMultiple can only be used to ask for a slot that supports a given mechanism (e.g., CKM_ECDSA) and that has been activated.  In the scenario above, the smart card satisfies these conditions and so the slot for the smart card is returned to PK11_Verify, which unsuccessfully attempts to use the smart card to perform the ECDSA signature verification.

The attached patch solves this problem by creating a new function, based on PK11_GetBestSlotMultiple, that permits a mechanism information flag to be specified for each specified mechanism.  PK11_Verify then asks for a slot that supports that CKM_ECDSA mechanism and for which the CKF_VERIFY flag is set.  Since the flag is not set for the smart card, the new PK11_GetBestSlotMultiple returns the next slot in the list, which does support ECDSA signature verification.

In creating the patch I changed a few of the calls to PK11_GetBestSlot or PK11_GetBestSlotMultiple to call the new functions, but tried to be very conservative any only make the change if it was clear that the calling function required the specified flag to be set for the mechanism.

Comment 4

[David Cooper]

Attachment: Adds a revised version of PK11_GetBestSlotMultiple that permits the specification of a required mechanism information flag for each mechanism.

Comment 5

[Viktor Tarasov]

My problem seems also to be related -- when sending key exchange, the client tries to generate EC key on the smartcard.

PIV card is present and declares available the CKM_ECDSA and CKM_EC_KEY_PAIR_GEN mechanisms.
Client negotiates the ECDH cipher and at the stage of sending the client key exchange,
tries to create the ephemeral EC keypair as a 'session' object.
The slot to generate keypair is obtained by the call to PK11_GetBestSlot(CKM_EC_KEY_PAIR_GEN) .

PK11_GetBestSlot() returns not the 'best' but the 'first' slot capable to perform the asked mechanisms .
It do not makes a difference between if 'session' or 'token' object was asked for, HW or internal slots.
In my case the first slot is PIV card.

Finally there is an attempt to generate EC key on the PIV card in the read-only session .

I don't know if a 'session' object is ever created with HW mechanism, 
but I guess that PK11_GetBestSlot(), at least, should firstly look through the internal slots and 'software' mechanisms, and after that look through the HW ones.
(At least in that manner the problem was solved for me.)

If the parameter list of GetBestSlot() will be extended by the mechanism flags, as David proposes,
I guess that it should also take into account the PK11_ATTR_xx flags .

Comment 6

[Doug Engert]

Attachment: Enable ECDSA in nss-0.12.10

Updated version of the 491832 patch for nss-0.12.10 tested with TB 5.0
with nss-0.12.10 on Solaris 10, using OpenSC-pkcs11.so with HSPD-12 PIV
cards that support ECC. This is part 1 of 3 patches for this bug that
are being updated to nss-0.12.10.

Comment 7

[Doug Engert]

Attachment: Updated veriosn of D. Cooper's BestSlot patch

Updated version of David Cooper's Best Slot patch, to work with nss-0.12.10.
This is part 2 of 3 for updates to this bug report.

Comment 8

[Doug Engert]

Attachment: modutil update for ECDSA

Patch for nss-0.12.10 to have modutil support for ECDSA. 
This is part 3 of 3 for ECDSA support for this bug.

Comment 9

[Doug Engert]

(In reply to Viktor Tarasov from comment #5)
> My problem seems also to be related -- when sending key exchange, the client
> tries to generate EC key on the smartcard.

David has submitted 5 additional bug reports to support ECDH.
591640, 676100, 676108, 676114, 676118. 
These combined with 357025, 613507, 613496, will allow
the OpenSC-pkcs11.so module to support both ECDSA and EDCH with 
the PIV card. (The OpenSC mods for ECDSA have been in OpenSC
since 12/2010, and the ECDH are now at github for testing.)
The OpenSC will only support crypto on the card, and the only
PKCS#11 session objects are secret key objects returned from a
C_DeriveKey. David's Best SLot mods force softoken to do all the
off card crypto and should address your problem.  
  
> 
> PIV card is present and declares available the CKM_ECDSA and
> CKM_EC_KEY_PAIR_GEN mechanisms.
> Client negotiates the ECDH cipher and at the stage of sending the client key
> exchange,
> tries to create the ephemeral EC keypair as a 'session' object.
> The slot to generate keypair is obtained by the call to
> PK11_GetBestSlot(CKM_EC_KEY_PAIR_GEN) .
> 
> PK11_GetBestSlot() returns not the 'best' but the 'first' slot capable to
> perform the asked mechanisms .
> It do not makes a difference between if 'session' or 'token' object was
> asked for, HW or internal slots.
> In my case the first slot is PIV card.
> 
> Finally there is an attempt to generate EC key on the PIV card in the
> read-only session .
> 
> I don't know if a 'session' object is ever created with HW mechanism, 
> but I guess that PK11_GetBestSlot(), at least, should firstly look through
> the internal slots and 'software' mechanisms, and after that look through
> the HW ones.
> (At least in that manner the problem was solved for me.)
> 
> If the parameter list of GetBestSlot() will be extended by the mechanism
> flags, as David proposes,
> I guess that it should also take into account the PK11_ATTR_xx flags .

Comment 10

[David Cooper]

Attachment: Implementation of PK11_GetBestSlotMultiple_mechanism_flags

No changes to patch from Comment #4 or Comment #7.  Just updated so that the patch works cleaning with the current development branch of the code.

Comment 11

[Robert Relyea]

Comment on attachment 633657 [details] [diff] [review]
Implementation of PK11_GetBestSlotMultiple_mechanism_flags

r-

For two reasons, one with the patch itself, and the other with timing...

The first problem is the patch to lib/ssl/ssl3con.c and lib/ssl/derive.c. This patch will break some hardware tokens. The NSS wrap/unwrap function can handle both wrap/unwrap and encrypt/decrypt depending on what tokens support. Some hardware accelerators purposefully do not support wrap to force NSS to decrypt then import the key.

The second is not your fault, just bad timing. There is a similiar patch in bug 475578 that's been waiting review and conflicts with this one. (It adds keysize to the options to GetBestSlot). I may look at a single version with both options merged in.

bob

Comment 12

[David Cooper]

Attachment: Implementation of PK11_GetBestSlotMultipleKeySize_mechanism_flags

(In reply to Robert Relyea from comment #11)

Okay, I removed the changes to lib/ssl/ssl3con.c and lib/ssl/derive.c, and I also added key size to the options for GetBestSlot.

I believe that if this patch is checked in before the one in bug 475578, then that patch will work fine with this code, with the exception that the calls to GetBestSlot in PK11_Verify() in lib/pk11wrap/pk11obj.c would have to be updated to include a check for the correct mechanism flag, as is done in this patch.  That is, if mech.mechanism is CKM_DSA then the call would be PK11_GetBestSlotKeySize_mechanism_flags(mech.mechanism, length*BITS_PER_BYTE,CKF_VERIFY,wincx), and if mech.mechanism isn't CKM_DSA then the call would be PK11_GetBestSlot_mechanism_flags(mech.mechanism,CKF_VERIFY,wincx).

Comment 13

[Robert Relyea]

Thanks Dave. 

I need to hold off until I've gotten my review on bug 475578. I'll go ahead an merge the two then. I've been waiting over a month for the review on that bug, so I'm sort of incented not to do anything that will make it more difficult to have it come in. Hopefully wtc can get that one reviewed this week.

Comment 14

[Robert Relyea]

Attachment: Merge David's patch with current tip (checked in).

Dave, here's a new version of your patch with some changes:

1) I've merged WithKeySize and _mechanism_flags to a single function with standard NSS naming call PK11_GetBestSlotMultipleWithAttribute. There is only one new exported function rather then 3 (or more exactly 2 verses 6).

2) I've split off the check for filtering out slots into it's own helper function, which is coded to only get the mechanism info once per call even if we are filtering on both flags and keySize.

Please take a look at my patch to make sure it does everything you wanted. If it does, I'll check it in.

Thanks,

bob

Comment 15

[Robert Relyea]

Comment on attachment 634146 [details] [diff] [review]
Implementation of PK11_GetBestSlotMultipleKeySize_mechanism_flags

remove review request in favor of my updated patch.

Comment 16

[David Cooper]

Comment on attachment 637262 [details] [diff] [review]
Merge David's patch with current tip (checked in).

Review of attachment 637262 [details] [diff] [review]:
-----------------------------------------------------------------

Bob,

I compared this patch to mine and ran a quick test with it, and it seems to work fine.  I just have two comments on the patch.

1) I did a search of all of the calls to C_GetMechanismInfo in NSS, and they are all of the form:

    if (!slot->isThreadSafe) PK11_EnterSlotMonitor(slot);
    crv = PK11_GETTAB(slot)->C_GetMechanismInfo(slot->slotID,
                               mechanism,&mechanism_info);
    if (!slot->isThreadSafe) PK11_ExitSlotMonitor(slot);

However, in your call to C_GetMechanismInfo in pk11_filterSlot, you do not surround the call with conditional calls to PK11_EnterSlotMonitor and PK11_ExitSlotMonitor.  My testing did not involve any slots for which isThreadSafe was FALSE.  Is there a reason that you didn't include these two lines of code?  (The same applies to the call to C_GetMechanismInfo in PK11_PQG_ParamGenV2.)

2) A very minor nit.  In the new functions, keySize is defined as type unsigned long.  However, length is PK11_Verify is defined as type unsigned int and primeBits in PK11_PQG_ParamGenV2 is defined as CK_ULONG.  For consistency, why not just define legnth and keySize to be type CK_ULONG?

Dave

Comment 17

[Robert Relyea]

Thanks David. I'll make the monitor change. I think you are right, it's required. Softoken is threadsafe, so it wouldn't trigger an issue in normal testing.

PK11_PQG_ParamGenV2 got changed to unsigned int in the final checking for DSA-2 under wtc's comments. I was making it unsigned long for GetSlotMultiple because I was dealing with arrays and I was worried about passing in an unsigned int to a PKCS #11 template. It turns out we are just comparing it, so I'll change it to unsigned int to make them all consistent. It means that it may not always be the same size as CK_ULONG coming from the mechanism_info structure, but the likelihood of key sizes being as large as 4G is really extremely low;).

I'll go ahead and check in with those 2 changes.

Thanks for your review.

bob

Comment 18

[Robert Relyea]

Checking in crmf/cmmfchal.c;
/cvsroot/mozilla/security/nss/lib/crmf/cmmfchal.c,v  <--  cmmfchal.c
new revision: 1.7; previous revision: 1.6
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.220; previous revision: 1.219
done
Checking in pk11wrap/pk11obj.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.27; previous revision: 1.26
done
Checking in pk11wrap/pk11pqg.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pqg.c,v  <--  pk11pqg.c
new revision: 1.17; previous revision: 1.16
done
Checking in pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.41; previous revision: 1.40
done
Checking in pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.112; previous revision: 1.111
done

Comment 19

[Robert Relyea]

I'm keeping this bug open because I believe this patch was just a setup for addition changes from David. If that's not the case let me know and I'll code it.

bob

Comment 20

[Robert Relyea]

Checking in cmd/crmftest/testcrmf.c;
/cvsroot/mozilla/security/nss/cmd/crmftest/testcrmf.c,v  <--  testcrmf.c
new revision: 1.13; previous revision: 1.12
done
Checking in lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.113; previous revision: 1.112
done

Comment 21

[Robert Relyea]

Comment on attachment 637262 [details] [diff] [review]
Merge David's patch with current tip (checked in).

David verbally r+ the patch, clearing the ? flag

Comment 22

[Wan-Teh Chang]

Bob, is this bug fixed?

Comment 23

[Wan-Teh Chang]

Comment on attachment 552185 [details] [diff] [review]
Updated veriosn of D. Cooper's BestSlot patch

Marked this patch obsolete. All the changes in this patch
have been checked in (presumably as a new patch), except
the changes to lib/ssl/{derive.c,ssl3con.c}.

Comment 24

[Wan-Teh Chang]

Comment on attachment 634146 [details] [diff] [review]
Implementation of PK11_GetBestSlotMultipleKeySize_mechanism_flags

This patch is also obsolete. An updated version of it
has been checked in.

Comment 25

[Wan-Teh Chang]

(In reply to Robert Relyea from comment #19)
> I'm keeping this bug open because I believe this patch was just a setup for
> addition changes from David. If that's not the case let me know and I'll
> code it.

Bob: you wrote this on 2012-06-29. Since you didn't get additional
changes from David, I suggest that we mark this bug FIXED in NSS 3.14,
to reflect the patch "Merge David's patch with current tip" that you
checked in.

I don't think the other two patches are appropriate for checkin.

Comment 26

[Robert Relyea]

so we had decided in the NSS meeting to push out to 3.14.1 for the remaining patches, but on looking at them I think you are right. David, if you need them, go ahead and open up a new bug. Also, if you want any patches in, you need to mark them for review. Thanks,

bob