820705 - Provide an option to allow RSA key exchange for SSL False Start

Status: RESOLVED WONTFIX

(NSS :: Libraries, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch 0

+++ This bug was initially created as a clone of Bug #713933 +++

The initial patch for bug 713933, contributed by Patrick McManus, and which I am attaching now, attempts to implement this enhancement as well as the enhancement that bug 713933 is about. These two bugs should be considered and/or fixed independently, as each bug is rather complex in its own right. Also, I think everybody agrees that it is worth solving bug 713933 in NSS proper but there is not much agreement about solving this bug or how to solve it. We should discuss the issues around this bug in more detail and probably do some experimentation within Firefox before we commit to having this feature in upstream NSS.

In bug 713933 comment 3 Wan-Teh wrote:
>> +#define SSL_FALSE_START_REQUIRES_FORWARD_SECRECY 25
>
> These two options make it very complicated to use SSL False Start.
> Ideally NSS should automatically do the right thing -- avoid
> incompatibilities and avoid cipher suite downgrade.
>
> Do you really need to flexibility provided by these two options?

We (most groups outside of Google) do not understand the compatibility issue as well as Google does. I haven't chatted with Patrick in detail about it yet, but it may still be worth us experimenting with different ways of addressing the compatibility issue.

Similarly, regarding the security issue (possible cipher suite downgrade), I think that we can improve upon the approach taken in this patch to reduce the the risk of cipher suite downgrade to an acceptable level, if it isn't already acceptably low with Patrick's approach.

I'm more concerned about the long-term effects of implementing this. With the current False Start implementation in libssl, we're really encouraging sites that care about performance to trade some extra CPU cycles--(EC)DHE key exchange--in return for significantly reduced network latency. In general, I think we should be encouraging sites to use the more-secure (EC)DHE key exchange mechanisms. On the other hand, it's hard to ignore the fact that most sites that are doing SSL are using RSA key exchange.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 691230 [details] [diff] [review]
Patch 0

Review of attachment 691230 [details] [diff] [review]:
-----------------------------------------------------------------

Re-submit this patch separated out from the patch for bug 713933.

Below, I note two of the bigger issues with the patch. There are more small issues. I will discuss them with Patrick on the phone.

::: security/nss/cmd/strsclnt/strsclnt.c
@@ +173,5 @@
>  	"       -u enable TLS Session Ticket extension\n"
>  	"       -z enable compression\n"
>  	"       -g enable false start\n",
> +	"            -gg enable false start without requiring NPN\n",
> +	"            -ggg enable false start without requiring NPN or forward secrecy\n",

This program is used as part of the NSS SSL test suite. This particular test is driven by the data in mozilla/security/nss/tests/ssl/sslstress.txt. Search that file for "-g" to get an idea of what you need to do to modify the test.

Also note that the syntax you chose for this does not allow us to test all four combinations of the two options you implemented. We should choose a syntax that works for all four combinations.

You can run that test suite like this:

cd mozilla/security/nss
make nss_clean_all
[OS_TARGET=WIN95] NSS_ENABLE_ECC=1 make nss_build_all
cd tests
[OS_TARGET=WIN95] HOST=localhost DOMSUF=local4 NSS_TESTS=ssl ./all.sh

Besides the output to stderr/stdout, see the generated files in mozilla/tests_results/localhost.XXX/.

::: security/nss/cmd/tstclnt/tstclnt.c
@@ +950,5 @@
> +    rv = SSL_OptionSet(s, SSL_FALSE_START_REQUIRES_NPN,
> +                       falseStartRequiresNPN);
> +    if (rv != SECSuccess) {
> +    SECU_PrintError(progName, "error setting false start requires NPN");
> +    return 1;

The indention is off. This file is indent-with-tabs, tab with == 8 spaces.

::: security/nss/lib/ssl/ssl3con.c
@@ +6091,5 @@
> +        rv = rv &&
> +            ss->ssl3.cwSpec->cipher_def->secret_key_size >= 10 &&
> +            (ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_rsa ||
> +             ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_dh  ||
> +             ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_ecdh);

This seems wrong to me. We don't want to allow these key exchange algorithms without also considering the key authentication algorithm used. For example, we don't want to allow anonymous Diffie-Hellman key exchange with False Start.

Comment 2

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #1)

> ::: security/nss/lib/ssl/ssl3con.c
> @@ +6091,5 @@
> > +        rv = rv &&
> > +            ss->ssl3.cwSpec->cipher_def->secret_key_size >= 10 &&
> > +            (ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_rsa ||
> > +             ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_dh  ||
> > +             ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_ecdh);
> 
> This seems wrong to me. We don't want to allow these key exchange algorithms
> without also considering the key authentication algorithm used. For example,
> we don't want to allow anonymous Diffie-Hellman key exchange with False
> Start.

FWIW that is exactly the condition that has been in NSS up until recently when it was changed to the set of conditions in the require-forward-secrecy branch.

If you have suggestions on an alternate forumaltion I'd appreciate it.

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #0)

> I'm more concerned about the long-term effects of implementing this. With
> the current False Start implementation in libssl, we're really encouraging
> sites that care about performance to trade some extra CPU cycles--(EC)DHE
> key exchange--in return for significantly reduced network latency. In
> general, I think we should be encouraging sites to use the more-secure
> (EC)DHE key exchange mechanisms. On the other hand, it's hard to ignore the
> fact that most sites that are doing SSL are using RSA key exchange.

IMO the much larger problem is that SSL generally performs slowly so sites won't adopt it at all. There are of course a myriad of reasons for this; many of them we can mitigate and we should do so whenever possible.

Comment 4

[Patrick McManus [:mcmanus]]

unless I hear other thoughts, as I refactor this I'll drop the option for NPN dependency and just make that a baked in requirement. It doesn't sound like anybody wants that flexibility at this time.

Comment 5

[Wan-Teh Chang]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> unless I hear other thoughts, as I refactor this I'll drop the option for
> NPN dependency and just make that a baked in requirement. It doesn't sound
> like anybody wants that flexibility at this time.

Yes. The NPN dependency is a simple way to determine if the server is
compatible with SSL False Start. Recall that agl managed a blacklist of
incompatible servers for a long time. He abandoned that approach in
favor of the NPN test.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> > ::: security/nss/lib/ssl/ssl3con.c
> > @@ +6091,5 @@
> > > +        rv = rv &&
> > > +            ss->ssl3.cwSpec->cipher_def->secret_key_size >= 10 &&
> > > +            (ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_rsa ||
> > > +             ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_dh  ||
> > > +             ss->ssl3.hs.kea_def->exchKeyType == ssl_kea_ecdh);
> > 
> > This seems wrong to me. We don't want to allow these key exchange algorithms
> > without also considering the key authentication algorithm used. For example,
> > we don't want to allow anonymous Diffie-Hellman key exchange with False
> > Start.
> 
> FWIW that is exactly the condition that has been in NSS up until recently
> when it was changed to the set of conditions in the require-forward-secrecy
> branch.

I see. The older code, before the patch in bug 810582, was that way, and so you were basically just reverting to the pre-patch test. However, I think this was just a bug in the previous version of the code, because it assumed that the application was only enabling a set of cipher suites that were compatible with False Start. In fact, that old code probably works fine in Chrome and even Firefox, but it isn't appropriate for libssl in general.

FWIW, I never reviewed the False Start code in libssl in detail. Besides this issue, I also remember Wan-Teh saying something to the effect that the False Start code in libssl may only work when SSL_ForceHandshake is used to drive the handshake. I am not sure yet if it works correctly and/or optimally (performance-wise) for Gecko, which doesn't (always) use SSL_ForceHandshake to drive the handshake.

> If you have suggestions on an alternate formulation I'd appreciate it.

When RSA key exchange is to be allowed, simply allow kea_rsa in addition to the other values allowed for ss->ssl3.hs.kea_def->kea.

Comment 7

[Wan-Teh Chang]

I remember I verified with NSS's ssltap tool that SSL False Start worked
in Firefox when the preference was set to true.

I suspect my comment about SSL_ForceHandshake must be regarding some other
SSL feature. I guess it was about our original patch for restart handshake
after cert auth, which you made work in Firefox.

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: patch 1

This (and 713933) are taken against the CVSROOT head.

I can't get the stress tests to run on my machine.. nameservice lookup errors.. this includes if I use localhost and loaldomain and is independent of these patches (i.e. using CVSROOT the same problems occur). So exactly what to do there probably needs some tweaking.

Comment 9

[Wan-Teh Chang]

(In reply to Patrick McManus [:mcmanus] from comment #8)
>
> I can't get the stress tests to run on my machine.. nameservice lookup
> errors.. this includes if I use localhost and loaldomain and ...

To run the NSS SSL tests, you need to set the HOST and DOMSUF environment
variables. NSS only uses their concatenation: $HOST.$DOMSUF

When I work on a computer that uses DHCP, I often work around the lack of
a hostname with HOST=localhost DOMSUF=intel.com. I then verify it with
    nslookup $HOST.$DOMSUF

nslookup should return the address 127.0.0.1 if the workaround works.

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 691847 [details] [diff] [review]
patch 1

this has been rolled into 658222 for now at least