349609 - C_SignUpdate does not work for DSA key

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[glen beasley]

See bug 342582 test program pk11mode.c method PKM_PublicKey 

if you create a DSA key, C_SignInit and C_Sign works,
but C_SignInit, C_SignUpdate, and C_SignFinal does not with 
the same DSA key and data. 

The code first crashes in sftk_MACUpdate on 
context->padDataLength = ulPartLen % context->blockSize;
because context->blockSize is set to 0. Fixing blockSize
does not fix the problem, the code will then crash in 
C_SignFinal. 

Code to reproduce in PKM_PublicKey

   /* Sign with DSA (works)
    crv = pFunctionList->C_SignInit(hRwSession, &dsaMech, hDsaPrivKey);
    if (crv != CKR_OK) {
        PKM_Error( "C_SignInit failed with 0x%08lX\n", crv);
        return crv;
    }
    dsaSigLen = sizeof(dsaSig);
    crv = pFunctionList->C_Sign(hRwSession, sha1Digest, sha1DigestLen,
                                dsaSig, &dsaSigLen);
    if (crv != CKR_OK) {
        PKM_Error( "C_Sign failed with 0x%08lX\n", crv);
        return crv;
    }
*/

    /* SignUpdate with DSA  does not work */
    crv = pFunctionList->C_SignInit(hRwSession, &dsaMech, hDsaPrivKey);
    if (crv != CKR_OK) {
        PKM_Error( "C_SignInit failed with 0x%08lX\n", crv);
        return crv;
    }
    dsaSigLen = sizeof(dsaSig);
    crv = pFunctionList->C_SignUpdate(hRwSession, sha1Digest, sha1DigestLen);
    if (crv != CKR_OK) {
        PKM_Error( "C_Sign failed with 0x%08lX\n", crv);
        return crv;
    }
    crv = pFunctionList->C_SignFinal(hRwSession, (CK_BYTE_PTR)dsaSig, &dsaSigLen);
    if (crv == CKR_OK) {
        PKM_LogIt("C_SignFinal succeeded\n");
    } else {
        PKM_Error( "C_SignFinal failed with 0x%08lX\n", crv);
        return crv;
    }

Comment 1

[Wan-Teh Chang]

I found that CKM_DSA is single-part operations only.
So C_SignUpdate shouldn't be used with CKM_DSA.  The
only thing we can do is to make C_SignUpdate return
an error code rather than crash.  It may be as simple
as passing needMulti=PR_TRUE instead of PR_FALSE to
sftk_GetContext in sftk_MACUpdate. I'm not sure if
CKR_OPERATION_NOT_INITIALIZED is the right error code
but I can't find a better error code.

Glen, you can use CKM_DSA_SHA1 with C_SignUpdate +
C_SignFinal.  You can look at how I used CKM_DSA_SHA1
with C_VerifyUpdate + C_SignFinal.

Comment 2

[Wan-Teh Chang]

The last paragraph in my previous comment should read:

Glen, you can use CKM_DSA_SHA1 with C_SignUpdate +
C_SignFinal.  You can look at how I used CKM_DSA_SHA1
with C_VerifyUpdate + C_VerifyFinal.

Comment 3

[Wan-Teh Chang]

Attachment: Fix the crash in sftk_MACUpdate

Glen, does this patch fix the crash in sftk_MACUpdate?

Comment 4

[Wan-Teh Chang]

Comment on attachment 235013 [details] [diff] [review]
Fix the crash in sftk_MACUpdate

I verified that this patch allows C_SignUpdate and
C_SignFinal with the CKM_DSA mechanism to fail with
CKR_OPERATION_NOT_INITIALIZED rather than crashing.

I found that C_SignFinal doesn't terminate an active
CKM_DSA single-part operation.  C_SignFinal fails with CKR_OPERATION_NOT_INITIALIZED, and the CKM_DSA operation
is still active.  Is this the correct behavior, Bob?

Comment 5

[glen beasley]

Comment on attachment 235013 [details] [diff] [review]
Fix the crash in sftk_MACUpdate

verified C_SignUpdate and
C_SignFinal with the CKM_DSA mechanism fails with
CKR_OPERATION_NOT_INITIALIZED rather than crashing.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Glen, you marked this bug "assigned".  
I think you meant to assign it to yourself.
So I'm reassigning it to you.

Comment 7

[Robert Relyea]

Comment on attachment 235013 [details] [diff] [review]
Fix the crash in sftk_MACUpdate

r=relyea

Comment 8

[Wan-Teh Chang]

I checked in the fix on the NSS trunk (3.12) and NSS_3_11_BRANCH (3.11.3).

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.89; previous revision: 1.88
done

Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.68.2.19; previous revision: 1.68.2.18
done