Closed Bug 325672 (canbypass) Opened 15 years ago Closed 14 years ago

NSS needs a function to indicate usability of the bypass feature

Categories

(NSS :: Libraries, enhancement, P1)

3.11
enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.11.8

People

(Reporter: neil.williams, Assigned: neil.williams)

Details

Attachments

(7 files, 17 obsolete files)

15.00 KB, application/octet-stream
Details
1.45 KB, patch
Details | Diff | Splinter Review
5.92 KB, patch
alvolkov.bgs
: review+
nelson
: review+
Details | Diff | Splinter Review
23.08 KB, patch
julien.pierre
: review+
nelson
: review+
Details | Diff | Splinter Review
6.23 KB, patch
alvolkov.bgs
: review+
Details | Diff | Splinter Review
561 bytes, patch
julien.pierre
: review+
nelson
: superreview+
Details | Diff | Splinter Review
456 bytes, patch
Details | Diff | Splinter Review
In 3.11 a feature was added to bypass PK11 routines for SSL connections. As this has a considerable affect on performance there is demand to use it when appropriate. Deciding when it is appropriate depends on both policy and technical considerations. For example for NSS to be in compliance with FIPS guidelines the bypass must not be enabled. 

This function should determine whether it is technically possible for a given certificate and private key to bypass PK11 for key derivation. For bypass to happen either the pre-master secret (PMS) or the master secret must be extracted in the clear from the token that has the private key. Reasons this may fail include a token that does not release key material in clear text and a key exchange algorithm is not supported for bypass.
Attached patch canbypass test function (obsolete) — Splinter Review
Actually two entry points to the canbypass function. One takes both a cert and private key handle the other takes just a cert. Return codes are same for both, SECFailure means the test was not completed because of some problem with the arguments, SECSuccess means the test was complete and the canbypass boolean has been set.
Attachment #210541 - Flags: review?(nelson)
Neil,

I would suggest that we only have the function that takes both the cert and private key as arguments, not the one that takes only the cert.
The reason is that the private key for a given cert may exist in multiple tokens with different properties. But PK11_FindKeyByDERCert will only return at most one key object. The server may end up using a different private key object in another slot, and then the answer from the canbypass function would be wrong.

If you want to have an interface that takes just a cert, you would need an API that finds all the private keys. Then you could check if they are all OK for bypass, and return PR_TRUE in *pcanbypass. But if any one fails, you would have to return PR_FALSE .
Priority: -- → P2
Version: unspecified → 3.11
QA Contact: jason.m.reid → libraries
Comment on attachment 210541 [details] [diff] [review]
canbypass test function

r-
(Please let me know if this review seems to contradict previous
feedback that I've given about this patch.)

1) SSL_CanBypass should use PK11_FindKeyByAnyCert (or something like it) 
rather than PK11_FindKeyByDERCert.  In particular, it should not be 
necessary that cert->slot be non-NULL prior to calling this function.

2) libSSL implements two forms of bypass: 
- "double bypass", which extracts the derived master secret from the 
PKCS#11 token, and bypasses (a) the derivation of the encryption and 
MAC keys from it, and (b) the actual encryption and MACing; and
- "triple bypass", which also bypasses the derivation of the master
secret from the RSA pre-master secret, by decrypting the PMS as data.

Double bypass works (or should work) with all types of KEA algorithms
and certs, IIRC.  Triple bypass works only with RSA, IIRC.  

SSL_PrivKeyCanBypass should set the canbypass output to true if either 
form of bypass works for the cert/key being tested.  So, the function
should work for other types of certs/keys besides RSA.  In particular,
now that NSS works with ECC keys and certs, These CanBypass functions
need to also work with them, IMO.  So, this code should check for 
double-bypass with all key types, and for triple-bypass only with RSA
keys/certs.

3) I think the comments should describe the results of the functions,
but not the internal workings of the functions.  (we can discuss that offline.)
Attachment #210541 - Flags: review?(nelson) → review-
Comment on attachment 210541 [details] [diff] [review]
canbypass test function

Neil, Nelson,

I feel that the SSL_CanBypass function as defined below has an insufficient prototype to deal with all cases properly :

SECStatus SSL_CanBypass(CERTCertificate *cert, PRBool *pcanbypass, void *pwArg);

In comparison, the SSL server configuration function is defined as follows :

SSL_IMPORT SECStatus SSL_ConfigSecureServer(
                                PRFileDesc *fd, CERTCertificate *cert,
                                SECKEYPrivateKey *key, SSLKEAType kea);

This API allows the server to pass a cert and private key where cert->slot != key->pkcs11Slot . The web server actually does this from time to time. That's because the web server does not look for the server private key in cert->slot when configuring itself . There is a very good reason why, which is well outside the scope of this bug, but I'll be happy to explain it to you offline .

Because the SSL_ConfigSecureServer API really relies on the key->pkcs11Slot and not cert->slot, SSL_CanBypass cannot use cert->slot to look for the private key for the cert . SSL_CanBypass cannot guess the value for key->pkcs11Slot unless it is passed in to the API  - or unless it's willing to search for the private key in all available slots, and then test every match for bypassability, which I think would be expensive and undesirable as there could be unnecessary negative answers - eg. the private key exists in 2 slots, only one supports bypass, and SSL_ConfigSecureServer uses the key in that slot, but SSL_CanBypass will return PR_FALSE because the other slot with the other copy of the private key doesn't support bypass.

I suggest passing in the SECKEYPrivateKey* to SSL_CanBypass, rather than just the PK11SlotInfo* which would be needed to resolve the ambiguity here.

As you pointed out in comment 3, we need to support multiple KEAs at least for double-bypass, not just RSA, so the kea argument would really come in handy for canbypass. This way the SSL_CanBypass function only needs to test for KEA algorithms that will actually be used by the server .
Attachment #210541 - Flags: superreview-
Julien, it appears that your review comments failed to notice the function
SSL_PrivKeyCanBypass, which explicitly provides separate cert and key pointers.
SSL_CanBypass exists to obviate the separate key pointer for programs that
don't need to separately provide it.
Nelson,

I saw that function. I don't think we should have a function that doesn't take a private key as input . Every server is already looking up their own private key during their configuration process, so this isn't going to save anyone any time. Adding code in NSS that attempts to mimic what the server does during its key lookup can only induce errors and confusion. We only need a single function, one that accepts a private key. And my comments about the SSLKEAType argument still apply.

Another problem is that this function only addresses the bypass server case. Bypass can also be used for client, in particular in servers that act as SSL clients. Do we need a test for client bypass with client auth ?
This might be another argument to the function (test for bypassability for server or client).
Now P1 for 3.11.6
Priority: P2 → P1
Target Milestone: 3.12 → 3.11.6
I don't expect this will be the final version although I believe it addresses all issues raised in previous comments.
Attachment #210541 - Attachment is obsolete: true
Attachment #252962 - Flags: review?(nelson)
I will ask for superreview for patch 252962 after discussing it with Nelson.
Status: NEW → ASSIGNED
Attached file canbypass test utility
This tar file contains the cmd utility used for degbugging the CanBypass function.

(Will not be committed.)
Not for review or checkin, this patch was an experiment to see what would happen when RSA fails to decrypt the PMS. It changes the mechanism table so that RSA decrypt is not supported. Without this patch (and WITH patch #252962) CanBypass is able to decrypt the PMS (the triple bypass); with this patch the triple bypass test fails, CanBypass continues on to derive the master secret and attempts to extract it which succeeds. This implies that trying to extract the MS ("double bypass") is a sufficient test for bypassability.

There was some concern that in the case of ECDHE-RSA where the cert's key is RSA but the KEA is ECDHE we might be able to decrypt a PMS (with the RSA key) but not be able to extract the MS that ECDHE generates. That's why we will always test for MS extraction as a necessary and sufficient condition for bypassing PKCS11.
Attachment #252962 - Attachment is obsolete: true
Attachment #254249 - Flags: review?(nelson)
Attachment #252962 - Flags: review?(nelson)
I'm trying to verify that this works in selfserv. As of this writing selfserv in bypass mode with patch #254242 installed fails with an invalid MAC from the client. My crippled mechanism list must be causing the failure.
Found this bug during experimental debugging. CanBypass returned an SECFailure when canbypass was FALSE--should return SECSuccess.
Attachment #254249 - Attachment is obsolete: true
Attachment #254466 - Flags: review?(nelson)
Attachment #254249 - Flags: review?(nelson)
Attachment #254466 - Attachment is obsolete: true
Attachment #254466 - Flags: review?(nelson)
Attached patch refinements added (obsolete) — Splinter Review
This patch checks all cipher suites supported by NSS (server side) except ECDHE (I claim).

In other news: There is no test utility except for the cbputil I attached earlier.  Rather than add a new utility how about adding a -T option to selfserv which would mean test the bypassability of the cipher suites and cert/key pair specified by the rest of the args?
Attachment #258622 - Flags: review?(nelson)
Attachment #258622 - Attachment is obsolete: true
Attachment #258622 - Flags: review?(nelson)
Checking in lib/ssl/derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.3.2.1.8.1; previous revision: 1.3.2.1
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.24.26.1; previous revision: 1.24
done
Checking in lib/ssl/ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.17.28.1.6.1; previous revision: 1.17.28.1
done
Comment on attachment 258746 [details] [diff] [review]
patch as checked in on bypass branch

This patch is good enough for "feature complete" on the bypass branch.
Some review comments:

1.  (nit) possible warning at this line:

>+    *pcbp = (rv == SECSuccess);

I like this approach.  
I usually cast the boolean expression to type PRBool.  e.g. 

>+    *pcbp = (PRBool)(rv == SECSuccess);

That same approach could have been used here:

>+	    if (strcmp(csdef.keaTypeName, "ECDHE") == 0) /* ephemeral? */
>+		testecdhe = PR_TRUE;
>+	    else
>+		testecdh = PR_TRUE;

e.g. testecdhe = (PRBool)(strcmp(csdef.keaTypeName, "ECDHE") == 0);
or   testecdhe = (PRBool)(!strcmp(csdef.keaTypeName, "ECDHE"));

2. intended result?

>+    if (testrsa && privKeytype == rsaKey
>+        && !ssl_canExtractMS(pms, PR_FALSE, pcanbypass) && ! *pcanbypass) {
>+	goto done;
>+    }

That code will go to done if canExtractMS returns FALSE *AND* canbypass is 
false.  In the case where canExtractMS returns success but canbypass is false
it will continue.  I don't think that's what you want.  I think you wanted

>+     && !(ssl_canExtractMS(pms, PR_FALSE, pcanbypass) && *pcanbypass)) {
or perhaps the equivalent 
>+     && (!ssl_canExtractMS(pms, PR_FALSE, pcanbypass) || !*pcanbypass)) {

That same issue also occurs below for testecdh.

>+    if (testecdh && privKeytype == ecKey
>+	&& !ssl_canExtractMS(pms, PR_TRUE, pcanbypass) && !*pcanbypass) {
>+	goto done;
>+    }

3. ecdhe test may need more work (or maybe just the comment :)

>+    if (testecdhe) {
>+	/* treat this like the non-ephemeral case since the server's cert
>+	   must be an ecKey */
>+	testecdh = PR_TRUE;
>+    }

I think that comment is saying that all ciphersuites that use ECDHE as the 
KEA also use certs with EC public keys.  (If that's not what it's saying
then please enlighten me.)  But consider the ciphersuites with names like
TLS_ECDHE_RSA_WITH_*.  They do ECDHE for KEA but use certs with RSA public
keys.  

I think the code is right to do the same test of deriving the MS from the
PMS for both ECDH and ECDHE.  ECDH and ECDHE differ in how the PMS is 
derived, not in how the MS is derived from the PMS.  
But the comment above seems wrong.

4. Derivation of pms is wrong for ecdhe.  
The code at "case ecKey" is right for ecdh, but not for ecdhe.

The ecdhe KEA does not use the private key (corresponding to the cert) at all.
ECDHE suites use the cert's private key only ot sign the ephemeral public
key.  The PMS derivation is done with the remote's public ephemeral EC key
and the local's private ephemeral EC key.  So to test it, we have to 
generate TWO ephemeral EC keys, one local and one remote, and use both to 
derive the PMS.  

We can fix these on Friday.
I wrote:
> So to test it, we have to generate TWO ephemeral EC keys, one local and 
> one remote, and use both to derive the PMS.  

I meant to add: This derivation may not take place in the same slot as the
private key for the cert.  We need to let NSS choose the same slot for that
derivation as it will use in SSL.
I wrote: 
> That same approach could have been used here:

No, it couldn't.  Sorry.  :-/  (I need more sleep)
After comment #15 here is a suggested patch to selfserv to do that.
Attachment #258867 - Flags: review?(nelson)
Attachment #258867 - Attachment description: change selfserv to execize canbypass function → change selfserv to exercize canbypass function
Comment on attachment 258867 [details] [diff] [review]
change selfserv to exercize canbypass function

Neil,  it appears to me that your new -q option will not work correctly
unless the -c <ciphers> option is also given. (Do you agree?)  But the 
code for -q does not explicitly check that the -c option has also been given.

I think that the new canbypass option should work properly even 
when the -c option has not been given, and the server is running
with the default set of enabled cipher suites.
Attachment #258867 - Flags: review?(nelson) → review-
Attachment #258746 - Attachment is obsolete: true
Attachment #260084 - Flags: review?(nelson)
When the testbypass option is set, just do canbypass and exit immediately. Also, test using the cipher suite list specified by all of the command line options including using the default set when no options are present.
Attachment #258867 - Attachment is obsolete: true
Attachment #260197 - Flags: review?(nelson)
Attachment #260197 - Attachment description: improved version → improved version of selfserv test
Comment on attachment 260197 [details] [diff] [review]
improved version of selfserv test

r=nelson for trunk and the special branch on which we've previously committed patches for this bug.
Attachment #260197 - Flags: review?(nelson) → review+
Comment on attachment 260084 [details] [diff] [review]
added test for stepdown, better ECDHE test

A few issues remain.  Can't use the EC key params from an RSA key.  I emailed the full review comments to Neil.
Attachment #260084 - Flags: review?(nelson) → review-
I hope this is the last patch for this. I had to mess around with ssl3con.c somewhat, moving some decls to sslimpl.h and changing some local functions to make them available to other SSL modules. Also found a couple of bugs in the selfserve patch. It will be committed along with this one when r=+.
Attachment #260084 - Attachment is obsolete: true
Attachment #262014 - Flags: review?(nelson)
Attachment #260197 - Attachment is obsolete: true
Attachment #262016 - Flags: review?
Attachment #262016 - Flags: review? → review?(alexei.volkov.bugs)
Comment on attachment 262016 [details] [diff] [review]
bug fixes to previous selfserv patch

Two little things:

>+PRUint16 cipherlist[100];
>+int nciphers;
>+
>+void
>+savecipher(int c)

I think it would be more preferable to use SSL_NumImplementedCiphers instead of '100'.
In this case you would not need to implement savecipher function at all.

Also, VLOG should be below "cleanup:"(See the last change).
Attachment #262016 - Flags: review?(alexei.volkov.bugs) → review+
Comment on attachment 262014 [details] [diff] [review]
fixed code for TLS_ECDHE_RSA case;re-arranged EC code

r=nelson.  I suggest the following changes.  (One is a MUST).
You may make them and check in the result without additional review.

>  * arguments were all valid but the slot cannot be bypassed.
>  */
>+#define EXPORT_RSA_KEY_LENGTH 64	    /* in bytes (from ssl3con.c) */

Rather than duplicating this here, can you move this to sslimpl.h, 
and take it out of ssl3con.c ?

>+	    if (!keapriv || !keapub) {
>+		if (keapriv)
>+		SECKEY_DestroyPrivateKey(keapriv);

Something wrong with the indentation there

>+	}
>+	else {

Please turn all of those into a single line " } else {"

>-static SECStatus 
>+SECStatus 
> ecName2params(PRArenaPool * arena, ECName curve, SECKEYECParams * params)

Since we're making this function extern (not static), its name should follow
the NSS style for extern names, e.g. prefix_FuncName.  This is a MUST.

>+ECName
>+ssl3_GetCurveWithKeyStrength(PRUint32 curvemsk, int requiredECCbits)

Please change that name to ssl3_GetCurveWithECKeyStrength  (addition of EC)
Attachment #262014 - Flags: review?(nelson) → review+
Checked in to 3.11 Bypass branch. Incorporates suggestions from comment 30.

Checking in lib/ssl/derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.3.2.1.8.2; previous revision: 1.3.2.1.8.1
done
Checking in lib/ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.76.2.19.6.2; previous revision: 1.76.2.19.6.1
done
Checking in lib/ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.3.2.11.6.1; previous revision: 1.3.2.11
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.42.2.9.2.2; previous revision: 1.42.2.9.2.1
done
(In reply to comment #29)
> (From update of attachment 262016 [details] [diff] [review])
> Two little things:
> 
> I think it would be more preferable to use SSL_NumImplementedCiphers instead of
> '100'.

Sounded like a good idea, except that SSL_NumImplementedCiphers is a global variable and its value is not available at compile time.

> In this case you would not need to implement savecipher function at all.

The idea is to check the ciphers listed in the command options--not all the implemented ciphers.

> 
> Also, VLOG should be below "cleanup:"(See the last change).
> 

I looked at this and it seemed to me to be better not to log anything. Since we don't actually start the server thread logging that it's exiting could be confusing.

Thanks for your comments.

Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.68.2.6.10.1; previous revision: 1.68.2.6
done
The next task is to produce a patch (or two) for the trunk and 3.11 branch
that have the same effect as the cumulative effect of the checkins made 
for this bug on the special bypass branch.
Target Milestone: 3.11.6 → 3.11.7
Attached patch version of patch for 3.11 branch (obsolete) — Splinter Review
This patch reflects all suggestions from comment 30.
Attachment #262840 - Flags: review?(nelson)
Attached patch selfserv patch for 3.11 branch (obsolete) — Splinter Review
Attachment #262841 - Flags: review?(alexei.volkov.bugs)
Comment on attachment 262841 [details] [diff] [review]
selfserv patch for 3.11 branch

r=alexei.volkov
Attachment #262841 - Flags: review?(alexei.volkov.bugs) → review+
Comment on attachment 262840 [details] [diff] [review]
version of patch for 3.11 branch

2nd review please, for 3.11 branch.
Attachment #262840 - Flags: superreview?(julien.pierre.boogz)
Neil,

Which NSS public header file did you intend to add SSL_CanBypass to ? I don't see it in your patch.
(In reply to comment #38)
> Neil,
> 
> Which NSS public header file did you intend to add SSL_CanBypass to ? I don't
> see it in your patch.
> 

Thanks, Julien. It should be in ssl.h but I see it got lost on the way from the bypass branch to 3.11. Will re-submit shortly.
Attachment #262840 - Attachment is obsolete: true
Attachment #262840 - Flags: superreview?(julien.pierre.boogz)
Attachment #262840 - Flags: review?(nelson)
If you would be so kind, one more (last) time.
Attachment #262014 - Attachment is obsolete: true
Attachment #263408 - Flags: superreview?(julien.pierre.boogz)
Attachment #263408 - Flags: review?(nelson)
Re: comments 29/32 about SSL_NumImplementedCiphers, you could use a dynamic array instead of a static array of 100 . But personally, I don't like the fact that we need that array at all in selfserv. More on this in my review for the API code.
Comment on attachment 263408 [details] [diff] [review]
includes declaration of canbypass in ssl.h

r=nelson
Attachment #263408 - Flags: review?(nelson) → review+
Comment on attachment 263408 [details] [diff] [review]
includes declaration of canbypass in ssl.h

Neil, here are my comments on this patch :

1) In the SSL_CanBypass prototype, do we need two arguments for the cipher 
pointer and number ? Can't we make it just a NULL-terminated array ?

2) about the need for a cipher list as input to the function :

Looking at the patches submitted so far, I notice that there is some 
redundant work being done to configure cipher suites. This is notable in 
the selfserv patch, which now configures ciphers both on the SSL socket 
and builds a separate cipherlist for the canbypass test. It seems like we  
could provide something that's more directly usable by runtime 
applications that takes an SSL socket and extracts the cipher list from 
it, ie. an SSL_SocketCanBypass . This could be an additional function that 
simply calls SSL_CanBypass . I had a chat with Neil about this and the 
reason stated for the current prototype was that some admin tools that could use this
function may not have a socket prebuilt to test with. That's a good point, but 
some other applications may simply want to use the function in their 
runtime and could more readily use a function taking a socket. Even for 
admin programs, they still have the possibility of building a socket 
that uses no OS socket resources. I believe Wan-Teh added a base NSPR 
layer type for that purpose last year, though I can't find where it's 
implemented right now. I won't block review for the lack of the API
that takes a socket, but it certainly would be nice to have one.

3) I am not sure that ssl_canExtractMS is complete . I don't see any 
reference to a test using the CKM_TLS_MASTER_KEY_DERIVE mechanism, which 
is sometimes used to derive the MS from the PMS. How come ? Is the MS from 
that mechanism always extractable ?

4) SSL_CanBypass tries to duplicate much of the processing that our SSL 
server implementation already does, for example to find the
KEA, public key, slot . However, there is much new code that isn't 
shared , so the 2 could get out of sync - and maybe already are, see 
comment 3 above. I am worried about the supportability of such code. Is 
there any way to share more code between the SSL implementation and the 
canbypass function ? Perhaps there is no way around this problem, but it 
seems like there is duplication here. Going back to my comment 2, maybe 
having a socket as an argument would make it easier to share code 
since a socket structure would be available.
Re: comment 43, I meant to refer to my earlier bullet points in that comment, and not to the early comments in the bug. I forgot that bugzilla would hyperlink.
(In reply to comment #43)
...
> 2) about the need for a cipher list as input to the function :
> 
> Looking at the patches submitted so far, I notice that there is some 
> redundant work being done to configure cipher suites. 
...
> 
> 4) SSL_CanBypass tries to duplicate much of the processing that our SSL 
> server implementation already does
...

Created bug 379667 to track this new entry point into canbypass.
I want to clarify about comment 43 that the most serious issue I see is the 3rd bullet point - completeness of implementation.

The 1 and 2 issues are "nice to have".

4 would be really nice but it's basically a rewrite of the function that would have to be done in conjunction with 2, so I understand if it cannot get done now. I realize I wasn't around during the time of the API design.

Comment on attachment 263408 [details] [diff] [review]
includes declaration of canbypass in ssl.h

Following email exchanges with Nelson and Neil, it became clear to me that SSL_CanBypass function needs to know whether the application intends to use the private key for SSL3, TLS, or both, in order to try to derive the MS from the PMS with the appropriate mechanism(s).

I suggest passing in an additional bit field and defining just two bits for it for now, for SSL3 and TLS 1.0, so that SSL_CanBypass can use the appropriate master key derive mechanism(s) among the 4 possible - CKM_SSL3_MASTER_KEY_DERIVE, CKM_SSL3_MASTER_KEY_DERIVE_DH, CKM_TLS_MASTER_KEY_DERIVE, CKM_TLS_MASTER_KEY_DERIVE_DH.

The test should be repeated for both SSL3 and TLS if the application is using both.
Attachment #263408 - Flags: superreview?(julien.pierre.boogz) → superreview-
(In reply to comment #47)
> (From update of attachment 263408 [details] [diff] [review])
...
> I suggest passing in an additional bit field and defining just two bits for it
> for now, for SSL3 and TLS 1.0, 
...

CanBypass already has the list of cipher suites to check. I see no reason to add another argument to it. Or are you thinking of adding the argument to ssl_canExtractMS?
(In reply to comment #48)
Never mind. I see there is not enough info in the arguments to detect whether the protocols are enabled or not.
Another clarification is needed regarding the list of cipher suites that is passed in. Currently, our bypass only works for SSL3 and TLS cipher suites, but not SSL2.
Yet, the application is allowed to pass any SSL cipher suite ID.  If SSL2 cipher suite IDs are passed in, we still go ahead and check the KEA, and do a key derivation and try to extract. I think that's a bug. We should just ignore all SSL2 cipher suites in SSL_CanBypass, since we can never bypass for them, and setting the SSL_BYPASS_PKCS11 option for SSL2 is not harmful.
Comment on attachment 263408 [details] [diff] [review]
includes declaration of canbypass in ssl.h

I took a closer look at this patch and found other problems.

1) There is a comment about only testing the double-bypass case with PK11_PubUnwrapSymkey, and that triple-bypass would use PK11_PrivDecryptPKCS1. Is it possible that the later would work and not the former ? And if so, would our test function respond that we can't bypass when we actually might be able to ?

2) int irv;

irv should be a SECStatus, not int.

3) 
	pms = PK11_PubUnwrapSymKey(srvPrivkey, &enc_pms,
				   CKM_SSL3_MASTER_KEY_DERIVE, CKA_DERIVE, 0);

	rv = ssl_canExtractMS(pms, PR_FALSE, pcanbypass);

It would be goot to check that pms is non-NULL before passing it to another function.
Same issue later with pms = PK11_PubDeriveWithKDF(...

4)

    srvPubkey = CERT_ExtractPublicKey(cert);
    privKeytype = SECKEY_GetPrivateKeyType(srvPrivkey);
    
You need to check that srvPubkey is non-NULL before calling SECKEY_GetPrivateKeyType .


5)
	    /* find a curve of equivalent strength to the RSA key's */
	    requiredECCbits = PK11_GetPrivateModulusLen(srvPrivkey) * BPB;

You probably want to check that the return value is greater than 0. PK11_GetPrivateModulusLen indicates an error by returning -1 .

6)

	if (testecdhe) {
	    /* generate server's ephemeral keys */
	    keapriv = SECKEY_CreateECPrivateKey(pecParams, &keapub, NULL); 
	    if (!keapriv || !keapub) {
		if (keapriv)
		    SECKEY_DestroyPrivateKey(keapriv);
		if (keapub)
		    SECKEY_DestroyPublicKey(keapub);
		PORT_SetError(SEC_ERROR_KEYGEN_FAIL);
		rv = SECFailure;
		goto done;
	    }

keapub needs to be initialized to NULL before the call to SECKEY_CreateECPrivateKey . Otherwise, if the call fails and the variable is left alone, the subsequent SECKEY_DestroyPublicKey might crash.

7) nit: I find it slightly confusing that you reuse the same variables for different purposes, ie.  client PMS and server PMS use the same variable. It would make the code more readable to have different variables.
There seems to be ongoing and recurrent confusion about what the canbypass
function is supposed to do, what question is it supposed to answer.  
I want to try to settle that here.

It is expressly NOT the purpose of CanBypass to say "your configuration is
completely correct, and every cipher suite that you have enabled will work
properly."  CanBypass is NOT required to output a negative (PR_FALSE) 
result just because some cipher suite will not and cannot work.  

CanBypass is required to tell you whether or not it is true that turning 
onn the bypass feature will make your server work less well than if you 
left that feature turned off.   If turning on the bypass feature will make
your server work NO WORSE than if it is turned off, then CanBypass should
output TRUE.  If turning on the bypass feature will make your server work
worse than without it (e.g. fail to handshake in cases where it would 
handshake OK without bypass), then ONLY in that case should CanBypass 
output a FALSE result.  

Now it may be the case that the admin will enable a set of cipher suites
with the property that SOME of the enabled cipher suites simply WILL NOT 
WORK at all, with or without the bypass, and the rest of the enabled 
cipher suites will all work OK, with or without the bypass.  In such a 
case, CanBypass must output TRUE, because the bypass will make the server
work NO WORSE than without it.  We don't want to tell the server that it
cannot bypass merely because there are SOME cipher suites that will fail
with or without the bypass, especially if all the rest of the cipher suites
will succeed with the bypass enabled.

For example, imagine a server configured to use both RSA and ECDHE_ECDSA
cipher suites, and is configured to use a flawed HSM that always returns an 
incorrect answer to the ECDHE computations, but does RSA flawlessly.  Such a 
configuration will always fail on the ECDHE_ECDSA cipher suites, and will 
generally succeed with the RSA cipher suites, with or without the bypass.
Should CanBypass return false in that case?  Should the server be deprived
of the bypass performance for the RSA cipher suites, merely because the 
ECC cipher suites all fail, with or without the bypass?  No, it should not.

In the course of using the various algorithms necessary to test the bypass
for a cipher suite, various failures can occur.  SOME of those failures 
have the effect of saying "this cipher suite will not work, with or without
the bypass", while other failures will have the effect of saying "if you 
enable the bypass, this cipher suite will fail in a way that it would not
fail if you disable the bypass".   Failures of that first kind, which 
imply that the cipher suite simply cannot work at all, with or without 
the bypas, should NOT affect the output of CanBypass AT ALL.  Any failure 
of the second kind should cause CanBypass to output FALSE.  

Depending on the cipher suites enabled, CanBypass will perform various 
key generation, key derivation or key wrapping and/or unwrapping and/or decryption operations, and operations to extract derived/unwrapped keys.  
Generally, any failure of key generation, key wrapping, or key unwrapping
operations indicate a failure that will stop that cipher suite from 
succeeding, with or without the bypass.  Failure to decrypt an RSA-encrypted
pre-master secret (the so-called "triple bypass") or failure to extract a
derived master secret are indicators that the cipher suite will fail if 
bypass is enabled.  

====================================================================
About Export cipher suites.  When export-grade RSA cipher suites are enabled
and the server's RSA keys are longer than 512 bits, the server is required 
to generate a 512-bit RSA key pair, known as the "step down" key pair, 
for use in the Export RSA cipher suites.  This is a time consuming operation.
It can take many seconds.  It is generally done only once, when the server
is started.

In order to really do the CanBypass tests thoroughly for export RSA cipher
suites, it is necessary to do that RSA step down key generation.  The key
must be generated (not imported) for the test results to be valid.  
This has the potential to be very costly at a time when such CPU cost is 
unexpected.  

So, when canbypass is called with a list of cipher suites that includes one
or more export RSA cipher suites, we have the following choices for the
behavior of CanBypass:
a) Do the RSA step down key gen, take the hit, spend the time, get the best  
   answer,
b) Ignore the export RSA cipher suites, and determine the answer based on 
   the results of testing the other non-export-RSA cipher suites,
c) test to see if the default RSA slot is an internal NSS slot.  If not,
   stop and output FALSE.  If so, behave as if the answer for this cipher
   suite is TRUE, and continue to test the other cipher suites, if any, 
d) Assume the answer is TRUE for this cipher suite, and continue to evaluate
   for the other cipher suites.  
e) Output FALSE.  Just say No. :)  If the server admin wants performance, 
   he should disable the RSA export cipher suites.

I don't like any of these alternatives.   Julien?
Alias: canbypass
Target Milestone: 3.11.7 → 3.11.8
Attachment #263408 - Attachment is obsolete: true
Attachment #265320 - Flags: review?(julien.pierre.boogz)
Attachment #265322 - Flags: review?(julien.pierre.boogz)
Attachment #265320 - Attachment is obsolete: true
Attachment #265320 - Flags: review?(julien.pierre.boogz)
Attachment #265322 - Attachment is obsolete: true
Attachment #265322 - Flags: review?(julien.pierre.boogz)
Attachment #265329 - Flags: review?(julien.pierre.boogz)
Attachment #265330 - Flags: review?(julien.pierre.boogz)
I've attached two patches. The only difference is that attachment 265329 [details] [diff] [review] tries
the triple bypass test in addition to the double bypass. I hope you find one to
be acceptable. The diffs from attachment 263408 [details] [diff] [review] are substantial because of a
deeper nesting but I believe it addresses the following reviewers' comments:
 
Comment 47. Added a bit field argument for specifying whether to test SSL,
TLS1.0  or both.

Comment 50. Changed the cipher list scanning to ignore SSL2 ciphers. If the
list contains only SSL2 cipher suites no testing is performed.


Comment 51. 1) See above.

 2) Changed declaration.

 3) Thanks for catching this. This test was lost in an early revision of the
    patch.

 4,5,6) Done.

Comment 52. Canbypass now continues to test in cases where key gen or derive fails. Only if all tests fail (or if the arguments were bad) will it return an error code.
This cannot be P1 any longer.  We didn't hold the release for it.  The 
people who wanted it so urgently decided not to use it in their next release.
Priority: P1 → P2
P1 again for 3.11.8, since we have demand again.
Priority: P2 → P1
Comment on attachment 265330 [details] [diff] [review]
same as above but without triple bypass test

I prefer to have the triple bypass test, so that the function tries to mimic the behavior of the SSL library as closely as possible, and doesn't claim that it can't bypass if triple-bypass would work.
I'm going to continue my review on the other patch.
Attachment #265330 - Flags: review?(julien.pierre.boogz) → review-
Re: comment 52 and export cipher suites,

Here are my thoughts on that subject :

a) is clearly expensive. We shouldn't always have to pay this keygen cost upfront

b) and d) seem equivalent to me. They both have the problem that the export cipher suite might not work with bypass, and if SSL_CanBypass returns that bypass is possible based on other tests, it could be lying to the caller, because turning on bypass could actually make the server worse and break it. This is what the existence of this API is designed to prevent.

c) We could optimize this test and skip the keygen if the NSS slot is default.
But if it's not, I think we have to fall back to a)

e) I *might* be OK with that if the application product that wants this API ships with export cipher suites disabled by default. But it would still be very non-obvious to the user why his server performance suddenly is divided by 3 if he turns on one export cipher suite :( So not my favorite option, though certainly the cheapest to implement.

Here is my suggestion, f)

- Always test the export cipher suites last. This way, if we have other failures, we will not do the key gen.

- If the default RSA slot is an NSS slot, skip the test

- If the default RSA slot is not an NSS slot, try to find out if export cipher suites can even work on that slot. There may be some further tests we can perform on the device, but I am not sure what they would be - about compatible keygens and mechs. This is an open question.

- Only if everything else looks good, then do the keygen and try to bypass with that key

- optionally, save the key in a global table, and modify SSL_ConfigSecureServer to use it . This would shift the keygen cost from one place to another instead of adding it
Comment on attachment 265329 [details] [diff] [review]
add a bit field argument to indicate which protocols to test

Sorry that I have to give this r- one more time.

Several things remain :

1) export cipher suite issue .

I had a long discussion with Nelson about this. See my previous comment 61 .

The code cannot remain the way it is because it may return false positives for export cipher suites. At least this much needs to be fixed. We should never have false positives.
Let's talk about this thursday.

2) Doc issues.

In ssl.h, the comment for SSL_CanBypass states :
 	
** protocolmask is formed from SSL options, e.g. (1<<SSL_ENABLE_TLS) would test

But the implementation has changed, and the mask is now defined differently :

 	

/* protocol mask bits */

	

#define SSL_CBP_SSL3	0x0001
#define SSL_CBP_TLS1_0	0x0002

There should be comments above each mask value, not just their name.

- I think the doc in the header is also insufficient in other respects. It doesn't make any reference to the SSL_BYPASS_PKCS11 SSL socket option for one thing, and it doesn't explain how the result from this function would be useful to the caller. 

On the other hand, the description of how the API is implemented is good, maybe even more than the application developers need.

I think a lot of what Nelson wrote in comment 52 should go in the doc.

- if we agree that some false negatives are acceptable for this function, ie. some cases where SSL_ENABLE_PKCS11 would work, but SSL_CanBypass says it wouldn't, then the discrepancies should allt be noted in the documentation in the header file also.
Attachment #265329 - Flags: review?(julien.pierre.boogz) → review-
Re: export cipher suites, I reviewed what we implemented in bug 148452 the last time we had a keygen problem. Nelson added the SSL_NO_STEP_DOWN option to the SSL library so that an application can explicitly state it will not use export cipher suites, and SSL_ConfigSecureServer will then not create the step-down keypair.

The reason this was necessary and we couldn't simply check if any export cipher suites were turned on in SSL_ConfigSecureServer was that the application could then have turned around later and enable the export suites after hte call to SSL_ConfigSecureServer, as there is no socket configuration "freeze" point with the libssl API.

The good news is that I checked with our customer, and the application defaults to having all the export cipher suites turned off, and the developer confirmed that it correctly sets SSL_NO_STEP_DOWN when those cipher suites are off, which is in most cases.

Given this, I think CanBypass should just say no if SSL_NO_STEP_DOWN is off, and this limitation should be noted in the doc in the header.
(In reply to comment #62)
Julien, I have a patch which I believe addresses all of your comments. Before I ask for another review, though...

(In reply to comment #63)
SSL_NO_STEP_DOWN is a socket option. As we're painfully aware now, CanBypass does not have a socket to check; are you suggesting a call to SSL_OptionSetDefault() to check SSL_NO_STEP_DOWN?
From offline comments the answer to my question in comment #64 is no.
Attachment #265329 - Attachment is obsolete: true
Attachment #265330 - Attachment is obsolete: true
Attachment #268991 - Flags: review?(julien.pierre.boogz)
Attachment #268991 - Flags: review?(julien.pierre.boogz) → review+
Comment on attachment 268991 [details] [diff] [review]
addresses Julien's comments

One more review for branch.
Attachment #268991 - Flags: review?(nelson)
Summary: NSS needs a function to indicate bypassability of a private key → NSS needs a function to indicate usability of the bypass feature
Comment on attachment 268991 [details] [diff] [review]
addresses Julien's comments

r=nelson. I found a few cosmetic issues that you may address, or not,
before checkin without requiring another review.

>+    /* For each protocol try to derive and extract an MS.
>+     * Failure of function any function except MS extract means

I think you meant to type either 
  "Failure of any function except ..."
or 
  "Failure of a function, any function, except..."


>+	for (; privKeytype == rsaKey && testrsa; ) {

The above statement is entirely equivalent to 
        while (privKeytype == rsaKey && testrsa) {
That is:
       for (;expression;) 
is equivalent to 
       while (expression)

which I prefer (slightly).  I found this unusual while construct
in several places in the patch, so if you change it in one place,
please change all of them.
Attachment #268991 - Flags: review?(nelson) → review+
Thanks guys.

Checking in lib/ssl/derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.3.2.2; previous revision: 1.3.2.1
done
Checking in lib/ssl/ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.17.28.2; previous revision: 1.17.28.1
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.24.2.1; previous revision: 1.24
done
Checking in lib/ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.3.2.12; previous revision: 1.3.2.11
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.42.2.11; previous revision: 1.42.2.10
done
Attachment #262841 - Attachment is obsolete: true
Attachment #269472 - Flags: review?(alexei.volkov.bugs)
Attachment #269472 - Flags: review?(alexei.volkov.bugs) → review+
Selfserve fix to test CanBypass().

Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.68.2.7; previous revision: 1.68.2.6
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 268991 [details] [diff] [review]
addresses Julien's comments

This patch adds the following declaration twice to sslimpl.h:

ECName	ssl3_GetCurveWithECKeyStrength(PRUint32 curvemsk, int requiredECCbits);

The second declaration is outside #ifdef NSS_ENABLE_ECC and breaks the
default non-ECC build because the type ECName is not defined.  Please
remove the second declaration.  Thanks.
reopening due to the preceeding comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Removed 2nd declaration.

Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.42.2.12; previous revision: 1.42.2.11
done
Thanks, Wan-Teh, for catching that.
I believe the last two checkins, shown in comment 70 and comment 73
still need to be comitted on the trunk.
Checking in lib/ssl/derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.5; previous revision: 1.4
done
Checking in lib/ssl/ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.19; previous revision: 1.18
done
Checking in lib/ssl/ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.26; previous revision: 1.25
done
Checking in lib/ssl/ssl3ecc.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3ecc.c,v  <--  ssl3ecc.c
new revision: 1.19; previous revision: 1.18
done
Checking in lib/ssl/sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.58; previous revision: 1.57
done
Checking in cmd/selfserv/selfserv.c;
/cvsroot/mozilla/security/nss/cmd/selfserv/selfserv.c,v  <--  selfserv.c
new revision: 1.78; previous revision: 1.77
done
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
SSL_CanByPass was marked as introduced in NSS 3.11.7. It is introduced in NSS 3.11.8.
Attachment #287902 - Flags: superreview?(nelson)
Attachment #287902 - Flags: review?(julien.pierre.boogz)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #287902 - Flags: review?(julien.pierre.boogz) → review+
Comment on attachment 287902 [details] [diff] [review]
Fix version in ssl.def

sr=nelson
Attachment #287902 - Flags: superreview?(nelson) → superreview+
committed on NSS trunk:
Checking in ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.20; previous revision: 1.19
done

committed on NSS_3_11_BRANCH:
Checking in ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.17.28.3; previous revision: 1.17.28.2
done
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
This patch was checked in on the NSS trunk (derive.c, rev. 1.6 and rev. 1.7)
but wasn't checked in on the NSS_3_11_BRANCH.  Nelson explained the problem
this patch solves in bug 324744 comment 10:

  Files in nss/lib must not include files from nss/cmd.
You need to log in before you can comment on or make changes to this bug.