one question: should we put an assertion to check the out_len after EVP_CipherUpdate() and see if it exceeds the original allocated size?

@yhwang I think that makes sense, yes. 👍

/cc @nodejs/crypto

Is there no 3DES support in Node 9? I tried the test in Node 9 and it threw Error: Unknown cipher.

Okay, I think it was introduced in OpenSSL 1.1.0h which was bumped in #19794 (66cb29e).

@addaleax Let me do it. thanks

@ryzokuken does it means that I should label this with dont-land-v9.x and others?

Marking this as security as I wrote a report for the Node.js security team a couple of days ago. An attacker can - hypothetically - write eight bytes into (un)allocated heap and at least partially control their contents.

@yhwang I think @tniessen and @addaleax would know better. AFAIK, 3des-wrap was introduced in Node.js 10 (it throws with unknownCipher in Node.js 6, 8 and 9), so, yeah. It should be semver-patch (because it throws on Node.js 10 with):

~/Code/temp/node
❯ node crypto.js
node(79558,0x7fffa1d72380) malloc: *** error for object 0x10250fd60: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
[1]    79558 abort      node crypto.js

but yeah, label it so that it doesn't land on Node.js 6, 8 and 9 (if 3des-wrap is the only cipher this one applies on, though).

I hope this clarifies everything.

I mean, the dont-land labels depend on a couple factors – as in, what is the negative impact if we do merge this to an LTS line? Does it make backporting there easier?

The only downside I can think of is slightly higher memory consumption, but as I understand it these chunks are not all that large to begin with. Plus, we could reduce that problem by doing a shrinking Realloc() call in case of success, I guess?

@bnoordhuis I was thinking the same thing, but a better solution can still be implemented later on. This patch at least makes sure that we don't overwrite unallocated heap memory. (And yes, the connection between envelope size and blocksize is debatable.)

I can live with that but I'd suggest the following:

• expand the comment to describe what I posted above
• add a CHECK that asserts the NID is a known one when it's a key wrap mode cipher
• add a regression test for each cipher

That way there can be no hidden footguns or time bombs.

@yhwang Ping me if you have questions or want help.

I want to say: I love your comments and I will update my change according to your comments. @bnoordhuis I will ping you if I need help.

I have a family event this weekend and will be back to this next Tuesday. I will do the change then.

@bnoordhuis

key wrap algorithms overflow because of the envelope that's added?

I am not sure if it's the envelope. Based on the spec it's the sha1 of the key: https://tools.ietf.org/html/rfc3217#section-2

For the key wrap algorithms, seems you can call EVP_CipherUpdate() and pass nullptr for the out parameter. Then it will return you the proper size you need to allocate. Should we do that?

add a CHECK that asserts the NID is a known one when it's a key wrap mode cipher

It shouldn't be specific to key wrap algorithms. We should do the check for the algorithms that we supports, right?

For the key wrap algorithms, seems you can call EVP_CipherUpdate() and pass nullptr for the out parameter. Then it will return you the proper size you need to allocate. Should we do that?

If that works, that would be great, but I couldn't find a conclusive solution in the documentation.

@tniessen I can't find documentation either. I read the logic in aes_wrap_cipher() and des_ede3_wrap_cipher(). And in cms_kek_cipher(), it calls EVP_CipherUpdate() with null output to obtain output length.

@tniessen I updated the change to get output length before memory allocation.

@bnoordhuis please let me know should I add CHECK for key wrapping? (my question is should we do the CHECK for all supported algorithms but not only for key wrapping? If that's the case, should I do it separately?)

For aes128-wrap, aes192-wrap and aes254-wrap, can I add test cases for these algorithms later in another PR?

I am not sure if it's the envelope. Based on the spec it's the sha1 of the key: https://tools.ietf.org/html/rfc3217#section-2

That's 3DES. AES is different: https://tools.ietf.org/html/rfc3394#page-4. I use 'envelope' as a catch-all for the key wrap data that's added.

It shouldn't be specific to key wrap algorithms

I think they're the only ones that add extra data. But if there's a reliable generic way of detecting that upfront, so much the better.

CI: https://ci.nodejs.org/job/node-test-commit/18189/

Landed in f7cdeba.

Back-porters, this should land along with #20587.