148452 - SSL_ConfigSecureServer always generates a step-down key for RSA

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Julien Pierre]

When using domestic RSA keys, this function will always generate a step-down RSA 
key. This key generation is CPU-intensive and is not always needed.

1) Servers that have export ciphers disabled and don't need that key at all. The 
API has no way of telling that currently, because the setting of ciphers happens 
at a different time, possibly after this call.
I suggest that a parameter should be added to allow not generating that stepdown 
key. The server should only do this if it knows it won't use export cipher 
suites.

Of course, the SSL code should be safe, so that if the server erroneously 
configures an export cipher suite but disables generation of the stepdown key, 
the handshake would fail with an error (such as misconfigured server).

2) When there are many virtual servers, it can mean that a server will take 
minutes to come up, spent almost entirely in this call, generating a stepdown 
key for each virtual server. The solution is to allow sharing of the stepdown 
key. We don't always want to do this because of the security implications, so 
again this should be a configuration parameter.

The three settings would be :
- don't generate a stepdown key
- generate a dedicated stepdown key (same as now)
- use a shared stepdown key (and generate it the first time if it doesn't exist)

There should also be an API to swap the shared stepdown key.

Comment 1

[Julien Pierre]

Bug 149165 brings up another problem here, the fact that we may not want to 
share a single step-down key as I previously thought, but rather that we want to 
stick it into the same token as the certified public key. 

This means that we'll need to create one step-down key per token used, 
rather than one for the entire server process as I originally thought. 
SSL_ConfigServer would have to look at the token where the certified public key 
lives, and then check in a table if it already has a step-down key object in 
that token. If not, it would create one and stick it into the table for later 
sharing by other model sockets on virtual servers using keys on the same token.

One of the trickiest part of doing this is the updating of the table when we 
want to change the step-down key. I haven't quite thought through yet how this 
would be done.

Comment 2

[Wan-Teh Chang]

Moved to NSS 3.6.

Comment 3

[Julien Pierre]

Moving to NSS 3.7 .
The fixes that I proposed earlier are possible, but not simple to implement.
A very simple fix if the application never wants to do any step-down handshakes
would be to have a new API "SSL_DisableStepDown()" that a server application
would call, which would cause the generation of the step-down keys to be
skipped. This would fix the virtual server performance problem, but of course at
the cost of not being able to communicate with export browsers.

Comment 4

[Julien Pierre]

Nelson,

What do you think of the proposal in comment #3 ? How hard is it to implement ?
What would be the behavior of an export browser trying to connect to such a
server that doesn't support step-down ? Would the user get any clue that he
needs to upgrade his browser ?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I wouldn't assume that the primary reason for doing SSL step down is the
existence of old "export" browsers.  

There are many SSL server administrators that consciously choose to enable
only the "export" ciphersuites, out of a mistaken but persistent belief 
that those suites use less CPU cycles than the "domestic" ciphersuites.

Try disabling the export ciphersuites in your browser, and notice how 
many servers you find with which you cannot succesfully interoperate.  
I'll bet most of those servers use certs with 1024-bit public keys.
None of those servers could work if they disabled SSL step down.

The export ciphersuites are _defined_ in such a way that the enforce the 
now outdated export controls, one of which is that they cannot use an
RSA public key stronger than 512 bits for the key exchange.  So, when an
SSL server has an RSA cert with a 1024-bit public key, and it negotiates
an "export" ciphersuite, it must do SSL step down.  

Comment 6

[Julien Pierre]

Nelson,

I'm not proposing any changes on the client side, but only on the server side.
I suggest we make a change to disable step down key generation on the server
side. If there isn't a significant number of browsers that are export-only, then
that is good news, and we might even want to make that the default setting. If
that isn't the case already, the export cipher suites should all be disabled by
default in our servers.

My new API proposal is as follows :

void SSL_SetStepDown(PRFileDesc* serversocket, PRBool state);

which stops the generation of step down keys in the server . The "serversocket"
argument would be either a model socket or listen socket.

Obviously there would be a problem if someone enabled an export cipher suite
after disabling stepdown, since no step down RSA key would be available. We
would assert in debug builds in this case, and fail the handshake in optimized
builds if that occurred (using the closest possible SSL alert for "server
configuration hosed").

It is however possible for a server application to prevent that from happening
through its configuration. Ie. the server could refuse to come up if setup with
stepdown disabled with export cipher suites enabled.

I just realized there is yet another approach that doesn't involve any API
change : simply delay the generation of the RSA stepdown key until it is needed
- ie. when a client actually attempts to negotiate an export cipher suite. This
would take care of the startup problem, and most likely of the problem
altogether, because it is unlikely that all SSL virtual servers would be getting
new SSL export cipher connections simultaneously. In most cases, it would just
mean a slight delay for the first export browser user connecting to a virtual
server.

Comment 7

[Julien Pierre]

Note that the last two proposals are not mutually exclusive.
Ie. we could both add the API to disable the stepdown key generation when it is
unneeded, and delay the stepdown key generation in cases where it is needed.

Comment 8

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 9

[Wan-Teh Chang]

Moved to NSS 3.9 and assigned to Nelson.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I wante the solution to this issue to have the property that the SSL code
*cannot* and will not get into a state where it negotiates an export cipher
suite when it has no step-down key (and one is needed, e.g. cert's public
key is larger than 512 bits).  Today, we allow ciphersuite preferences and
policies, and even server cert selection to take place at any time, even
after one or more handshakes have completed on a socket.  I don't want the
need for a step-down key to change on a running socket.

Here is a proposal that I think meets that requirement (well enough).

I propose to create a new function, NSS_SetNoExportPolicy, which sets the
set of cipher policies to the complement of NSS_SetExportCipherPolicy.
That is, it allows all non-export ciphers, and disallows all export ciphers. 

Then I would change SSL_ConfigSecureServer (or perhaps
ssl3_CreateRSAStepDownKeys) to check to see if ANY export ciphersuites are
allowed by policy.  If so, it would generate step-down keys as today, and if
not, it would not.  SSL_ConfigSecureServer would also test the cert's public
key strength, and not generate a step-down key if it is only 512 bits.  

I would also change all the NSS_Set*Policy functions so that only one of them
may be called, and after one of them is called, Calling any other will return an
error code.  This is intended to avoid problems where the cipher suites
preferences are changed after calling SSL_ConfigSecureServer.  It doesn't
completely solve those issues, because it doesn't prevent SSL_CipherPolicySet
from being called after SSL_ConfigSecureServer.  But I think most applications
do not set their own custom policies, and instead use one of the NSS_Set*Policy
functions.   

I invite comments to this proposal, either added here or by email.

Comment 11

[Julien Pierre]

Nelson,

I believe most applications (all the released apps I have seen so far, excluding
our tools) do not use the NSS_Set*Policy functions in fact, but use the
functions to enable individual ciphers on sockets for SSL. So I don't think
adding a new policy for non-export ciphers will be very useful. As you pointed
out yourself, this new policy would not prevent potential a missing stepdown key
in applications that set individual ciphers, which I believe to be most of them.
I don't have anything against adding it, but I doubt it will be used much.

For example, a web server might have 2 virtual servers with export cipher
suites, and 100 virtual servers with domestic cipher suites. By policy, the web
server would have to use SetExportPolicy, and that would cause 102 stepdown keys
to be generated - one for each virtual servers. So we would not get the benefit
of any optimization.

For the purpose of the optimization of stepdown key generation, an application
would explictly call SSL_SetStepDown(myserversocket, PR_FALSE) on the 100 server
sockets that don't need an export cipher suite, and would have to make sure not
to enable an export cipher suite for this socket - or it could later fail the
handshake.

The only way to truly prevent the "missing stepdown key" case 100% of the time
is for us to implement one of the complex stepdown key generation/caching
mechanisms I devised earlier, but this is a lot of work and I don't think it is
worth the trouble.

So here is what I propose :
1) as you suggest, add the NSS_SetNoExportPolicy(), which tells the NSS never to
generate any stepdown key in SSL_ConfigSecureServer
2) also add SSL_SetStepdown per-socket function to explicitly disable step-down
key generation . This would enable or disable the generation of a step-down key
for that socket in SSL_ConfigSecureServer.

Note that this is still a compromise, because every time the server is
configured (NES supports dynamic reconfiguration) we will regenerate a stepdown
key for virtual servers that need an export cipher suite.
But at least we won't generate any stepdown keys for virtual servers that never
need them.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Regarding the question of which Policy functions are being used by NSS 
programs, I found that 

SSLsample/client
SSLsample/server 
selfserv/selfserv.c
strsclnt/strsclnt.c
testcltn/testcltn.c
vfyserv/vfyserv.c

all use NSS_Set{Domestic,Export}Policy.  

Only sslstrength/sslstrength.c calls SSL_CipherPolicySet to set cipher 
policies individually, and we do not build it routinely in NSS builds.

Comment 13

[Robert Relyea]

I think setting Using Policy setting is an elegant solution to the problem. The
one whole you described could be filled by creating a 'lockPolicy()' call which
ConfigureSecureServer() would call, thus locking whatever policy that was
selected in place.

bob

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - flawed new feature implementation

Here's the next revision of my proposal for this RFE.
I propose to add a new SSL socket option, SSL_NO_STEP_DOWN, defaults to false.
When set to true, then a subsequent call to SSL_ConfigSecureServer() with a
cert with an RSA public key would have the following behavior:
   If the cert's public key is 512 bits or less, no step-down key would be 
      generated (because none is needed, by definition).
   Else
      the policy bits for this server socket are modified to exclude all
      export cipher suites.  

The initial implementation in this patch only defines the option number.
A call to SSL_OptionSet with this option will return an error (unrecognized
option number).  But that's a mere bug, which can be fixed after the feature
complete date.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 133370 [details] [diff] [review]
patch v2 - flawed new feature implementation

Reviewers, Please let me know if this approach meets with your satisfaction,
ASAP.

Comment 16

[Julien Pierre]

Comment on attachment 133370 [details] [diff] [review]
patch v2 - flawed new feature implementation

Nelson, this proposal looks acceptable.

Comment 17

[Wan-Teh Chang]

Comment on attachment 133370 [details] [diff] [review]
patch v2 - flawed new feature implementation

r=wtc.

>+#define SSL_NO_STEP_DOWN               15 /* Never step down for export suites*/

Could you changed the comment to also say what the default for this option is?

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Marking bug as enhancement request, since the fix introduces a new feature.
Patch above checked in on trunk.  Bug not yet fully fixed.  :)

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

retarget for 3.10

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Attachment: basic implementation, v1

This implementation accomplishes the crucial part: it stops the 
generation of SSL step down keys.  Ideally, the implementation of 
this feature would also
a) disable all export ciphersuites, and 
b) prevent any export ciphersuites from being subsequently enabled.
But a server that is willing to disable the export ciphersuites 
itself, and not allow them to be enabled after setting this option,
will get the performance benefits that it wants.  

Julien, please review this in time for NSS 3.10 Beta 1.

Comment 21

[Julien Pierre]

Comment on attachment 176422 [details] [diff] [review]
basic implementation, v1

The code looks correct, but it is incomplete

1) we should have a test in all.sh to enable that option . We should make sure
selfserv can work with it disabled, and test the results with export cipher
suites on the client and server side (more below)

2) SSL_CipherPrefSet and SSL_CipherPrefSetDefault should fail to enable export
cipher suites if this flag is set, and SSL_OptionSet/SSL_OptionSetDefault
should fail to set this flag if export cipher suites are enabled

3) minor indentation issues in sslimpl.h & sslsock.c with the noStepDown flag
line

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Well, that guarantees this features won't go into NSS 3.10 B1. :(

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Attachment: selfserv patch to test new feature

This patch adds a new -E option to selfserv.
It disables all export ciphersuites, and sets the no-step-down flag.
I have not yet incorporated this into ssl.sh, but I have tested it
and it works as expected.  

This patch also demonstrates the use of SSL_GetCipherSuiteInfo to 
obtain attributes of a ciphersuite.  In this case, it is trying to 
find the export ciphersuites.

Jullien please review this patch by itself, without regard to the 
other patches attached to this bug.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, more complete implementation

This implementation is the same as the last, with the addition of code
to disable export ciphersuites when NO_STEP_UP is set, and disallowing
export ciphersuites to be enabled while it is set.  
There are separate implementations for the global cipher preferences 
and for the per-socket cipher preferences.

Julien please review.  I'd like to get this into Build 3 for NSS 3.10

Comment 25

[Julien Pierre]

Comment on attachment 179523 [details] [diff] [review]
selfserv patch to test new feature

Nelson,

This patch looks good.

I would add an assertion if the following condition is true. That would be
indicative of a bug in the preceding call to SSL_GetCipherSuiteInfo . It would
claim to have returned the info for the cipher suite, but would have returned
the info for another. IMO, it's serious enough that we would want to know about
it through QA failure so we can fix it.

+	if (info.cipherSuite != suite) {
+	    printf(
+"SSL_GetCipherSuiteInfo returned wrong suite! Wanted 0x%04x, Got 0x%04x\n",
+		   suite, i);
+	


The other error conditions you check right after probably indicate bugs in SSL
as well, and asserting wouldn't be a bad idea either.

Comment 26

[Julien Pierre]

Comment on attachment 179535 [details] [diff] [review]
patch v2, more complete implementation

1) There is a problem with SSL_DisableDefaultExportCipherSuites.
You are using a loop with an index i, which is never used in the body of the
loop. I think you meant to add something like :

pInfo = suiteInfo[i];

2) In SSL_DisableExportCipherSuites, in the following :

+    for (i = 0; i < NUM_SUITEINFOS; ++i, ++pInfo) {

The use of ++pInfo may cause the code to skip over the first cipher suite in
suiteInfo. I don't think it's an export suite so it doesn't matter. But it
probably should be fixed.

3) Nit: There is a problem with the indentation for the noStepDown declaration
in both sslsock.c and sslimpl.h .

Other than that, the code looks OK.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #26)
> (From update of attachment 179535 [details] [diff] [review] [edit])
> 1) There is a problem with SSL_DisableDefaultExportCipherSuites.
> You are using a loop with an index i, which is never used in the body of the
> loop.

you're right, there's a missing ++pInfo in that for statement.
I will add that.

> 2) In SSL_DisableExportCipherSuites, in the following :
> 
> +    for (i = 0; i < NUM_SUITEINFOS; ++i, ++pInfo) {
> 
> The use of ++pInfo may cause the code to skip over the first cipher suite in
> suiteInfo. 

I do not believe that.  The ++i and the ++pinfo get executed at the
bottom of the loop.  (This is an example of a an expression with 
multiple parts separated by commas.  The entire expression (++i, ++pInfo)
gets evalated as a single expression at the bottom of the loop.)

> 3) Nit: There is a problem with the indentation for the noStepDown declaration
> in both sslsock.c and sslimpl.h .

No, there isn't, except in raw diffs.  The "problem" occurs because some
lines use tabs and others use spaces.  When the extra column is put on the
front of the line (for a + or -), lines that use spaces appear to be 
indented one more column than lines that use tabs, but they only appear 
that way in the raw diff, not in the patched code.  bugzilla's patch diff
feature shows what the code looks like with the patch applied, and it shows 
that there is no indentation problem.  Please see this at 
https://bugzilla.mozilla.org/attachment.cgi?id=179535&action=diff

Do you want me to add another patch to show the missing ++pInfo?

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 179523 [details] [diff] [review]
selfserv patch to test new feature

Checked in selfserv patch.
selfserv.c; new revision: 1.63; previous revision: 1.62

Our QA test scripts rely on program exit codes to report test failures.
(Any non-zero return code indicates failure.)  So rather than asserting, 
this code ensures that the test program returns a code that indicates 
failure in the cases discussed above.  That did not require a change to
the reviewed patch.  The reviewed patch did that.

Comment 29

[Julien Pierre]

Nelson,

Re: comment #27, go ahead and check in the patch with your first loop fixed. It
would be good if you attached the patch as checked in as well.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Checked in above patch, modifed as suggested in comment 27.
Will attach patch of final checkin.

sslimpl.h;  new revision: 1.36; previous revision: 1.35
sslinfo.c;  new revision: 1.9; previous revision: 1.8
sslsecur.c; new revision: 1.27; previous revision: 1.26
sslsock.c;  new revision: 1.35; previous revision: 1.34

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch as checked in

This is what was checked in.