Closed Bug 415799 Opened 15 years ago Closed 14 years ago

Create interface+implementation for HMAC support

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #301538 - Flags: review?(kengert)
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 :)
(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.
Blocks: 360387
We need this for bug 360387, which is blocking.
Flags: blocking1.9?
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.
Attachment #301538 - Flags: review?(rrelyea)
Attachment #301538 - Flags: review?(kengert)
Attachment #301538 - Flags: review-
Duplicate of this bug: 328891
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta4
(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.
Fixed according to comment 4 and added test for UpdateFromStream into nsCryptoHash test (there were none!).
Attachment #301538 - Attachment is obsolete: true
Attachment #305740 - Flags: review?(dveditz)
Attachment #301538 - Flags: review?(rrelyea)
Same as before, just added missing test modification.
Attachment #305740 - Attachment is obsolete: true
Attachment #305741 - Flags: review?(dveditz)
Attachment #305740 - Flags: review?(dveditz)
Attachment #305741 - Flags: review?(dveditz) → superreview?(dveditz)
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.
Attachment #305741 - Flags: review+
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?
Attachment #305741 - Flags: review?(rrelyea)
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
Attachment #305741 - Flags: review?(rrelyea) → review+
Attached patch v2.2 (obsolete) — Splinter Review
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.
Attachment #305741 - Attachment is obsolete: true
Attachment #305911 - Flags: superreview?(dveditz)
Attachment #305741 - Flags: superreview?(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
Attachment #305741 - Flags: superreview+
Attachment #305911 - Flags: superreview?(dveditz) → superreview+
Attachment #305741 - Flags: superreview+
(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.
Attached patch patch as landedSplinter Review
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
Attachment #305911 - Attachment is obsolete: true
One landed patch per bug is best -- more patching, file followups. I've learned this the hard way.

/be
Depends on: 419794
419794 filed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.