283654 - libsmime fails to finalize crypto contexts before destroying session

Status: RESOLVED INVALID

(NSS :: Libraries, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

Antonio Andres Espallardo <aae1 (at) alu.um.es> reported the NSS does not 
finalize decryption operations before destroying the session on which they
are performed.

In file nss/lib/smime/cmscipher.c we see that the following calls:
NSS_CMSCipherContext_StartEncrypt and
NSS_CMSCipherContext_StartDecrypt each call PK11_CreateContextBySymKey()
  and save the addresses of PK11_CipherOp and PK11_DestroyContext
  in their own NSSCMSCipherContext.
NSS_CMSCipherContext_Decrypt      calls PK11_CipherOp, repeatedly, through 
  the pointer in NSSCMSCipherContext.
NSS_CMSCipherContext_Destroy      calls PK11_DestroyContext through the 
  pointer in NSSCMSCipherContext.

But this code never calls PK11_Finalize.  

I think the solution is have NSS_CMSCipherContext_Destroy PK11_Finalize
before calling PK11_DestroyContext.  It may be desirable to add another
pointer to the SMIME context for that Finalize function, and have the two
Begin functions each set that pointer.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Please fix this for NSS 3.11.

Comment 2

[Christophe Ravel]

The target milestone is already released. Resetting target milestone.

Comment 3

[Konstantin Andreev]

I am working on another bug in cmscipher.c (CMSCipher crashes if cipher is a stream cipher).

I can address this bug conjointly.

This bug is usually unnoted by users, because CMS en/decryption tries to proceed in a distinct PKCS#11 session. But this bug will get into a mess, if particular token is lack of sessions.

But I am confused by specific implementation detail. NSSCMSCipherContext keeps pointers to PK11_CipherOp() [doit] and to PK11_DestroyContext [destroy] and calls PK11_* via pointers.

I do not understand, what the sense in this fuss. Why not to call PK11_* derectly?

Comment 4

[Robert Relyea]

> I do not understand, what the sense in this fuss. Why not to call PK11_*
> derectly?

It's almost certainly an historical dreg left over from the old PKCS #7 code. In that code the pointer to the individual crypto implementations were put in the pointer. The conversion (circa 1996) to using PK11 wrappers simply coopted the pointer.

If there is any possible justification for keeping those pointers it would be for someone who has grafted the CMS library onto another crypto library other than NSS. 

If you want to drop the indirection, I would be happy to review the patch.

bob

Comment 5

[Konstantin Andreev]

Finalizing crypto-op is two-fold:

  1) it gets final data from operation (C_{En,De}cryptFinal).
  2) it frees PK11 session for a new operation of the same type

As concerns (1), CMSCipher is not designed to work with ciphers which have final part. Thus, discarding final part is consistent.

As concerns (2), the [NSS_CMSCipherContext_Destroy( cc ) -> PK11_DestroyContext( cc->cx )] does the work:

    if PK11Context owns underlying PKCS#11 session, then session is closed together with crypto operations it encompass. => no need to Final() ciphering operation.

    if PK11Context doesn't own session, i.e. borrows slot session, then 'pk11wrap' level cares about cleaning alien session (to the moment, 'pk11wrap' does this wrong, but it is a subject for another bug).