Closed Bug 274512 Opened 16 years ago Closed 15 years ago

generating too much key material for some SSL ciphersuites


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: julien.pierre, Assigned: nelson)



(2 files)

In softoken/pkcs11c.c, there is a constant called NUM_MIXERS which is set to 9 .
It is used in a loop that includes a number of SHA1 and MD5 calls, used to fill
a key_block array .
We always go 9 times through this loop, even though this much data may not be
required in all cases . It would be good to figure out how much data we need
ahead of time, before going into the loop . This may help the SSL performance .

When I replaced this loop with a memset on both the client and server side, I
got a 2.8% performance improvement in SSL3 restart handshake performance (4190
to 4310 ops/s on the v20z dual opteron). We probably can't get that much because
some data is necessary, but it would still be good to reduce the number of
hashing calls as much as possible.
I am about to test a patch for this.  It looks like we are being particularly
wasteful in the TLS case, and TLS's "prf" function is very inefficient, so we
should be as consservative as possible in that case.  
Attached patch patch v1Splinter Review
This patch makes basically two changes.
1) both the MASTER_KEY_DERIVE and the KEY_AND_MAC_DERIVE sets of mechanisms 
had loops in which they called hash update functions twice, once for the 
client's random data and again for the server's random data.  This patch 
concatenates the client and server random data into a buffer, so that only
one hash update call is needed in the loop, not two.  This cuts the number
of hash updates significantly (about 33%) for SSL-related  key derivation.

2) For the KEY_AND_MAC_DERIVE mechanisms, we previously had determined the 
worst case, that is, the case that required the largest amount of key
material, and always computed that much, often using significantly less 
than what was generated.  Now, the code computes exactly how much it needs
and generates only that amount.  I found that for RC4_SHA ciphersuites,
we only need 4/9 of the amount previously being generated, so this should
speed up key derivation considerably.  

I consider this too risky one week before 3.10 RTM, so I propose this for 
NSS 3.11.
Attachment #180329 - Flags: review?(rrelyea)
Priority: -- → P2
Summary: We may be generating too much key material in NSC_DeriveKey → generating too much key material for some SSL ciphersuites
Target Milestone: --- → 3.11
Comment on attachment 180329 [details] [diff] [review]
patch v1


comments apply to the preexisting code and do not need to be addressed before
checkin. (however I would look favorably on a patch that implements one or both

1) There's a comment in the code about the similarity between the mixing
functions used to generated the ssl3 master secret derive and the ssl3 mac key
derive, and that maybe the mixing function should be a small function used by
both sections of code. The comment is probably very old, and likely predates
the tls code with does use put it's mixing function in a separate function.

2) Derive is *very* long (even for functions in pkcs11c.c). The SSL derive
mechanism are crying to be made into separate functions. At some point it would
be nice to split these out. (it may be argued that they belong in freebl
anyway.. I could go either way).

anyway patch approved for checkin.
Attachment #180329 - Flags: review?(rrelyea) → review+
QA Contact: bishakhabanerjee → jason.m.reid
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.69; previous revision: 1.68
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: 3.11 → 3.12
Reopening.  Unfortunately, this patch broke the build on HPUX 11.
HPUX 11's compiler won't allow a struct member to be initialized with the 
value of a variable, or with an address of a struct member that is accessed 
through a pointer.  The address of a local or global variable is OK.
So, I have a new small patch that will satisfy HPUX, forthcoming. 
Resolution: FIXED → ---
I want to test build this patch on several platforms before 
asking for review.
Comment on attachment 208991 [details] [diff] [review]
Proposed patch for HPUX

HPUX builds with this patch, without errors or warnings in that file.
Julien, please review for the trunk.  Thanks.
Attachment #208991 - Flags: review?(julien.pierre.bugs)
Comment on attachment 208991 [details] [diff] [review]
Proposed patch for HPUX

The patch looks good.
This seems like a really weird restriction in the HP compiler. I wonder what C version/standard forbids this.
Attachment #208991 - Flags: review?(julien.pierre.bugs) → review+
Comment on attachment 208991 [details] [diff] [review]
Proposed patch for HPUX

>+	    /* HPUX won't let a structure member be initialized with the 
>+	     * value of a variable, but the address of a local variable. :-/

Is this missing "is OK"?

It looks a little strange to only initialize
some fields of these structures, especially
the SECItem "keyblk".  I'm wondering if the
compiler initializes to 0 the fields without
an initializer.
Attachment #208991 - Flags: review+
Comment on attachment 208991 [details] [diff] [review]
Proposed patch for HPUX

Checked in on trunk.
Checking in pkcs11c.c;  new revision: 1.70; previous revision: 1.69
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
I wanna carry this fix back to the 3.11 branch.
Any objections?
go for it.
Checked in on branch
lib/softoken/pkcs11c.c; new revision:; previous revision:
Target Milestone: 3.12 → 3.11.1
You need to log in before you can comment on or make changes to this bug.