Closed
Bug 325672
(canbypass)
Opened 18 years ago
Closed 17 years ago
NSS needs a function to indicate usability of the bypass feature
Categories
(NSS :: Libraries, enhancement, P1)
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.
Assignee | ||
Comment 1•18 years ago
|
||
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)
Comment 2•18 years ago
|
||
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 .
Updated•18 years ago
|
Priority: -- → P2
Version: unspecified → 3.11
Updated•18 years ago
|
QA Contact: jason.m.reid → libraries
Comment 3•18 years ago
|
||
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 4•18 years ago
|
||
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-
Comment 5•18 years ago
|
||
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.
Comment 6•18 years ago
|
||
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).
Assignee | ||
Comment 8•17 years ago
|
||
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)
Assignee | ||
Comment 9•17 years ago
|
||
I will ask for superreview for patch 252962 after discussing it with Nelson.
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•17 years ago
|
||
This tar file contains the cmd utility used for degbugging the CanBypass function. (Will not be committed.)
Assignee | ||
Comment 11•17 years ago
|
||
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.
Assignee | ||
Comment 12•17 years ago
|
||
Attachment #252962 -
Attachment is obsolete: true
Attachment #254249 -
Flags: review?(nelson)
Attachment #252962 -
Flags: review?(nelson)
Assignee | ||
Comment 13•17 years ago
|
||
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.
Assignee | ||
Comment 14•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #254466 -
Attachment is obsolete: true
Attachment #254466 -
Flags: review?(nelson)
Assignee | ||
Comment 15•17 years ago
|
||
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)
Assignee | ||
Comment 16•17 years ago
|
||
Attachment #258622 -
Attachment is obsolete: true
Attachment #258622 -
Flags: review?(nelson)
Assignee | ||
Comment 17•17 years ago
|
||
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 18•17 years ago
|
||
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.
Comment 19•17 years ago
|
||
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.
Comment 20•17 years ago
|
||
I wrote:
> That same approach could have been used here:
No, it couldn't. Sorry. :-/ (I need more sleep)
Assignee | ||
Comment 21•17 years ago
|
||
After comment #15 here is a suggested patch to selfserv to do that.
Attachment #258867 -
Flags: review?(nelson)
Assignee | ||
Updated•17 years ago
|
Attachment #258867 -
Attachment description: change selfserv to execize canbypass function → change selfserv to exercize canbypass function
Comment 22•17 years ago
|
||
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-
Assignee | ||
Comment 23•17 years ago
|
||
Attachment #258746 -
Attachment is obsolete: true
Attachment #260084 -
Flags: review?(nelson)
Assignee | ||
Comment 24•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #260197 -
Attachment description: improved version → improved version of selfserv test
Comment 25•17 years ago
|
||
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 26•17 years ago
|
||
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-
Assignee | ||
Comment 27•17 years ago
|
||
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)
Assignee | ||
Comment 28•17 years ago
|
||
Attachment #260197 -
Attachment is obsolete: true
Attachment #262016 -
Flags: review?
Assignee | ||
Updated•17 years ago
|
Attachment #262016 -
Flags: review? → review?(alexei.volkov.bugs)
Comment 29•17 years ago
|
||
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+
Updated•17 years ago
|
Attachment #262016 -
Flags: review+
Comment 30•17 years ago
|
||
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+
Assignee | ||
Comment 31•17 years ago
|
||
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
Assignee | ||
Comment 32•17 years ago
|
||
(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
Comment 33•17 years ago
|
||
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
Assignee | ||
Comment 34•17 years ago
|
||
This patch reflects all suggestions from comment 30.
Attachment #262840 -
Flags: review?(nelson)
Assignee | ||
Comment 35•17 years ago
|
||
Attachment #262841 -
Flags: review?(alexei.volkov.bugs)
Comment 36•17 years ago
|
||
Comment on attachment 262841 [details] [diff] [review] selfserv patch for 3.11 branch r=alexei.volkov
Attachment #262841 -
Flags: review?(alexei.volkov.bugs) → review+
Assignee | ||
Comment 37•17 years ago
|
||
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)
Comment 38•17 years ago
|
||
Neil, Which NSS public header file did you intend to add SSL_CanBypass to ? I don't see it in your patch.
Assignee | ||
Comment 39•17 years ago
|
||
(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.
Assignee | ||
Updated•17 years ago
|
Attachment #262840 -
Attachment is obsolete: true
Attachment #262840 -
Flags: superreview?(julien.pierre.boogz)
Attachment #262840 -
Flags: review?(nelson)
Assignee | ||
Comment 40•17 years ago
|
||
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)
Comment 41•17 years ago
|
||
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 42•17 years ago
|
||
Comment on attachment 263408 [details] [diff] [review] includes declaration of canbypass in ssl.h r=nelson
Attachment #263408 -
Flags: review?(nelson) → review+
Comment 43•17 years ago
|
||
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.
Comment 44•17 years ago
|
||
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.
Assignee | ||
Comment 45•17 years ago
|
||
(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.
Comment 46•17 years ago
|
||
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 47•17 years ago
|
||
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-
Assignee | ||
Comment 48•17 years ago
|
||
(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?
Assignee | ||
Comment 49•17 years ago
|
||
(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.
Comment 50•17 years ago
|
||
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 51•17 years ago
|
||
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.
Comment 52•17 years ago
|
||
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
Assignee | ||
Comment 53•17 years ago
|
||
Attachment #263408 -
Attachment is obsolete: true
Attachment #265320 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Comment 54•17 years ago
|
||
Attachment #265322 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Updated•17 years ago
|
Attachment #265320 -
Attachment is obsolete: true
Attachment #265320 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Updated•17 years ago
|
Attachment #265322 -
Attachment is obsolete: true
Attachment #265322 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Comment 55•17 years ago
|
||
Attachment #265329 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Comment 56•17 years ago
|
||
Attachment #265330 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Comment 57•17 years ago
|
||
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.
Comment 58•17 years ago
|
||
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
Comment 60•17 years ago
|
||
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-
Comment 61•17 years ago
|
||
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 62•17 years ago
|
||
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-
Comment 63•17 years ago
|
||
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.
Assignee | ||
Comment 64•17 years ago
|
||
(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?
Assignee | ||
Comment 65•17 years ago
|
||
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)
Updated•17 years ago
|
Attachment #268991 -
Flags: review?(julien.pierre.boogz) → review+
Assignee | ||
Comment 66•17 years ago
|
||
Comment on attachment 268991 [details] [diff] [review] addresses Julien's comments One more review for branch.
Attachment #268991 -
Flags: review?(nelson)
Updated•17 years ago
|
Summary: NSS needs a function to indicate bypassability of a private key → NSS needs a function to indicate usability of the bypass feature
Comment 67•17 years ago
|
||
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+
Assignee | ||
Comment 68•17 years ago
|
||
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
Assignee | ||
Comment 69•17 years ago
|
||
Attachment #262841 -
Attachment is obsolete: true
Attachment #269472 -
Flags: review?(alexei.volkov.bugs)
Updated•17 years ago
|
Attachment #269472 -
Flags: review?(alexei.volkov.bugs) → review+
Assignee | ||
Comment 70•17 years ago
|
||
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: 17 years ago
Resolution: --- → FIXED
Comment 71•17 years ago
|
||
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.
Comment 72•17 years ago
|
||
reopening due to the preceeding comment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 73•17 years ago
|
||
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
Assignee | ||
Comment 74•17 years ago
|
||
Thanks, Wan-Teh, for catching that.
Comment 75•17 years ago
|
||
I believe the last two checkins, shown in comment 70 and comment 73 still need to be comitted on the trunk.
Assignee | ||
Comment 76•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 77•17 years ago
|
||
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)
Updated•17 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•17 years ago
|
Attachment #287902 -
Flags: review?(julien.pierre.boogz) → review+
Comment 78•17 years ago
|
||
Comment on attachment 287902 [details] [diff] [review] Fix version in ssl.def sr=nelson
Attachment #287902 -
Flags: superreview?(nelson) → superreview+
Comment 79•17 years ago
|
||
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: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 80•16 years ago
|
||
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.
Description
•