Open Bug 327773 Opened 15 years ago Updated 6 years ago

ECC code conspicuously absent from lib/pk11wrap/pk11pk12.c

Categories

(NSS :: Libraries, enhancement, P2)

3.11
enhancement

Tracking

(Not tracked)

People

(Reporter: nelson, Unassigned)

References

Details

(Whiteboard: unshrouded EC keys unsupported)

Attachments

(1 file)

The file lib/pk11wrap/pk11pk12.c contains numerous functions for importing
private keys.  It handles RSA, DH and DSA, but not ECC. 
There is code in JSS and elsewhere that uses these functions, and that code
should also work for ECC keys.
P1. I think we cannot ship NSS in products until this is fixed.
Priority: -- → P1
None of the existing tests in all.sh appear to exercise this
code or we would have spotted this oversight earlier.
Are there any other tests one could use to make sure
that the ECC patch (once it is created) is working as expected.

vipul
You are right. I put "PORT_Assert(0)" in PK11_ImportAndReturnPrivateKey and PK11_ImportPrivateKeyInfoAndReturnKey and ran all.sh, but it passed and I didn't get any core file. This is another hole in our test coverage.
Unfortunately, NSS's QA test coverage is inadequate. 
I ran across this by accident. 
Do our test scripts with ECC use PK12util to export and import EC certs/keys?

I know of products that use some of the functions in this source file,
that I expect will run into trubles if it is not enhanced at the same
time as the rest of NSS.  
IIRC pk12util uses this function under some circumstances.

I'm trying to think of a good way to find other candidate souce files for this inspection.
(In reply to comment #4)

> Do our test scripts with ECC use PK12util to export and import EC certs/keys?

   Yes and those tests (in ectools.sh or the merged tools.sh attached
   to bug Id 324887) pass.

   So we still need some test cases that would exercise the code in this file.
Adding Neil to the cc list.  
Neil may be able to help with the pkcs12 test coverage.

But my bigger worry is: how do we determine if there are any other places
where EC code is needed but missing?
(In reply to comment #6)
> Adding Neil to the cc list.  
> Neil may be able to help with the pkcs12 test coverage.
> 
> But my bigger worry is: how do we determine if there are any other places
> where EC code is needed but missing?

   I looked for all the files in NSS that made references to
   rsaKey, CKK_RSA and CKM_RSA and then looked for
   corresponding references to ecKey, CKK_EC, CKM_EC* etc.

   It appears that there are other places where ECC code is
   absent. However, it isn't clear to me if all of these places
   need to have ECC code, e.g. many of them do not mention
   DH or DSA either.

   Clearly, code in these files isn't being covered by the tests
   in all.sh. 

   Here are the files/directories I found:

   1. nss/cmd/crmftest/   (it is possible that bug 211235 will address this)
   2. nss/lib/crmf/  (again, 211235 might address this)
   3. nss/tests/pkcs11/netscape/suites/security/pkcs11/
   4. nss/cmd/symkeyutil/symkeyutil.c (has some references to ECC but only for ECDSA, not ECDH)
   5. nss/cmd/signtool/
   6. nss/cmd/pk11util/scripts/ 
   7. nss/lib/ckfw/ (this directory appears to have been added after the first ECC check-in)
   8. nss/lib/dev/
   9. nss/lib/pki/oids.txt  (who uses this?)
 10. nss/lib/pk11wrap/pk11pk12.c (the file that triggered this bug report)
 11. nss/lib/pk11wrap/pk11kea.c
 12. nss/lib/cryptohi/seckey.c  (has ECC code except in SECKEY_ImportDERPublicKey)

  Not knowing too much about who uses the code in these files, 
  1, 2, 10, 11, 12 seem to be the most important omissions. 
  I'd appreciate input from folks with greater NSS experience.

  vipul
   
(In reply to comment #7)
>    1. nss/cmd/crmftest/   (it is possible that bug 211235 will address this)
>    2. nss/lib/crmf/  (again, 211235 might address this)

  I should have said bug 326159 rather than 211235.

  vipul
Because the functions mentioned above in lib/pk11wrap/pk11pk12.c had no capability to handle EC algorithms there was some concern that our QA tests were not testing ECC properly. I investigated the pk12util QA tests with ECC turned (having applied the patches from bug 324887) and found that pk12util does not call the functions in pk11pk12.c because those are for unshrouded bags. Instead it calls functions in lib/pk11wrap/pk11akey.c which have ECC implemented.

The question remains whether unshrouded bags are supported and whether ECC keys work or not. I'll look into that next.
Lowering this bug (about ECC in PKCS12) to P2.  If this only affects 
unencrypted private keys in PKCS12 files, (which we do not knowingly produce)
then we wouldn't hold the release for this bug, IMO.

I think we need separate bugs for the other places in NSS that are missing
ECC support (from Vipul's list in comment 7) so that they may be individually
prioritized.  
Priority: P1 → P2
Whiteboard: ECC
QA Contact: jason.m.reid → libraries
The target milestone is already released. Resetting target milestone.
Target Milestone: 3.11.1 → ---
Blocks: FIPS2008
No longer blocks: FIPS2008
Assignee: vipul.gupta → nobody
Whiteboard: ECC → unshrouded EC keys unsupported
Folks, the user is complaining of this:

  [attached]
  news://news.mozilla.org:119/mailman.80.1302289621.20808.dev-tech-crypto@lists.mozilla.org
  http://groups.google.com/group/mozilla.dev.tech.crypto/msg/510ff2db8ae62b59
  http://old.nabble.com/Extracting-PrivateKey-from-PKCS8EncodedKeySpec-with-EC-tp31354778p31354778.html

And what is important:
  
  at p12 layer NSS has two routines

    PK11_ImportEncryptedPrivateKeyInfo
    PK11_ImportAndReturnPrivateKey

which both map "keyUsage" into CKA_*. Results of mapping differ and one of them is wrong. The map shall be encapsulated in a separate routine to avoid discrepancies.
You need to log in before you can comment on or make changes to this bug.