Create interface+implementation for HMAC support

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
mozilla1.9beta4
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 301538 [details] [diff] [review]
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.
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 :)
(Assignee)

Comment 2

10 years ago
(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.

Updated

10 years ago
Blocks: 360387

Comment 3

10 years ago
We need this for bug 360387, which is blocking.
Flags: blocking1.9?

Comment 4

10 years ago
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-

Updated

10 years ago
Duplicate of this bug: 328891

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta4
(Assignee)

Comment 6

10 years ago
(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.
(Assignee)

Comment 7

10 years ago
Created attachment 305740 [details] [diff] [review]
Adding support for HMAC through interface, v2

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)
(Assignee)

Comment 8

10 years ago
Created attachment 305741 [details] [diff] [review]
Adding support for HMAC through interface, v2.1 (added missing test)

Same as before, just added missing test modification.
Attachment #305740 - Attachment is obsolete: true
Attachment #305741 - Flags: review?(dveditz)
Attachment #305740 - Flags: review?(dveditz)
(Assignee)

Updated

10 years ago
Attachment #305741 - Flags: review?(dveditz) → superreview?(dveditz)

Comment 9

10 years ago
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 10

10 years ago
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 11

10 years ago
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+

Comment 12

10 years ago
Created attachment 305911 [details] [diff] [review]
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.
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+

Comment 14

10 years ago
(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

10 years ago
Created attachment 305943 [details] [diff] [review]
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
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

Updated

10 years ago
Depends on: 419794

Comment 17

10 years ago
419794 filed.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.