349632 - C_VerifyUpdate fails for hmac

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[glen beasley]

See bug 342582 for test program pk11mode.c method PKM_Hmac

C_VerifyInit and C_Verify works, but 
C_VerifyInit, C_VerifyUpdate, and C_VerifyFinal does not work.

Comment 1

[Wan-Teh Chang]

Looking at the source code, I think it's actually the opposite:
it is C_Verify that fails for HMAC.  If context->hashInfo is
not NULL, I think C_Verify needs to call context->hashUpdate
and context->end and then pass the output to context->verify.
I believe NSS only calls C_VerifyUpdate and C_VerifyFinal for
HMAC, so C_Verify has never been used with HMAC.

Comment 2

[Wan-Teh Chang]

Attachment: Support HMAC in NSC_Verify

This should make NSC_Verify behave the same as
NSC_VerifyUpdate and NSC_VerifyFinal for HMAC.

Comment 3

[Wan-Teh Chang]

Comment on attachment 235019 [details] [diff] [review]
Support HMAC in NSC_Verify

I verified that this patch is necessary to make
C_Verify work with CKM_DSA_SHA1.  I didn't have
the setup to test C_Verify with HMAC.

Comment 4

[glen beasley]

Comment on attachment 235019 [details] [diff] [review]
Support HMAC in NSC_Verify

C_Verify still needs some
work, this patch did show
that sftk_HMACCmp also needs a fix. 

 sftk_HMACCmp(CK_ULONG *copyLen,unsigned char *sig,unsigned int sigLen,
                                unsigned char *hash, unsigned int hashLen)
 {
-    return PORT_Memcmp(sig,hash,*copyLen) ? SECSuccess : SECFailure ;
+    return (PORT_Memcmp(sig,hash,*copyLen) == 0) ? SECSuccess : SECFailure ;
 }

Comment 5

[glen beasley]

Attachment: added to wan-teh fix

verified this patch works for both C_Verify and C_VerifyUpdate/C_VerifyFinal
for CKM_SHA_1_HMAC, CKM_SHA256_HMAC, CKM_SHA384_HMAC, and CKM_SHA512_HMAC.

Comment 6

[Wan-Teh Chang]

Comment on attachment 235052 [details] [diff] [review]
added to wan-teh fix 

r=wtc. Good catch, Glen.  I also verified that all
the other PORT_Memcmp calls in this file are correct.

Comment 7

[Robert Relyea]

Comment on attachment 235052 [details] [diff] [review]
added to wan-teh fix 

r+, Because this is an improvement, but the patch is incomplete. Encrypted Mac will still fail.

Instead of context->hashInfo, we should check for context->multi and call
sftk_MACUpdate
Followed by
NSC_VerifyFinal
return the results of verify Final after cleaning up.

Comment 8

[glen beasley]

Today I check in the fix for sftk_HMACCmp only to 3_11 branch and the trunk. I will implement bob suggestion, and request another review for NSC_Verify. 

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.68.2.17; previous revision: 1.68.2.16
done

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.87; previous revision: 1.86
done

Comment 9

[Wan-Teh Chang]

Attachment: Implement Bob's suggestion

I implemented Bob's suggestion.  I emulated how NSC_Sign
handles multi-part signing.

Bob, why don't we need to call sftk_FreeContext(context)
inside the if (context->multi) block?

Comment 10

[Wan-Teh Chang]

Comment on attachment 235309 [details] [diff] [review]
Implement Bob's suggestion

I forgot to mention that I tested this patch with both
CKM_DSA and CKM_DSA_SHA1.

Comment 11

[glen beasley]

Comment on attachment 235309 [details] [diff] [review]
Implement Bob's suggestion

tested successfully.

Comment 12

[Robert Relyea]

Comment on attachment 235309 [details] [diff] [review]
Implement Bob's suggestion

r= relyea

Answer to why you need to free the sesssion: Because you are holding a reference to it.

(see comment on sft_GetContext()).

Comment 13

[Robert Relyea]

Comment on attachment 235309 [details] [diff] [review]
Implement Bob's suggestion

r= relyea

Answer to why you need to free the sesssion: Because you are holding a reference to it.

(see comment on sft_GetContext()).

Comment 14

[Wan-Teh Chang]

I checked in the patch on the NSS trunk (3.12) and the
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.88; previous revision: 1.87
done

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