Skip to content

crypto: Deprecate createCipher for createCipheriv #13941

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.

Sign up for GitHub

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

Closed
iangcarroll wants to merge 13 commits into from
Closed

crypto: Deprecate createCipher for createCipheriv #13941

Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cc8976b
crypto: Deprecate createCipher for createCipheriv
Jun 27, 2017
3d0b5a5
crypto: remove unused mode variable
Jun 27, 2017
1e810d0
crypto: mark createCipher as deprecated
Jul 4, 2017
8f2b81f
crypto: add legacy KDF methods
Jul 4, 2017
7f913ec
crypto: update tests for createCipher
Jul 9, 2017
b7abe64
crypto: fix IV buffer length
Jul 9, 2017
b16018a
crypto: fix removal error
Jul 9, 2017
39a2112
crypto: Fix createCipheriv with null IVs and FIPS mode.
Sep 29, 2017
d0f2778
crypto: fix style issues
Sep 29, 2017
14e0566
doc: add crypto.createCipher deprecation information
Sep 29, 2017
3f0fdb2
crypto: downgrade createCipher deprecation to runtime
Sep 29, 2017
b0fe314
crypto: improve documentation of createCipher changes
Oct 8, 2017
0025983
crypto: improve createCipher testing
Oct 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3340,7 +3340,6 @@ void CipherBase::Init(const char* cipher_type,
}

unsigned char key[EVP_MAX_KEY_LENGTH];
unsigned char iv[EVP_MAX_IV_LENGTH];

int key_len = EVP_BytesToKey(cipher_,
EVP_md5(),
Expand All @@ -3349,11 +3348,19 @@ void CipherBase::Init(const char* cipher_type,
key_buf_len,
1,
key,
iv);
nullptr);

EVP_CIPHER_CTX_init(&ctx_);
const bool encrypt = (kind_ == kCipher);

EVP_CipherInit_ex(&ctx_, cipher_, nullptr, nullptr, nullptr, encrypt);

const int iv_len = EVP_CIPHER_iv_length(cipher_);

if (encrypt && iv_len != 0) {
return env()->ThrowError("crypto.createCipher() is no longer supported with ciphers that require initialization vectors. Generate a random IV and pass it to crypto.createCipheriv().");
}
Copy link
Member

@jasnell jasnell Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No comments on whether this is something we should do (ping @nodejs/crypto) ... but this is not the correct way of doing it. There are two kinds of deprecations we use: documentation and runtime. A runtime deprecation uses either the util.deprecate() or process.emitWarning('...', 'DeprecationWarning') API to emit a warning at runtime, but the code still continues to operate as it did in every other respect.

Copy link
Author

@iangcarroll iangcarroll Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My PR intends to immediately prohibit the use of this API for ciphers that require an IV. The deprecation of this API entirely hasn’t been done yet. I’m guessing that would go in lib/crypto.js?

Copy link
Member

@jasnell jasnell Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, and that's the point: Our deprecation policy requires that APIs go through a proper deprecation cycle so this PR would not be able to land as is. The first step is to do a docs or runtime deprecation as a semver-major, which would go into Node.js 9.0.0.

Copy link
Member

@bnoordhuis bnoordhuis Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have provisions for making exceptions for security-related issues but as this is more about protecting users against themselves, it's up for debate whether those provisions apply.

I'm personally leaning towards 'acceptable in node 9' (raising an exception, that is) but I won't hold it against anyone who feels differently.

Copy link
Contributor

@shigeki shigeki Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prohibits users from using not only counter modes but also cbc modes. I'm not sure if the cbc mode has a security issue in this API.


if (!EVP_CIPHER_CTX_set_key_length(&ctx_, key_len)) {
EVP_CIPHER_CTX_cleanup(&ctx_);
return env()->ThrowError("Invalid key length");
Expand All @@ -3363,7 +3370,7 @@ void CipherBase::Init(const char* cipher_type,
nullptr,
nullptr,
reinterpret_cast<unsigned char*>(key),
reinterpret_cast<unsigned char*>(iv),
nullptr,
kind_ == kCipher);
initialised_ = true;
}
Expand Down Expand Up @@ -3443,14 +3450,22 @@ void CipherBase::InitIv(const FunctionCallbackInfo<Value>& args) {

THROW_AND_RETURN_IF_NOT_STRING(args[0], "Cipher type");
THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Key");
THROW_AND_RETURN_IF_NOT_BUFFER(args[2], "IV");

if (!Buffer::HasInstance(args[2]) && !args[2]->IsNull()) {
return env->ThrowTypeError("IV must be a buffer or null");
}
Copy link
Member

@jasnell jasnell Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Author

@iangcarroll iangcarroll Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this API will now accept null as a valid IV.

Copy link
Author

@iangcarroll iangcarroll Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This means you don’t need to pass an empty Buffer for ciphers where there is no IV)

Copy link
Member

@jasnell jasnell Jun 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The better approach here then, would be to do the type checking at the js layer and use the internal/errors API to generate a proper ERR_INVALID_ARG_TYPE error.

Copy link
Member

@tniessen tniessen Jun 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this about createCipheriv? Why would any sane person come up with the idea to use a function createCipheriv for ciphers where there is no IV?


const node::Utf8Value cipher_type(env->isolate(), args[0]);
ssize_t key_len = Buffer::Length(args[1]);
const char* key_buf = Buffer::Data(args[1]);
ssize_t iv_len = Buffer::Length(args[2]);
const char* iv_buf = Buffer::Data(args[2]);
cipher->InitIv(*cipher_type, key_buf, key_len, iv_buf, iv_len);

if (args[2]->IsNull()) {
cipher->InitIv(*cipher_type, key_buf, key_len, nullptr, 0);
} else {
ssize_t iv_len = Buffer::Length(args[2]);
const char* iv_buf = Buffer::Data(args[2]);
cipher->InitIv(*cipher_type, key_buf, key_len, iv_buf, iv_len);
}
}


Expand Down