Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: API changes needed for AES Counter with CBC-MAC (CCM) support #2383

Closed
brycekahle opened this issue Aug 14, 2015 · 19 comments
Closed
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. semver-minor PRs that contain new features and should be released in the next minor version.

Comments

@brycekahle
Copy link
Contributor

Overview

I've started work on adding AES CCM support to node. This is part of a larger goal of getting full DTLS support into node. Specifically I need AES_128_CCM_8 support. Until then, I'm working on a native module for the crypto support and helping @Rantanen improve his DTLS protocol module.

OpenSSL already supports this cipher, so I thought it was a matter of adding the necessary calls. I've since run into a complication with the order of calls required by OpenSSL and how the CipherBase class is structured.

I'm looking for some discussion and/or guidance on how to proceed with implementation.

CCM Requirements

CCM requires specifying 2 or 3 additional lengths for proper operation.

  • Length of the IV. This is already available as an argument and does not present any issues.
  • Length of the authentication tag
  • Length of the plaintext / ciphertext (only required if also specifying AAD)

Authentication Tag Length

The authentication tag length must be specified before calling EVP_CipherInit_ex with the key and iv parameters (source). If specified afterwards, OpenSSL returns a buffer full of zeroes.

Plaintext / Ciphertext length

The total plaintext or ciphertext length must be specified before setting the AAD (via EVP_CipherUpdate in cipher.setAAD) and before adding any plaintext (via EVP_CipherUpdate in cipher.update). If not specified before, cipher.setAAD will fail.

Proposed API Changes

In order to provide the 1 or 2 lengths required, some API changes will be needed. This is where some input would be greatly appreciated.

The requirement for the authentication tag length is needed before the final OpenSSL call of CipherBase::initiv. This means crypto.createCipheriv and crypto.createDecipheriv need additional optional parameters.

  • Add an optional authTagLength property to the options parameter of crypto.createCipheriv and crypto.createDecipheriv

The requirement for the plaintext / ciphertext length depends on using AAD, thus perhaps it makes sense to add an optional parameter to cipher.setAAD?

  • Change the signature of cipher.setAAD to cipher.setAAD(buffer, [length]). The confusing part here is that length is not the length of the AAD, but the plaintext / ciphertext length.

I'm open to any and all suggestions regarding API signature changes, or even new methods.

What does everyone think?

@brendanashworth brendanashworth added the crypto Issues and PRs related to the crypto subsystem. label Aug 14, 2015
@ChALkeR ChALkeR added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 15, 2015
@bnoordhuis
Copy link
Member

/cc @nodejs/crypto

@indutny
Copy link
Member

indutny commented Aug 15, 2015

I'm -1 for adding yet another argument to createCipheriv. I think it would be better to just pick it up from the options object that we accept as the last argument. And while we are here, I suggest to pick iv from it as well.

@brycekahle
Copy link
Contributor Author

@indutny Updated proposal for authTagLength. Thoughts on the setAAD change?

@indutny
Copy link
Member

indutny commented Sep 9, 2015

May I ask you to educate me a little bit about the length of cipher-/plain- text? Why is it required to specify, could it be different for the same cipher?

@brycekahle
Copy link
Contributor Author

@indutny
Copy link
Member

indutny commented Sep 10, 2015

I think this sounds like a good idea to put it in setAAD, it is a part of the authenticated data, right?

@shigeki
Copy link
Contributor

shigeki commented Sep 10, 2015

I also had the same question as fedor, the reason why the clear/cipher-text length is needed before setAAD and looked the openssl code and found that it is included in the first block sequence of CBC-MAC for authentication.

And I found one more requirement for CCM that EVP_Encrypt/DecryptUpdate of clear/cipher-text can be invoked only once. It is different from the case of GCM.

So we can have an another idea of api that setAAD just stores the data and the clear/cipher-text length and data of aad can be registered at the first cipher.update only in CCM case so that we need not to pass the clear/cipher-text length to setAAD and it is consistent with GCM. And we need to prohibit to execute cipher.update more than once in CCM.

@stemail23
Copy link

Does this explain why if I try to use aes-128-ccm algorithm, I get Trying to add data in unsupported state exception thrown?

var crypto = require('crypto');

var plaintext = crypto.randomBytes(16);
var key = crypto.randomBytes(16);

var encipher = crypto.createCipher('aes-128-ccm', key);
encipher.setAutoPadding(true);
var ciphertext = Buffer.concat([encipher.update(plaintext), encipher.final()]);

var decipher = crypto.createDecipher('aes-128-ccm', key);
decipher.setAutoPadding(true);
try {
    var deciphertext = Buffer.concat([decipher.update(ciphertext), decipher.final()]);
} catch(e) {
    console.error(e);
}

@shigeki
Copy link
Contributor

shigeki commented Sep 30, 2015

Yes, Node currently supports only GCM mode for AEAD. You cannot use CCM now.

@calvinmetcalf
Copy link
Contributor

another option would be something along the lines of #941 to create a non-streaming api for encrypting/decrypting. this would also avoid the footgun in gcm of decrypting before authenticating.

Strickly speaking we don't really need the authtag length, we get by in GCM by only allowing a single size.

@Partha-Mukherjee-G2H
Copy link

Hello Bryce (and others). I was checking to see if AES_128_CCM_8 is supported by Node.js crypto lib, and ran into this thread. Since it was initiated in August, Since I would like to use this cipher suite for a project I am working on, I am wondering if this work has been completed yet? Or if there is a date it is expected to be done? Thanks!

@brycekahle
Copy link
Contributor Author

@Partha-Mukherjee-G2H it is not supported yet by the core crypto API. You can use my module node-aes-ccm in the meantime.

@Partha-Mukherjee-G2H
Copy link

@brycekahle - I greatly appreciate the fast reply! I have not worked with native C++ modules in node.js before so I will take a good look at that.
I more really quick question for you:
In your notes you mention: "node-aes-ccm requires io.js >= 3.0 because we need OpenSSL 1.0.2d or later for AES CCM support.",
but the io.js website states that it (as of 3.3.31) has merged with node.js.
Do I still need to io.js installed to run the node-aes-ccm module?
Or is the latest stable version of node.js enough?
thanks!!

@brycekahle
Copy link
Contributor Author

I haven't tested with later versions of node, but it should work. If not, raise an issue on that repo and I can fix it.

@jasnell jasnell added the feature request Issues that request new features to be added to Node.js. label Jun 6, 2016
@Trott
Copy link
Member

Trott commented Jul 7, 2017

Is it the case right now that there's agreement that the feature makes sense, but no one planning to implement and submit a pull request?

Is someone willing to mentor a moderately crypto-savvy person who wanted to do this? (That's not me and I don't have anyone specific in mind. I'm just trying to figure out a way to move this forward.)

@tniessen
Copy link
Member

I am planning to implement this after landing #17566.

@eduardbme
Copy link
Contributor

Any update on this ?

This code still returns "Trying to add data in unsupported state".

var crypto = require('crypto');

var plaintext = 'some text';
var key = crypto.randomBytes(16);

var encipher = crypto.createCipher('aes-128-ccm', key);
encipher.setAutoPadding(true);
var ciphertext = Buffer.concat([encipher.update(plaintext), encipher.final()]);

var decipher = crypto.createDecipher('aes-128-ccm', key);
decipher.setAutoPadding(true);
try {
    var deciphertext = Buffer.concat([decipher.update(ciphertext), decipher.final()]);
    console.log(deciphertext.toString());
} catch(e) {
    console.error(e);
}

Is it possible to use a 'ccm' methods in node ?

@tniessen
Copy link
Member

@eduardbcom #18138 is awaiting TSC approval and will add full support for aes-128-ccm, aes-192-ccm and aes-256-ccm in non-FIPS mode.

@eduardbme
Copy link
Contributor

@tniessen got it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

No branches or pull requests