848384 - Change the default value of the 'policy' setting from SSL_NOT_ALLOWED to SSL_ALLOWED for all cipher suites

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Adam Langley]

Attachment: patch

The cipher suite policy code has been ineffective for some time, except to make people wonder why nothing works until they call NSS_SetDomesticPolicy.

This change removes the last bits of the policy support. Ciphersuites can still be enabled/disabled by setting the preference, of course.

Comment 1

[Wan-Teh Chang]

Attachment: patch v2, by Adam Langley

I reviewed this patch at https://codereview.chromium.org/21564003/

https://hg.mozilla.org/projects/nss/rev/fee4f3677439

Comment 4

[Wan-Teh Chang]

Summary of changes:

1. Originally, each cipher suite was controlled by three settings:
   * enabled
   * isPresent
   * policy

The 'policy' setting was initialized to SSL_NOT_ALLOWED for all cipher suites.
(This is why an NSS-based program must call NSS_SetDomesticPolicy() for SSL/TLS
to work.)

2. In NSS 3.13.2 (bug 651523), NSS_SetExportPolicy() and NSS_SetFrancePolicy()
became synonyms of NSS_SetDomesticPolicy().

3. In patch v2 of this bug (comment 1), the cipher suite policy code was completely
removed. This means each cipher suite is now controlled by only two settings:
   * enabled
   * isPresent

The 'enabled' setting is the cipher suite preference. The 'isPresent' setting is
determined at run time by the presence of a crypto token that supports the cipher
suite.

It is no longer necessary to call NSS_SetDomesticPolicy(). It is as if the 'policy'
setting were hardcoded to be SSL_ALLOWED for all cipher suites.  SSL_CipherPolicySet()
does nothing and returns SECSuccess. SSL_CipherPolicyGet sets the output argument
*policy to SSL_ALLOWED and returns SECSuccess.

Question: an alternative is to simply initialize the 'policy' setting to SSL_ALLOWED
for all cipher suites and keep the cipher suite policy code. This will also make it
unnecessary to call NSS_SetDomesticPolicy(). I support Adam's patch to remove the
cipher suite policy code completely because I think the cipher suite preference code
is sufficient to control which cipher suites should be enabled. But I'd like to know
if there are objections. Thanks.

Comment 5

[Chris Newman]

I support removal of cipher policy code. Security policy is hard enough for most people to understand, even those who need to. Having two bits of enable information where one is sufficient was a design error.

However, I notice bug 848384 was closed as a duplicate of this bug. The gist of that bug was to provide a way to have the enable bits for cipher suites set to useful/appropriate defaults that will change over time. For example, the API proposed in that bug probably should turn off RSA/RC4/MD5 cipher suites by default today as they aren't appropriate for most uses (except when unavoidable for backwards compatibility). And I could see RSA/RC4/SHA1 suites going away in some number of years. The proposal was to have an API with that behavior so applications that wanted complete interface stability over security could choose not to call it and those that wanted security over interface stability could call it. I would like to understand how that will be addressed by this bug. Thanks.

Comment 6

[Florian Weimer]

(In reply to Chris Newman from comment #5)
> However, I notice bug 848384 was closed as a duplicate of this bug. The gist
> of that bug was to provide a way to have the enable bits for cipher suites
> set to useful/appropriate defaults that will change over time. For example,
> the API proposed in that bug probably should turn off RSA/RC4/MD5 cipher
> suites by default today as they aren't appropriate for most uses (except
> when unavoidable for backwards compatibility).

RC4 is actually unavoidable for backwards compatibility.  Browsers which do not support RC4 cannot connect to the web site of the German Federal Information Security Agency <https://www.bsi.bund.de>, for example.

> And I could see RSA/RC4/SHA1
> suites going away in some number of years. The proposal was to have an API
> with that behavior so applications that wanted complete interface stability
> over security could choose not to call it and those that wanted security
> over interface stability could call it.

I think the desire for complete behavioral stability is a bit at odds with security updates.  To fix protocol-level issues, we have to make externally visible changes.  From a distribution perspective, we'd rather have the behavioral upgrades by default.  We'll have to patch all NSS users to call something like NSS_SetContemporaryPolicy otherwise.

This doesn't apply just to cipher suites, but also to TLS version upgrades.

Comment 7

[Chris Newman]

My personal preference is that a "secure-by-default-at-the-API-layer" philosophy trumps interface stability (but only barely -- interface stability has been a huge benefit of NSS relative to other SSL libraries). In the past NSS designers considered interface stability more important than secure-by-default. The proposal in bug 842307 was a compromise between these two positions so a consuming application could choose a philosophy, but if NSS designers have switched to the secure-by-default design philosophy then I'm personally fine with the outcome.

The downside to the secure-by-default philosophy for cipher suites is applications can't hardcode cipher suite information; they have to query the NSS layer using SSL_ImplementedCiphers, SSL_NumImplementedCiphers, SSL_GetCipherSuiteInfo to get the list of ciphers that can be enabled/disabled by admin configuration and provide a way to adjust the defaults. Sample code to do that is in bug 358440.

Secure-by-default will make it necessary to disable RC4 by default in the near future (http://blog.cryptographyengineering.com/2013/03/attack-of-week-rc4-is-kind-of-broken-in.html), so end-users who want to use sites that require use of insecure ciphers will need a way to turn that back on manually until the sites get fixed.

Comment 8

[Wan-Teh Chang]

(In reply to Chris Newman from comment #5)
>
> However, I notice bug 848384 was closed as a duplicate of this bug. The gist
> of that bug was to provide a way to have the enable bits for cipher suites
> set to useful/appropriate defaults that will change over time.

I don't know the current goal of bug 848384. I thought it was a complaint
about the need to call NSS_SetDomesticPolicy() in order to use any cipher
suite. I will reopen bug 848384, and limit the scope of this bug to the fate
of the SSL cipher suite policy code.

Comment 9

[Wan-Teh Chang]

Attachment: Resurrect the SSL cipher policy framework, but change default to SSL_ALLOWED

Bob: this patch implements the changes we discussed in the NSS meeting this
morning.

1. It brings back the 'policy' setting for cipher suites.

2. It changes the default value of the 'policy' setting from SSL_NOT_ALLOWED
to SSL_ALLOWED for all cipher suites. The default value of the 'policy'
setting of all supported cipher suites is specified in the 'cipherSuites'
array in ssl3con.c.

3. It changes NSS_SetDomesticPolicy() to use the SSL_ImplementedCiphers
array rather than an 'ssl_ciphers' array defined locally in sslsock.c.

4. Please check the documentation of the three NSS_SetXXXPolicy() functions
in ssl.h.

I will try to generate another patch that shows the diffs against the
original code before the removal patch.

Comment 10

[Wan-Teh Chang]

Attachment: [Custom patch to help review] Resurrect the SSL cipher policy framework, but change default to SSL_ALLOWED

Bob: this custom patch is generated against the state of the tree before
the removal patch. It helps you review the net changes.

Comment 11

[Wan-Teh Chang]

Comment on attachment 790954 [details] [diff] [review]
[Custom patch to help review] Resurrect the SSL cipher policy framework, but change default to SSL_ALLOWED

This custom patch (the net changes from the original code before the SSL
cipher policy code was removed) can also be reviewed at
https://codereview.chromium.org/23279007/.

Comment 12

[Wan-Teh Chang]

Comment on attachment 790949 [details] [diff] [review]
Resurrect the SSL cipher policy framework, but change default to SSL_ALLOWED

I checked in this patch before obtaining Bob's review because it
mostly adds back the code that was removed:
https://hg.mozilla.org/projects/nss/rev/87b9b62d8028

Please review
1. the comments in ssl.h that describe the current state of
NSS_SetExportPolicy and NSS_SetFrancePolicy, and
2. the new implementation of NSS_SetDomesticPolicy in sslsock.c

and approve the change to initialize the 'policy' setting of
all cipher suites to SSL_ALLOWED instead of SSL_NOT_ALLOWED.

It is easier to review these changes using the custom patch
(attachment 790954 [details] [diff] [review]).

Comment 13

[Wan-Teh Chang]

Updated the bug's summary to reflect the final change.