211546 - PK11_ImportEncryptedPrivateKeyInfo does nothing when isPerm is false

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

PK11_ImportEncryptedPrivateKeyInfo unwraps an encrypted private key, 
creating a new private key object in the token, and a new SECKEYPrivateKey 
struct to represent it outside of the token.  Then
PK11_ImportEncryptedPrivateKeyInfo calls SECKEY_DestroyPrivateKey, which 
destroys the underlying private key object ONLY if it is a "session" object,
which is to say, only if it is not a "permanent" "token" object.

So, the net result is that if you call PK11_ImportEncryptedPrivateKeyInfo
to import an encrypted private key, with isPerm set to false, the function
accomplishes nothing.  

If there is a way to create the session object without leaking a reference,
PK11_ImportEncryptedPrivateKeyInfo should do that.  Otherwise, it ought to
assert that isPerm is true.  

We probably also should create a function like
PK11_ImportEncryptedPrivateKeyInfoAndReturnKey, which returns the
SECKEYPrivateKey, and hence is meaningful for session objects.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

This implements Nelson's suggestions: assert isPerm is true in
PK11_ImportEncryptedPrivateKeyInfo and add a
PK11_ImportEncryptedPrivateKeyInfoAndReturnKey function.

I'd prefer that PK11_ImportEncryptedPrivateKeyInfoAndReturnKey
returns SECKEYPrivateKey * rather than using an output parameter,
but I found two existing PK11_ImportXXXAndReturnKey functions
and they both use an output parameter, so I think the new
function should be consistent.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch v2

I forgot to export the new function in nss.def.

Comment 3

[Matt Mueller]

Comment on attachment 571524 [details] [diff] [review]
Proposed patch v2

Tested by patching into NSS and using the new function instead of my workaround.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 571524 [details] [diff] [review]
Proposed patch v2

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

r=bsmith

I think the comment is confusing. Instead of "It cannot create a session object without leaking a reference" I would write "If it were to create a session object that could later be looked up by its nickname, it would leak a reference." Also, I think this comment would be better in the header file or above the definition, instead of in the function body, so that it is more easily found by people thinking to use this function.

I would expect that if this function were to return SECSuccess, then I would be able to look up the key by the nickname. Since I would not be able to do so when isPerm is false, wouldn't it make more sense to return SECFailure (SEC_ERROR_INVALID_ARGS) in that case? If we cannot make that change for compatibility reasons, then I guess it is OK to return SECSuccess.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch v3

bsmith: thank you for the review.  I made all the changes you
suggested.  I verified that all the PK11_ImportEncryptedPrivateKeyInfo
calls in NSS pass isPerm=PR_TRUE, so we can make the function fail
with SEC_ERROR_INVALID_ARGS if isPerm is false.

Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.216; previous revision: 1.215
done
Checking in lib/pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.34; previous revision: 1.33
done
Checking in lib/pk11wrap/pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.37; previous revision: 1.36
done