A multi-part cipher operation (encryption or decryption) in the SunPKCS11 security provider may thrown an exception leaking a Session to the Session Manager with an active multi-part operation (CKR_OPERATION_ACTIVE). When a service gets this Session from the Session Manager, any use of it may fail. I've triggered this bug using the NSS Software Token as a backend for SunPKCS11.

There are 4 scenarios in which the previous leak can occur:

 1) P11Cipher::implUpdate(byte[], int, int, byte[], int, int)

 2) P11Cipher::implUpdate(ByteBuffer, ByteBuffer)

 3) P11Cipher::implDoFinal(byte[], int, int)

 4) P11Cipher::implDoFinal(ByteBuffer)

We should cancel any ongoing operation upon a failure, previous to returning the Session to the Session Manager. The rationale here is that a failed multi-part operation won't be resumed. Cancelling -which is achieved by means of a C_EncryptFinal/C_DecryptFinal call- should only incur on a performance cost if the Token requires explicit cancel (see 'explicitCancel' in SunPKCS11 configuration). However, this extra-cost is paid only in an exception -and already slow- execution path; it's not the hot path. The benefit is that we can re-use the Session for a different purpose later.

A different approach would be to keep the Session unreleased and the P11Cipher internal state as 'initialized', so calling P11Cipher::reset(doCancel = true) from P11Cipher::implInit would effectively cancel the operation before use. I don't see any a strong reason for a retry, though; I don't expect a P11Cipher instance to be used again for the same operation after a PKCS11 failure. Please note that ShortBufferException is checked before making a PKCS11 call.

How the proposed change affects the Cipher::doFinal and Cipher::update API specifications?

The proposed change is consistent with Cipher::doFinal because it says:

'Upon finishing, this method resets this cipher object to the state it was in when previously initialized via a call to {@code init}. That is, the object is reset and available to encrypt or decrypt (depending on the operation mode that was specified in the call to {@code init}) more data. Note: if any exception is thrown, this cipher object may need to be reset before it can be used again.'

Please note that 'explicit reset' is optional and that being able to use the cipher again is an expectation after calling this method.

In regards to Cipher::update, the only case that I see to continue the operation is when a ShortBufferException is thrown. But this is not modified with the proposed change because such checked is previous to calling P11Cipher::reset (so the P11Cipher instance will remain initialized and holding the Session to continue the multi-part operation). When getting a PKCS11 exception, we check against short buffers (CKR_BUFFER_TOO_SMALL) but that should be redundant and not executed: we keep track of block sizes in the Java side to prevent a costly failure (having to call PKCS11 to realize that the buffer is small).