crypto: add support for AES-CCM

[tniessen]

CCM ("Counter with Cipher Block Chaining-Message Authentication Code")

OpenSSL currently supports two AEAD algorithms, GCM and CCM. While node has supported GCM for years, CCM is a bit more difficult to implement and use.

Currently, this PR implements an API similar to what was proposed by @brycekahle in #2383:

• createCipher, createDecipher, createCipheriv and createDecipheriv accept an option authTagLength. While the tag length is not relevant for GCM, it is for CCM. (And yes, using CCM with createCipher / createDecipher is inherently insecure.)
• setAAD accepts additional options as an optional second argument. The only currently recognized option is plaintextLength, which specifies the length of the plaintext / ciphertext in bytes.

There are some critical aspects:

• The current implementation always requires the authTagLength option when using CCM. Not providing this option causes an error to be thrown by create(De|C)ipher(iv)?.
• Similarly, the current implementation requires the plaintextLength option when using CCM if and only if AAD is provided. The explanation is simple, the length of the plaintext is encoded into the first block of the cipher, and thus must be available before performing an update (and before encoding the AAD).
• update() can only be called once. Again, the reason is very simple, CCM operates in what is sometimes called a "packet" or "offline mode", meaning that all data is processed at once.
• It seems like OpenSSL does not support CCM decryption without the authentication tag.

Some of these points can be relaxed:

• By providing a default value for the authTagLength option, we could simplify the migration to this "new" AEAD. We could stick with the default value used within OpenSSL (12 bytes) or upgrade to 16 bytes (as e.g. pycryptodome does).
• By caching the data passed to setAAD until update is called, we take away the need to specify the plaintextLength. All data still needs to be passed to update in a single call.

We should discuss both, but it should be possible to add these features later on if they turn out to be good ideas (as long as they don't break anything). Aside from that, I am always happy about feedback and suggestions!

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

crypto

[tniessen]

Rebased due to a conflict with #18017. @nodejs/crypto and @nodejs/tsc, is there anything I can do to aid in discussing and reviewing this change?

[jasnell]

Generally LGTM but needs docs.

[tniessen]

@jasnell Thanks! Yes, documentation is still pending, I just want to discuss the general API before documenting its details :)

[jasnell]

ping @nodejs/tsc

[tniessen]

@bnoordhuis Thank you for your thorough review! I tried to address all suggestions (except #18138 (comment)), PTAL if you can.

[tniessen]

✔️ Light CI: https://ci.nodejs.org/job/node-test-commit-light/179/
❌ Full CI: https://ci.nodejs.org/job/node-test-pull-request/12752/ (fails in FIPS mode)

[tniessen]

Seems like I found the cause of the FIPS failure thanks to gdb. openssl/openssl@9cca7be introduced a slight change to the CCM API back in 2015, and this change never made it into the FIPS release. Without this commit, the authentication tag must be specified along with its length when decrypting, and I believe this makes it incompatible with our current design. The only option I can think of right now is to make CCM unsupported when in FIPS mode and to throw an error in create(De|C)ipher(iv)?, what do you think @bnoordhuis@jasnell? Is there any way to amend the proposed implementation to work around this problem?

This is the conflicting line:

(gdb) f
#0 aes_ccm_ctrl (c=0x3d1b490, type=17, arg=8, ptr=0x0) at e_aes.c:12841284 if ((c->encrypt && ptr) || (!c->encrypt && !ptr))
(gdb) next
1285 return 0;

---

There is one other thing I came across, OpenSSL 1.1.0 seems to have fixed the problem with EVP_CipherFinal in CCM mode, see openssl/openssl#2550 (comment). We should revisit that part of CCM once we transition to a newer version.

[bnoordhuis]

make CCM unsupported when in FIPS mode

I'm okay with that.

[jasnell]

I'm good with not supporting CCM in FIPS also.

[tniessen]

For now, I only disabled CCM decryption in FIPS mode, PTAL.

[bluetiger30]

great work guys , I was waiting for this
when will this be added to node ?

[tniessen]

@bluetiger30 This change will most likely be included in all releases beginning with node 10.0.0, but not in previous releases.

[tniessen]

@addaleax I suggested to make it semver-minor in #18138 (comment) without response, I am okay with that. The only reason I marked this semver-major is that it can break code which incorrectly used CCM before, because we did not account for that in previous releases, e.g., if you pass invalid CCM options to createCipheriv(). That would have worked in older releases as those options would have been ignored, but it will throw now.

[rvagg]

Yes, I'm fine with this addition and the API and being conservative on semver-major is probably a good idea although I wouldn't say it's essential.

However#19794 is more important than getting this landed for Node 10 and that PR is going to heavily impact this one. AES-CCM is going to have fairly minimal use due to the necessarily awkward API so this is pretty low priority (sorry @tniessen but I suspect you already understand this). Getting #19794 safely landed without holding it up even further is critical so I'd rather see this PR have to change to fit that PR rather than the other way around.

@tniessen could you do a quick investigation to see what changes might be required to sit on top of the 1.1.0 support @shigeki is adding?

I wouldn't discard the exact code in this PR btw, we could possibly relax the semver-major and land this on 8.x as it is now so it's not entirely useless as it is now. I think I'd prefer to wait until after 10.x goes live to make a call on 8.x support for CCM.

[tniessen]

the necessarily awkward API

Side note: On a par with OpenSSL.

so this is pretty low priority

It indeed seems to be, the lack of TSC attention was noticeable long before 1.1.0h was even released. There haven't been any changes to this PR in weeks.

could you do a quick investigation to see what changes might be required to sit on top of the 1.1.0 support @shigeki is adding?

It lands cleanly and lite CI passes: https://ci.nodejs.org/job/node-test-commit-lite/589/

[Trott]

R=@danbev perhaps?

[rvagg]

beautiful, if this lands cleanly with 1.1.0 then I see no other holdup, lgtm

[Trott]

Only CI that didn't finish last time was Linux, so here's a re-run of just that: https://ci.nodejs.org/job/node-test-commit-linux/17674/

[tniessen]

Thank you, @rvagg and @Trott!

I created tniessen@a191f6b by "landing" #19766, #18138 (this PR) and #19794 (in this order) on top of the current master. Again, there were no conflicts.

Additional CI for that commit: https://ci.nodejs.org/job/node-test-commit/17477/

Unless someone objects, I intend to land this along with #19766 today.

[rvagg]

CI is a mess, but not because of these changes. lgtm, let's get it merged, thanks for the hard work on this @tniessen.

[tniessen]

Landed in 1e07acd, there are no conflicts with #19794 according to GH interface. Thank you, everyone!

[tniessen]

@Trott I marked this PR as semver-major again given that multiple TSC members approved after nodejs/TSC#516 and that it landed in time for node 10. At the very least, this is semver-minor.

[tniessen]

openssl/openssl@67c81ec just landed and slightly relaxes one of the difficulties in using CCM, thus reducing the API differences between CCM and other modes slightly. I am not sure if it is worth cherry-picking since I assume we usually only cherry-pick security stuff.