283642 - pk11wrap functions pass invalid key handle to modules

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Presently, NSS is known to occasionally call PKCS11 functions with 
invalid object handles.  These fall into two categories:
a) passing CK_INVALID_HANDLE 
b) passing a handle of an object that was once valid but is no longer.

Sun would like to eliminate all such occurrences from NSS.  
Passing invalid handles is wrong, wastes time, and may cause other problems.

Today I found 3 places where this occurs, in functions PK11_VerifyRecover,
PK11_Verify, and pk11_PubEncryptRaw (which is called from PK11_PubEncryptRaw
and PK11_PubEncryptPKCS1).  All these functions call PK11_ImportPublicKey
to import the public key as a new session key, and then take no notice of
whether the import succeeded or failed.  If it failed, they then pass
CK_INVALID_HANDLE to the module's C_VerifyRecoverInit, C_VerifyInit, or
C_EncryptInit function.  

I propose to change these functions to detect failure to import the key,
and then to set error SEC_ERROR_BAD_KEY, free the slot reference (if 
approrpiate) and return without first attempting the Verify or Encrypt 
Init operations.

Patch forthcoming later today.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

Bob, please review.

Comment 2

[Robert Relyea]

Passing invalid handles are not necessarily 'wrong'. PKCS #11 modules that can't
handle them are improperly implemented.

Passing deleted handles (which is a point of a previous bug) is another matter,
and definately needs to be fixed.

However I do agree we should be friendly as we can and do not object to this
patch (other then my review comments I'll make shortly).

I do worry if the tests you add are actually tripped over in real life. It means
that we are not able to import the key into the PKCS #11 module, which (again)
indicates a problem with the PKCS #11 module.

bob

Comment 3

[Robert Relyea]

Comment on attachment 175560 [details] [diff] [review]
patch v1

The only thing wrong with the patch is the test.

We should test if id is equal to CK_INVALID_HANDLE. pk11wrap is pretty careful
not to assume CK_INVALID_HANLDE is actually '0' elsewhere.

With that change you can assume an r+ from me for the patch.

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #2)
> ... if the tests you add are actually tripped over in real life. It means
> that we are not able to import the key into the PKCS #11 module, which (again)
> indicates a problem with the PKCS #11 module.

Key imports fail when the module correctly determines the keys to be invalid.
Recently someoen got an SSL server cert with a 2 k-bit DSA prime.  NSS refused
to import it, properly IMO.  

I'll change the patch to use CK_INVALID_HANDLE and check it in.

Comment 5

[Robert Relyea]

Oh, OK, I'm less worried that we are pasting over another problem then.

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2

checked in.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11obj.c,v  <--  pk11obj.c
new revision: 1.5; previous revision: 1.4