415799 - Create interface+implementation for HMAC support

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Honza Bambas (:mayhemer)]

Attachment: Adding support for HMAC through interface

This shall be very close to the way how nsICryptoHash is designed and implemented. I am extending this interface just of reset() method that reinitialize the HMAC context to be used with the same setting on different data.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

moa=biesi for the netwerk/ changes in that patch.


You should probably remove the initWithString mention from the IDL comment, since this interface doesn't have such a function :)

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to comment #1)
> moa=biesi for the netwerk/ changes in that patch.
> 
> 
> You should probably remove the initWithString mention from the IDL comment,
> since this interface doesn't have such a function :)
> 

Thanks for the hit :) I will change it after regular review.

Comment 3

[Dave Camp (:dcamp)]

We need this for bug 360387, which is blocking.

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 301538 [details] [diff] [review]
Adding support for HMAC through interface

Looks mostly good, but I'd like to request some changes.

In addition it would be good if Bob Relyea could do a quick scan over the PK11 function calls.


>+     * @param aKeyData the raw key data used fot the HMAC
>+     *        computation

nit, typo: for


>+NS_IMETHODIMP nsCryptoHMAC::UpdateFromStream(nsIInputStream *aStream, PRUint32 aLen)
>+{
>+  PRUint32 read;
>+  
>+    rv = aStream->Read(buffer, read, &read);

This looks dangerous. The function could assume both variables are different and might read/write to them in any order. Please use two different variables for input and output, unless you can show the API documentation for this function call explicitly allows that use.


>+NS_IMETHODIMP nsCryptoHMAC::Finish(PRBool aASCII, nsACString & _retval)
>+{
>+    char *asciiData = BTOA_DataToAscii(buffer, hashLen);
>+    _retval.Assign(asciiData);

Is it safe to call Assign(0) ?
But I think if asciiData is null, you should return out_of_memory.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to comment #4)
> (From update of attachment 301538 [details] [diff] [review])
> >+NS_IMETHODIMP nsCryptoHMAC::UpdateFromStream(nsIInputStream *aStream, PRUint32 aLen)
> >+{
> >+  PRUint32 read;
> >+  
> >+    rv = aStream->Read(buffer, read, &read);
> 
> This looks dangerous. The function could assume both variables are different
> and might read/write to them in any order. Please use two different variables
> for input and output, unless you can show the API documentation for this
> function call explicitly allows that use.
> 

This is copied from nsCryptoHash. I will fix this on next review round in both components, but personally, formerly the pointer (&read) is copied to the stack than value of read is copied. read its self is modified inside later after the original value is copied to that stack before method call. IMO this is not a problem, unless the API change (what I don't believe because of @status FROZEN of the interface).

> 
> >+NS_IMETHODIMP nsCryptoHMAC::Finish(PRBool aASCII, nsACString & _retval)
> >+{
> >+    char *asciiData = BTOA_DataToAscii(buffer, hashLen);
> >+    _retval.Assign(asciiData);
> 
> Is it safe to call Assign(0) ?
> But I think if asciiData is null, you should return out_of_memory.
> 

I will fix this. This is also copied from nsCryptoHash.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: Adding support for HMAC through interface, v2

Fixed according to comment 4 and added test for UpdateFromStream into nsCryptoHash test (there were none!).

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: Adding support for HMAC through interface, v2.1 (added missing test)

Same as before, just added missing test modification.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 305741 [details] [diff] [review]
Adding support for HMAC through interface, v2.1 (added missing test)

Looks good to me, thanks for addressing my comments.

Comment 10

[Dave Camp (:dcamp)]

Comment on attachment 305741 [details] [diff] [review]
Adding support for HMAC through interface, v2.1 (added missing test)

Bob, we need to check this in today, are you able to take a look at this?

Comment 11

[Robert Relyea]

Comment on attachment 305741 [details] [diff] [review]
Adding support for HMAC through interface, v2.1 (added missing test)

r+ with 2 caveats.

caveat 1 - minor issue. The last parameter to PK11_CreateContextBySymKey should be NULL for HMACs. The parameter is the parameter for the PKCS #11 function, which for HMAC is empty. Please fix this one before checkin.

Caveat 2: This one is much more major, but it affects the API.

The PK11_ImportSymKey can and will fail on most FIPS devices (including our own FIPS module). The problem is FIPS modules do not allow import or export of plain-text keys. This means any crypto interface that uses plain-text keys will not support only of these modes/devices. Exporting such an API can get your own customers in trouble (just like my allowing it is potentially getting you in trouble).

Ideally you need to design an API where keys are exported as real objects. There may actually be such an object in the mozilla crypto world for symmetric operations.

Anyway you should consider modifying this api to take a key, then you can set the key to some plaintext key (still won't work in fips mode), but you have an API that could in the future move to something that can handle real tokens.


The rest of the code, and use of NSS objects is fine. good work Honzab.

bob

Comment 12

[Dave Camp (:dcamp)]

Attachment: v2.2

Updated to fix the first caveat.  After some discussion with rrelyea, I think we should land this patch as it is so we can land 360387, and leave this bug open to fix the API issue.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 305741 [details] [diff] [review]
Adding support for HMAC through interface, v2.1 (added missing test)

Minor license issue:

>+++ netwerk/base/public/nsICryptoHMAC.idl	2008-02-26 13:07:31.085799400 +0100
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2001

This appears to be based on nsICryptoHash.idl, it should use the creator and date of that file  (dougt/2005) rather than this old Netscape header.


>+#define NS_CRYPTO_HMAC_CLASSNAME "Mozilla Cryto HMAC Function Component"

"Crypto" (I guess NS_CRYPTO_HASH_CLASSNAME needs fixing, too)

sr=dveditz

Comment 14

[Dave Camp (:dcamp)]

(In reply to comment #11)
> (From update of attachment 305741 [details] [diff] [review])
> r+ with 2 caveats.
> 
> caveat 1 - minor issue. The last parameter to PK11_CreateContextBySymKey should
> be NULL for HMACs. The parameter is the parameter for the PKCS #11 function,
> which for HMAC is empty. Please fix this one before checkin.

honzab's helpful unit tests caught that this change breaks things.  I verified with http://www.mozilla.org/projects/security/pki/nss/tech-notes/tn5.html that this is supposed to be a zero'ed out SECItem struct and reverted that change.

Comment 15

[Dave Camp (:dcamp)]

Attachment: patch as landed

I landed the attached patch.  I'll let rrelyea and honzab decide if they want to continue work on this bug or open a new one.

Checking in netwerk/base/public/Makefile.in;
/cvsroot/mozilla/netwerk/base/public/Makefile.in,v  <--  Makefile.in
new revision: 1.127; previous revision: 1.126
done
RCS file: /cvsroot/mozilla/netwerk/base/public/nsICryptoHMAC.idl,v
done
Checking in netwerk/base/public/nsICryptoHMAC.idl;
/cvsroot/mozilla/netwerk/base/public/nsICryptoHMAC.idl,v  <--  nsICryptoHMAC.idl
initial revision: 1.1
done
Checking in netwerk/build/nsNetCID.h;
/cvsroot/mozilla/netwerk/build/nsNetCID.h,v  <--  nsNetCID.h
new revision: 1.71; previous revision: 1.70
done
Checking in security/manager/ssl/src/nsNSSComponent.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v  <--  nsNSSComponent.cpp
new revision: 1.159; previous revision: 1.158
done
Checking in security/manager/ssl/src/nsNSSComponent.h;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.h,v  <--  nsNSSComponent.h
new revision: 1.51; previous revision: 1.50
done
Checking in security/manager/ssl/src/nsNSSModule.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSModule.cpp,v  <--  nsNSSModule.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in security/manager/ssl/tests/test_hash_algorithms.js;
/cvsroot/mozilla/security/manager/ssl/tests/test_hash_algorithms.js,v  <--  test_hash_algorithms.js
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/security/manager/ssl/tests/test_hmac.js,v
done
Checking in security/manager/ssl/tests/test_hmac.js;
/cvsroot/mozilla/security/manager/ssl/tests/test_hmac.js,v  <--  test_hmac.js
initial revision: 1.1
done

Comment 16

[Brendan Eich [:brendan]]

One landed patch per bug is best -- more patching, file followups. I've learned this the hard way.

/be

Comment 17

[Dave Camp (:dcamp)]

419794 filed.