150704 - PK11_Finalize doesn't do anything

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ian McGreer]

In the course of testing the patches in bug #145322, I produced a crash.  I
determined this crash was a result of sending a NULL buffer to NSC_SignFinal,
which attempts to memcpy into it (first problem).

It was my patch that instigated the call to NSC_SignFinal.  This is because my
path calls PK11_Finalize in an attempt to shut down a context's session, so that
the session can be used for more operations later.  However, PK11_Finalize
merely calls C_XXXFinal for the in-progress operation, with a NULL buffer. 
According to cryptoki, this will not actually shut down the in-progress
operation, but will put the length of the final data into the last parameter
(problem 2).

I think PK11_Finalize should handle the case where the context is being
destroyed, but the operation is still in progress.  In that case, the C_XXXFinal
operations will return a non-zero length.  Unfortunately, it seems the only way
to completely finalize the operation is to then provide a buffer of that length,
and call C_XXXFinal again. I think in most cases that can be done with a stack
buffer.

Comment 1

[Ian McGreer]

Ok, now I'm debating what the correct solution to problem 2 is.  Bob, focusing
on pk11_Finalize (since that is a private function, more easy to change), what
do you think is best?:

1.  As described above, try to force the finalization by passing a buffer to
C_XXXFinal, even if that means allocation.

2.  Return SECFailure if the session's state length > 0.  Callers would have to
decide whether to delete the session (C_CloseSession), or keep it.  In the case
of this bug, we would choose to delete it and open a new one.

Comment 2

[Ian McGreer]

Attachment: attempt to make C_XXXFinal operations work

This patch is a hybrid of the two proposals made in the last comment.  It
attempts to make both pk11wrap and softoken work according to the PKCS#11
standard for C_XXXFinal calls.

First, I went through all NSC_XXXFinal functions and tried to handle the case
where a NULL output buffer is passed.  According to the standard, the function
should then return a value which is at least greater than the amount of
remaining data.  This is a straightforward bugfix.

I also made changes to PK11_Finalize that were more difficult than I had
anticipated.  This function is an exposed API function, so I had to be careful
with it.

PK11_Finalize purports to "finalize" a context, making it available for new
operations.  This implies finishing any multipart crypto operations in progress
within the context's session.  However:

1)  If the C_XXXFinal call returns CKR_OPERATION_NOT_ACTIVE, PK11_Finalize
returns SECFailure.  I consider this a bug.  If an operation is not active,
then the session is ready to go.  I changed the function to return SECSuccess
in this case.

2)  PK11_Finalize was calling C_XXXFinal with a NULL buffer.  As noted above,
in the worst case, this causes crashes because the softoken could not handle
that case.

3)  Even if the softoken behaved correctly, this is essentially a no-op.  A
count of remaining bytes is returned, but the operation is not finalized (this
according to the standard) unless the count == 0.  I added a stack buffer of
256 bytes.  If the count 0 < count <= 256, a call to C_XXXFinal is made again,
this time with the stack buffer.  This should allow the operation to finish
(the output is ignored), making the session available for new operations.

However, if count > 256, PK11_Finalize returns SECFailure.  I made this
decision, because at some point, if there's too much work required to finalize
the session, it probably isn't worth it, and the caller should just get a new
one.

Is this the optimal behavior?

I've found that this case occurs often in SSL connections.  I need to look into
it more, but why are we using sessions for multipart crypto operations without
grabbing the last part?  That seems strange.

Comment 3

[Ian McGreer]

> I've found that this case occurs often in SSL connections.  I need to look into
> it more, but why are we using sessions for multipart crypto operations without
> grabbing the last part?  That seems strange.

OK, that assessment is incorrect.  This was occuring because of a bug in my
working patch, which was fixed in the patch shown here.  After running the tests
with the correct patch, I found that this case *never* occured.  That is, at the
time of the call to PK11_Finalize, there is no data remaining in the session.  I
feel better about my patch now :)

Comment 4

[Ian McGreer]

Making P1 3.6 because this blocks bug 145322.

Bob, can you review?

Comment 5

[Ian McGreer]

Attachment: rev 2 of same patch

There was an unneeded assert left in the last patch.

Bob, can you review?  This is still blocking bug 145322.

Comment 6

[Ian McGreer]

patch checked in.

Comment 7

[Ian McGreer]

marking fixed.