Last Comment Bug 274512 - generating too much key material for some SSL ciphersuites
: generating too much key material for some SSL ciphersuites
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.9.4
: All All
: P2 normal (vote)
: 3.11.1
Assigned To: Nelson Bolyard (seldom reads bugmail)
: Jason Reid
Depends on:
  Show dependency treegraph
Reported: 2004-12-13 15:44 PST by Julien Pierre
Modified: 2006-10-25 19:52 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch v1 (12.16 KB, patch)
2005-04-10 16:26 PDT, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
Details | Diff | Splinter Review
Proposed patch for HPUX (1.91 KB, patch)
2006-01-19 11:31 PST, Nelson Bolyard (seldom reads bugmail)
julien.pierre: review+
wtc: review+
Details | Diff | Splinter Review

Description Julien Pierre 2004-12-13 15:44:29 PST
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.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2005-03-26 22:30:20 PST
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.  
Comment 2 Nelson Bolyard (seldom reads bugmail) 2005-04-10 16:26:51 PDT
Created attachment 180329 [details] [diff] [review]
patch v1

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.
Comment 3 Robert Relyea 2005-04-11 16:22:29 PDT
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.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-01-18 17:13:50 PST
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.69; previous revision: 1.68
Comment 5 Nelson Bolyard (seldom reads bugmail) 2006-01-19 11:13:24 PST
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. 
Comment 6 Nelson Bolyard (seldom reads bugmail) 2006-01-19 11:31:54 PST
Created attachment 208991 [details] [diff] [review]
Proposed patch for HPUX

I want to test build this patch on several platforms before 
asking for review.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2006-01-20 17:01:58 PST
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.
Comment 8 Julien Pierre 2006-01-20 19:42:09 PST
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.
Comment 9 Wan-Teh Chang 2006-01-20 20:09:31 PST
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.
Comment 10 Nelson Bolyard (seldom reads bugmail) 2006-01-21 22:43:13 PST
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
Comment 11 Nelson Bolyard (seldom reads bugmail) 2006-01-21 22:44:42 PST
Comment 12 Nelson Bolyard (seldom reads bugmail) 2006-04-24 00:05:49 PDT
I wanna carry this fix back to the 3.11 branch.
Any objections?
Comment 13 Robert Relyea 2006-04-24 13:17:17 PDT
go for it.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2006-04-24 23:33:43 PDT
Checked in on branch
lib/softoken/pkcs11c.c; new revision:; previous revision:

Note You need to log in before you can comment on or make changes to this bug.