189345 - pk11_Finalize sometimes fails to finalize context

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Jamie Nicolson]

In PK11_VerifyMechanism, we call PK11_CipherOp, which in turn calls
pk11_Finalize. pk11_Finalize first calls C_EncryptFinal with a NULL buffer to
get the output count, and then, if the count is nonzero, calls C_EncryptFinal
again with a non-NULL buffer to get the final output bytes. If the first call to
C_EncryptFinal tells us that the output count is zero, we do not call
C_EncryptFinal again.

The problem is that nCipher requires us to call C_EncryptFinal with a non-NULL
buffer before they consider the encryption context to be finalized. If we later
try to start a new encryption context with C_EncryptInit, it fails with
CKR_OPERATION_ACTIVE.  Their point of view seems to be justified by the PKCS #11
spec, in the documentation for C_EncryptFinal: "A call to C_EncryptFinal always
terminates the active encryption operation unless it returns
CKR_BUFFER_TOO_SMALL or is a successful call (i.e., one which returns CKR_OK) to
determine the length of the buffer needed to hold the ciphertext."

We could fix this by always calling C_EncryptFinal with a non-NULL buffer, even
if there are no bytes left to pick up.

Comment 1

[Wan-Teh Chang]

The target milestone will be changed to 3.7.2 when
Bugzilla has it.

Comment 2

[Jamie Nicolson]

Attachment: proposed patch

This is a patch I used to fix the problem. With this patch, we will call
C_EncryptFinal even if the number of bytes available is zero.

Comment 3

[Wan-Teh Chang]

Comment on attachment 111859 [details] [diff] [review]
proposed patch

r=wtc. Bob, could you review this patch?

I'm wondering if any token would fail now
that we may be calling C_EncryptFinalize
twice.

Comment 4

[Robert Relyea]

Comment on attachment 111859 [details] [diff] [review]
proposed patch

Sigh. I'm a little worried because I know that our internal token will return
an error on the second finalize call because it did the finalize on the first
call because there was not data to return... (sigh). This may return spurious
errors.

Also Wan-Teh pointed out the fixed buffer we are using. This may fail for
C_SignFinal operations on RSA keys.

Comment 5

[Jamie Nicolson]

See also bug 189546.

Comment 6

[Wan-Teh Chang]

Attachment: proposed patch 2

I suggest that we also fix the bug that C_SignFinal
on RSA keys may fail if stackBuf is too small.	This
patch allocates a buffer from the heap if stackBuf is
too small and frees the heap-allocated buffer before
returning from the buffer.  It also uses stackBuf if
count is equal to sizeof stackBuf.

Please review this patch.

Comment 7

[Robert Relyea]

Comment on attachment 112170 [details] [diff] [review]
proposed patch 2

looks good r=relyea

Comment 8

[Wan-Teh Chang]

*** Bug 189545 has been marked as a duplicate of this bug. ***

Comment 9

[Wan-Teh Chang]

The patches have been checked in on the NSS
trunk (3.8) and NSS_3_7_BRANCH (3.7.1).