1614758 - Fully implement RFC6367 suites

Status: RESOLVED INACTIVE

(NSS :: Libraries, enhancement, P5)

Description

[Mark Straver]

Currently NSS only has basic support for RFC6367 (Camellia) in CBC mode (and only with DHE and RSA KE, as far as I understood).
With CBC being considered risky for a certain class of attacks on block ciphers, it should be extended to include the AEAD / GCM suites for this cipher.

While I'm aware that Firefox as a product doesn't include this cipher in its default set anymore, our platform (that is using NSS) does and would like to include FS non-CBC suites based on Camellia for robust support.

Comment 1

[Darkspirit]

Please consider removing deprecated Camellia CBC from NSS and wontfixing this.

(In reply to Mark Straver from comment #0)
https://www.palemoon.org/Camellia_Appreciation_Day.html == SSL_ERROR_NO_CYPHER_OVERLAP day :-/
https://www.ssllabs.com/ssltest/analyze.html?d=www.palemoon.org&s=2a01%3a4a0%3a68%3a1%3a0%3a0%3a0%3a492a&latest
Instead, please support TLS 1.3 with AES-GCM and CHACHA, remove plain RSA ciphers, disable TLS 1.0 + 1.1 and enforce https. (Users of super old insecure Windows versions could still download with another device like their smartphone.)
https://ssl-config.mozilla.org/#server=nginx&version=1.17.7&config=intermediate&openssl=1.1.1d&guideline=5.4
https://wiki.mozilla.org/Security/Server_Side_TLS#Intermediate_compatibility_.28recommended.29

Comment 2

[Roy Tam]

FYI: GnuTLS and mbedTLS supports Camellia GCM.

Comment 3

[J.C. Jones [:jcj] (he/him)]

Thanks for raising this. We're certainly supportive of wiring up Camellia with GCM, and in agreement with retiring CBC modes when possible.

The necessary changes to do this would be to the PKCS11 layer in softoken, libssl, the relevant gtests, and it'd be good to add to the bltest cmd (at least). There are many places to change, but most of it is rather mechanical.

Currently the NSS team is focusing on TLS 1.3 modes, but we'd be willing to review and land such a patch - so marking P5, happy-to-accept-patch.

Comment 4

[Mark Straver]

I'm working on implementing this in our copy of NSS (although it may take a little due to a milestone release with more important things to tackle first) and can supply patches here, but I work only in git, not mercurial. Is it okay to supply git patches (the code changes are effectively the same, after all)?
Also, for the implementation, I don't think it makes much sense to implement non-ephemeral suites. Do you agree?

Comment 5

[Martin Thomson [:mt:]]

Phabricator works just as well with git.

Comment 6

[Mark Straver]

Attachment: nss-patch1.patch

This is part 1, implementing the ephemeral suites from RFC 6367 section 2.1 in NSS.
This adds 4 suites of the 8 total listed because non-ephemeral really doesn't make much sense to add in 2020.

I have the camellia-GCM part (section 2.2) done as well (tuned to the kind of support available in server implementations, i.e. leaving out non-ephemeral as well and less common suites, resulting in 8 more suites to be added instead of the 20 listed in the RFC) but our copy of NSS is 3.48 and apparently between that and current tip a hell of a lot was moved around, renamed and changed and to provide upstream patches here it looks like I'll have to redo all the changes to add camellia-gcm algorithm support pretty much from scratch to match what there is now :(

It's probably not useful to upload the 3.48 patches here, is it?

Comment 7

[Mark Straver]

Attachment: nss-patch2.patch

This patch adds the ductwork for using Camellia-GCM, ported to the state at git commit a660b2c3f941e9a5918617f5445d7b3336c128f8 (April 2nd)

Comment 8

[Mark Straver]

Attachment: nss-patch3.patch

Part 3 implements Camellia-GCM suites in NSS.

This excludes non-ephemeral suites and DSS suites listed in the RFC because they would not see use, and is primarily based on what GnuTLS and mbedTLS support in practice, reducing the 20 listed suites to 8 (-10 non-ephemeral and -2 ephemeral DSS suites) to keep support lean and focused.

Comment 9

[Mark Straver]

The three submitted patches should be all that's needed for a functional implementation.

the relevant gtests, and it'd be good to add to the bltest cmd (at least).

Unfortunately I'm out of my depth regarding this request -- I'm unfamiliar with Google Test and don't have the free cycles (nor much desire) to learn it -- so I'll only be able to add the functional code here and will have to leave writing tests up to someone else.

Comment 10

[Mark Straver]

Attachment: Add missing OID entries.

Missed adding the OID entries.

Comment 11

[Mark Straver]

According to someone who has a gnuTLS backed server (not publicly accessible) this does not yet work. I've based this on previously submitted work for camellia-GCM years back, as to where to change things, but it's very well possible I'm missing some place(s) that need additional changes that I'm unaware of. Some help from someone with more familiarity with NSS would be appreciated.

Comment 12

[Roy Tam]

Attachment: Camellia-GCM freebl WIP changes

My trial on implementing Camellia-GCM in freebl. In details, trying to make freebl camellia_* functions more AES-alike and make use of freebl's GCM functions.

But apache mod_gnutls returns Alert (Level: Fatal, Description: Bad Record MAC) i.e. server failed to decrypt our packet(s). I DO hope there will be some advises for making it totally working.

Comment 13

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 9138014 [details] [diff] [review]
nss-patch3.patch

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

::: lib/ssl/ssl3con.c
@@ +157,5 @@
>  
>   /* RSA */
>   { TLS_RSA_WITH_AES_128_GCM_SHA256,         SSL_ALLOWED, PR_TRUE,  PR_FALSE},
>   { TLS_RSA_WITH_AES_256_GCM_SHA384,         SSL_ALLOWED, PR_TRUE,  PR_FALSE},
> + { TLS_RSA_WITH_CAMELLIA_128_GCM_SHA256,    SSL_ALLOWED, PR_FALSE, PR_FALSE},

I'd rather we avoid adding any more non-ephemeral ciphersuites. I like adding some additional compatibility with the Camellia suites, but I'd rather not have to go take these static RSA ones back out, even disabled-by-default as they are.

This is just opinion, of course, but I'd like to lean forward if we possibly can.

Comment 14

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 9144623 [details] [diff] [review]
Camellia-GCM freebl WIP changes

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

On basic inspection I don't see what's wrong either. 

I would like to add Camellia to the ghash gtest [0] which currently has only AES-GCM known answers from NIST (in gcm-vectors.h) [1]. Can you find us some Camellia-GCM KATs that we can run in that test? The whole point of that struct breakdown is that it should isolate where the mismatch lies in the implementations.

[0] https://searchfox.org/nss/source/gtests/freebl_gtest/ghash_unittest.cc
[1] https://searchfox.org/nss/source/gtests/common/testvectors/gcm-vectors.h

Comment 15

[Roy Tam]

(In reply to J.C. Jones [:jcj] (he/him) [increased latency due to COVID-19] from comment #14)

Comment on attachment 9144623 [details] [diff] [review]
Camellia-GCM freebl WIP changes

Review of attachment 9144623 [details] [diff] [review]:

On basic inspection I don't see what's wrong either.

I would like to add Camellia to the ghash gtest [0] which currently has only
AES-GCM known answers from NIST (in gcm-vectors.h) [1]. Can you find us some
Camellia-GCM KATs that we can run in that test? The whole point of that
struct breakdown is that it should isolate where the mismatch lies in the
implementations.

[0] https://searchfox.org/nss/source/gtests/freebl_gtest/ghash_unittest.cc
[1] https://searchfox.org/nss/source/gtests/common/testvectors/gcm-vectors.h

maybe we can have tests from https://gist.github.com/h-yamamo/27241d8d96966317adce6b83e6a3bdfd#file-camellia-gcm-patch-L712

Comment 16

[Mark Straver]

My responsibilities have shifted considerably for our project and I don't see myself having the time or opportunity to re-acquaint myself with NSS with how things have been refactored over time to work on this bug. Please clear my assignment from this bug; I'd be happy if anyone else would pick this up as it's still desired to have Camellia as an alternative to AES in NSS.

Comment 17

[Benjamin Beurdouche [:beurdouche]]

Thanks Mark, I will mark this as INACTIVE for now.