NSS: Add ciphers to map

[mkolechkin]

Add cipher names to the cipherlist map, based on the list of ciphers
implemented by the NSS in the source code file
https://github.com/nss-dev/nss/blob/master/lib/ssl/sslenum.c

[mkolechkin]

I looked at the latest NSS source code and compared list of ciphers they implement with the mapping in the curl source code file lib/vtls/nss.c (static const struct cipher_s cipherlist[])
I found some ciphers are not included into the curl NSS back-end map cipherlist.
This change proposes to add the missing ciphers mapping.

I difficulty building NSS on my machine, I'll try to overcome them, but I do not environment to build and test my change locally. I'll send you update if I test it properly. I'd like to discuss first if this change makes sense.
Thank you

[danielgustafsson]

I found some ciphers are not included into the curl NSS back-end map cipherlist.
This change proposes to add the missing ciphers mapping.

The fact that they're missing doesn't necessarily mean that they wanted. Can you provide some rationale for why we should include them? TLS_DHE_DSS_WITH_RC4_128_SHA is for example marked as disabled in the cipherSuites array in ssl3con.c which makes me want some more background.

[kdudka]

@danielgustafsson The user (or the application that uses libcurl) decides which cipher-suites will be enabled. The mapping just needs to be updated to keep this configurable.

[kdudka]

To be clear, the proposed change does not affect the list of cipher-suites enabled by default.

[mkolechkin]

Daniel and Kamil, thank you for the discussion.
I agree with Kamil, this change doesn't modify the set of ciphers enabled by default, it just adds some ciphers for completeness.

If users do not care about specific ciphers, they do not set the cipher list value. If users specified some cipher(s), they have some reason for it.
This change doesn't modify the default ciphers list, this is just a mapping to ciphers implemented by NSS. The cipherSuites enabled/disabled value can be changed by SSL_CipherPrefSet.
NSS disabled some weak ciphers by default when users do not care about specific cipher, but they are still available if users need them.

The main reason for this change is to complete the mapping to ciphers implemented by NSS and make it more consistent.
I think curl should allow users to chose ciphers implemented by NSS and do not limit users from selecting one or another cipher implemented by an encryption library.
Otherwise, curl better make it consistent way, and remove the mapping for RC4/RC2, DES, RSA, and other ciphers, also explain that criteria to users, maintain a list of allowed ciphers, etc.
Right now I see missing cipher mapping is just an error we can fix but if curl library will be responsible for filtering ciphers, that is a different thing and it assumes more effort and more maintenance.
Consistency is the reason we better fix this, i.e. we better have all cipher mappings available for NSS rather than selectively map some and skip others.

Thank you

[danielgustafsson]

Sorry for being slow to respond. Thanks for clearing up the intent and impact, I don't have any objections to aligning ourselves with what is available in NSS (I haven't reviewed this list wrt which versions of NSS this would require vs the baseline version we require but I have little doubt they wont overlap).

[kdudka]

Looks good to me. It would be good to also update https://github.com/curl/curl/blob/master/docs/CIPHERS.md but that could be handled separately.

[kdudka]

I would just put there an additional space to keep the second column aligned.

[mkolechkin]

Hi Daniel,

Sorry for being slow to respond.

No problem, I am also not fast.

Thanks for clearing up the intent and impact, I don't have any objections to aligning ourselves with what is available in NSS (I haven't reviewed this list wrt which versions of NSS this would require vs the baseline version we require but I have little doubt they wont overlap).

I tried to track down constants to versions they were introduced. Ciphers with AES algorithm (TLS_DHE_DSS_WITH_AES_128_GCM_SHA256, TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 and TLS_DHE_DSS_WITH_AES_256_CBC_SHA256) were introduced by 3.20 release (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.20_release_notes). The first one AES with Galois counter mode is already included, only two cipher block chaining ciphers are missing and added by this change. I added them under #ifdef to protect compilation and keep the same style as other mappings.

Camellia ciphers were introduced in the 3.12 release (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.12_release_notes.html). I added an #ifdef guard for them.

The single SEED NSS suite TLS_RSA_WITH_SEED_CBC_SHA was added in the 3.12.3 release (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.12.3_release_notes). I added an #ifdef guard for it. I'll send an updated diff soon.

The TLS_DHE_DSS_WITH_RC4_128_SHA suite is not registered by IANA, but NSS implements it since a long time ago, I did not find the first version it was introduced, but for sure 3.10 already has it. I checked other cipher numbers already in the curl nss.c source code and found they were introduced after NSS 3.10 release (between 3.10 and 3.12), like TLS_ECDHE_ECDSA_WITH_NULL_SHA, TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA, TLS_ECDH_anon_WITH_AES_256_CBC_SHA and others (13 total). Therefore I assume the base version of NSS is supported by curl is later than 3.10. Hence I did not add #ifdef for that cipher.

The case for ciphers TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA, TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA, TLS_DHE_RSA_WITH_DES_CBC_SHA, and TLS_DHE_DSS_WITH_DES_CBC_SHA is more complicated. The NSS 3.16 release introduced those constants in addition to already defined SSL_ names (#define constants) having the same number as TLS_ counterparts (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.16_release_notes).
A similar case for ciphers already in curl nss.c file like SSL_RSA_WITH_RC4_128_MD5, SSL_RSA_WITH_3DES_EDE_CBC_SHA, and some others. Based on that idea, I changed TLS_ #define names for those ciphers to SSL_ (i.e. SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA, SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA, SSL_DHE_RSA_WITH_DES_CBC_SHA, and SSL_DHE_DSS_WITH_DES_CBC_SHA)
Also, I moved them up to the same group where the rest of SSL_ ciphers in the map.

I'll send the updated version soon. Please let me know if that version should be improved.

Also, I have another question about cipher names but I'll make another comment for it.

[mkolechkin]

I tried to figure out a pattern for cipher names and found that some ciphers contain 'cbc' part for cipher clock mode (dhe_dss_aes_128_cbc_sha, dhe_rsa_aes_256_cbc_sha, etc.) and some don't (ecdh_ecdsa_aes_128_sha, ecdh_rsa_3des_sha, etc.) That is ok, even IANA and IETF cipher names are not very consistent. I googled and chose variants without cbc to match mod_nss and Firefox cipher names.
It did not work for the TLS_DHE_DSS_WITH_RC4_128_SHA cipher. I found a single reference to the name 'tls_dhe_dss_rc4_128_sha' (https://listman.redhat.com/archives/freeipa-users/2015-September/msg00328.html) but prefix tls_ is very unusual for short aliases and I think we better skip it.

Another question about naming, unification with OpenSSL or IANA.
Users need to set cipher name depending on backing encryption library: for curl with OpenSSL, command line is one, for curl with NSS command line is another even if the cipher is the same.
Among libraries supported by curl, OpenSSL and wolfSSL have cipher names matching each other for most of their ciphers. If we can add cipher aliases similar to OpenSSL names, a user would have a better experience and doesn't need to keep in mind different names for the same cipher suite.
If you think the idea is not very bad, I can work on it as another PR.
Thank you

[kdudka]

Just a minor remark: we do not need any #ifdef for constants introduced in nss-3.12.3 because curl requires at least nss-3.14.x since 2013: 30e7e75

[bagder]

Thanks!