Closed Bug 373276 Opened 17 years ago Closed 17 years ago

Enhance SSL's Bypass feature to withstand failures

Categories

(NSS :: Libraries, enhancement, P1)

3.11
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.11.7

People

(Reporter: nelson, Assigned: nelson)

Details

Attachments

(1 file)

Attached patch patch v1Splinter Review
In NSS 3.11 we added a performance feature to libSSL that enables it to 
optionally "bypass" the overhead of the PK11Wrap layer and the PKCS#11 module
and call directly into the basic crypto algorithm code for SOME (not all) 
operations.  In particular, we always do all asymmetric (public/private key) 
algorithms in a PKCS#11 module. We never bypass PKCS#11 for those algorithms.

The performance benefits were SUBSTANTIAL.  With the bypass enabled, and 
with the other performance enhancements we did, NSS 3.11 is over twice as 
fast as NSS 3.10 in certain benchmarks using SSL_RSA_WITH_RC4_128_MD5 or 
SSL_RSA_WITH_RC4_128_SHA cipher suites, doing over twice as many connections
per second.

We knew that this feature did not appeal to all NSS users.  Clearly this 
bypass operation is incompatible with FIPS 140, so we created an SSL socket
option to turn this feature on or off, default to off.  The application must
explicitly enable this feature to use it.

The bypass is not compatible with all PKCS#11 modules, and particularly not
with some FIPS 140 compliant modules that do no RSA decryption, and do not
allow any keys (including derived keys) to be extracted from the module 
"in the clear" (that is, unwrapped). 

As originally implemented, if the application enabled the bypass feature, 
but was using an incompatible PKCS#11 module (as described above), all 
"full" handshakes would fail.  The bypass code had no ability to recover
from a failure to get certain values from the PKCS#11 module.  It was 
unable to "fall back" on the original non-bypass pure PKCS#11 code base.

This RFE requests that the bypass be made able to fall back when it is used
with incompatible PKCS#11 modules.  

Accomplishing that task will make the bypass code somewhat slower, and will
make it use more memory.  These effects only occur when bypass is enabled.
Attachment #257912 - Flags: review?(alexei.volkov.bugs)
Priority: -- → P1
Comment on attachment 257912 [details] [diff] [review]
patch v1

Some explanation of this patch.  

Background:  There are two places in the flow of an SSL/TLS handshake
where the bypass tries to stop using PKCS#11 and do everything after
that point without PKCS#11.  These two places are known as the 
"triple bypass" and the "double bypass".  

The triple bypass is used only in full handshakes that use the RSA
key exchange algorithm.  It is called the triple bypass because it
bypasses PKCS#11 for 3 things: 
- the derivation of the master secret from the pre-master secret
- the derivation of the cipher and MAC keys from the master secret
- the bulk data enciphering/deciphering and MACing.

The triple bypass attempts to use the PKCS#11 module with the 
RSA private key to decrypt the PMS into process memory as data,
rather than trying to unwrap it into the module as a session key.
Without this patch, if that decryption attempt fails, the handshake
fails.  This patch causes a failed triple bypass to fall back on
attempting a double bypass.  This is thought to have negligible 
cost, and I believe it does not reintroduce Bleichebacher's 
"million question" attack.

A double bypass uses the PKCS#11 module to derive the pre-master
secret and then to derive the master secret from the pre-master 
secret.  The derived master secret is a session key in the PKCS#11
module.  The double bypass attempts to fetch the value of the 
master secret from the PKCS#11 module "in the clear" (without 
wrapping it with another key).  It is called the "double bypass"
because when it succeeds, it bypasses PKCS#11 for two subsequent
groups of operations:
- the derivation of the cipher and MAC keys from the master secret
- the bulk data enciphering/deciphering and MACing.

A double bypass failure occurs when and if the derived master secret 
cannot be extracted from the PKCS#11 module "in the clear". 
This patch adds support for recovering from a double bypass failure.
This support is conditionally compiled, predecated on the definition
of a new symbol: NSS_SURVIVE_DOUBLE_BYPASS_FAILURE .  It is conditional
because it has performance cost.  We believe it would be better to 
detect the conditions for double bypass failure at configuration time
and avoid them by disabling the bypass feature.  We may yet make it 
run-time conditional rather than compile time conditional.

To recover from the double bypass failure, it is necessary to 
completely recompute the "handshake hashes", which are the hashes
of all the SSL/TLS handshake messages sent or received at the point
where they are needed.  When the bypass is enabled, we optimistically
begin to compute the hashes outside of the PKCS#11 module, but if 
the double bypass fails, then the hashes must be recomputed INSIDE
the PKCS#11 module because those hashes need the value of the master 
secret.  So, being prepared to recover from double bypass failure 
means keeping a copy of the handshake messages, in order to be 
able to feed them into the PKCS#11 module for the hashes, if the 
recovery is needed.  

Being able to be flexible about the choice of module in which the 
handshake hashes are computed may have other benefits to NSS.

This patch: 

The code to initialize the handshake hashes was duplicated in several 
places in ssl3con.c.  This patch creates a new function to do that task,
and replaces all the duplicated code with calls to the new function.
With just one function to initialize the hashes, and just one to update
them, the logic for handling the two cases can be minimized.

This code also eliminates a bunch of duplicated code whose purpose is
to append bytes of data to an SSLBuffer.  It creates one new function
to do that, and calls it from the places that duplicated it.
Commited on bypass branch:
Checking in config.mk;  new revision: 1.21.26.1;     previous revision: 1.21
Checking in ssl3con.c;  new revision: 1.76.2.19.6.1; previous revision: 1.76.2.19
Checking in sslimpl.h;  new revision: 1.42.2.9.2.1;  previous revision: 1.42.2.9
Checking in sslsecur.c; new revision: 1.34.2.3.2.1;  previous revision: 1.34.2.3
Status: NEW → ASSIGNED
Comment on attachment 257912 [details] [diff] [review]
patch v1

This needs two reviews for NSS 3.11.7 ASAP
Attachment #257912 - Flags: superreview?(wtc)
Attachment #257912 - Flags: review?(neil.williams)
Comment on attachment 257912 [details] [diff] [review]
patch v1

I think ss->ssl3.hs.md5_cx and ss->ssl3.hs.sha_cx will be leaked after recovery from double bypass.
Attachment #257912 - Flags: review?(alexei.volkov.bugs) → review-
Comment on attachment 257912 [details] [diff] [review]
patch v1

I was wrong: there is no leak bug. sha_cx and md5_cx are the part of SSL3HandshakeState structure and will be freed with it. The found problem is that functions 
 SHA1_DestroyContext and MD5_DestroyContext wont be called for sha_cx and
md5_cx buffers in case of recovery from double bypass. The problem is not important, since SHA1_DestroyContext only zero the context of sha1 and MD5_DestroyContext does not do anything at all then second argument is PR_FALSE.
Attachment #257912 - Flags: review- → review+
Comment on attachment 257912 [details] [diff] [review]
patch v1

r=+, but sp. error in patch ssl3con.c line ~6564;
s/PSM/PMS
Attachment #257912 - Flags: review?(neil.williams) → review+
Committed on trunk:
Checking in config.mk;  new revision: 1.22;  previous revision: 1.21
Checking in ssl3con.c;  new revision: 1.101; previous revision: 1.100
Checking in sslimpl.h;  new revision: 1.56;  previous revision: 1.55
Checking in sslsecur.c; new revision: 1.38;  previous revision: 1.37

On branch, next.
On Branch:

Checking in config.mk;  new revision: 1.21.2.1;  previous revision: 1.21
Checking in ssl3con.c;  new revision: 1.76.2.20; previous revision: 1.76.2.19
Checking in sslimpl.h;  new revision: 1.42.2.10; previous revision: 1.42.2.9
Checking in sslsecur.c; new revision: 1.34.2.4;  previous revision: 1.34.2.3
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment on attachment 257912 [details] [diff] [review]
patch v1

I didn't review this patch in time.  Sorry.
Attachment #257912 - Flags: superreview?(wtc)
You need to log in before you can comment on or make changes to this bug.