crypto: allocate more memory for cipher.update() #20370

yhwang · 2018-04-27T19:50:08Z

For some algorithms, they need extra 2x blocksize to store the ciphertext in
order to avoid invalid write. Also add a test case to verify it.

refs: #19655

Signed-off-by: Yihong Wang yh.wang@ibm.com

Checklist

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

yhwang · 2018-04-27T19:52:41Z

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

addaleax · 2018-04-27T19:54:30Z

@yhwang I think that makes sense, yes. 👍

/cc @nodejs/crypto

ryzokuken · 2018-04-27T20:24:11Z

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

ryzokuken · 2018-04-27T20:27:29Z

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

yhwang · 2018-04-27T21:11:54Z

@addaleax Let me do it. thanks

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

tniessen

LGTM as a fix.

tniessen · 2018-04-27T22:33:24Z

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.

ryzokuken · 2018-04-28T07:51:46Z

@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.

addaleax · 2018-04-28T09:46:39Z

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

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

Allocating double the block size doesn't look Obviously Correct to me as the envelope size has no direct relationship to the block size.

The reason this PR fixes the immediate issue is because openssl currently only supports AES and 3DES key wrap modes, where envelope size == block size by happenstance.

I think a better solution is to check first if EVP_CIPHER_mode(cipher) == EVP_CIPH_WRAP_MODE and then check the NID against a whitelist with EVP_CIPHER_nid(cipher) and throw an exception when we don't recognize it.

You can look up the NIDs with grep wrap deps/openssl/openssl/crypto/objects/obj_dat.h | grep NID but note that some ciphers are disabled in our builds so you probably want to intersect with node -p 'crypto.getCiphers().filter(s => s.toLowerCase().includes("wrap"))'.

Ideally, each whitelisted cipher should have a test case to make sure it's working as expected.

bnoordhuis · 2018-04-28T10:04:00Z

test/parallel/test-crypto-des3-wrap.js

+// then plaintext to store ciphertext.
+const test = {
+  key: crypto.randomBytes(24),
+  iv: crypto.randomBytes(0),


I'd use fixed strings to keep the test deterministic.

tniessen · 2018-04-28T14:56:41Z

@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.)

bnoordhuis · 2018-04-28T16:34:25Z

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.

yhwang · 2018-04-29T05:10:15Z

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.

yhwang · 2018-05-01T21:21:41Z

@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?

tniessen · 2018-05-01T21:25:46Z

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.

yhwang · 2018-05-01T22:36:42Z

@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.

yhwang · 2018-05-01T23:16:02Z

@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?

tniessen

LGTM assuming you checked the returned value manually and CI passes. I'd still prefer to see this behavior of EVP_CipherUpdate documented just so we don't rely on undefined behavior, but that's probably beyond our power.

tniessen · 2018-05-02T08:24:37Z

src/node_crypto.cc

+  *out_len = 0;
+  int buff_len = len + EVP_CIPHER_CTX_block_size(ctx_);
+  // For key wrapping algorithms, get output size by calling
+  // EVP_CipherUpdate() with null output


Nit: Period at the end of the sentence.

@tniessen fixed. Thanks!

bnoordhuis · 2018-05-02T11:30:47Z

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.

For key wrapping algorithms, calling EVP_CipherUpdate() with null output could obtain the size for the ciphertext. Then use the returned size to allocate output buffer. Also add a test case to verify des3-wrap. Signed-off-by: Yihong Wang <yh.wang@ibm.com>

yhwang · 2018-05-02T16:52:40Z

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

For key wrapping algorithms, calling EVP_CipherUpdate() with null output could obtain the size for the ciphertext. Then use the returned size to allocate output buffer. Also add a test case to verify des3-wrap. Signed-off-by: Yihong Wang <yh.wang@ibm.com> PR-URL: #20370 Fixes: #19655 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

tniessen · 2018-05-04T09:18:00Z

Landed in f7cdeba.

For key wrapping algorithms, calling EVP_CipherUpdate() with null output could obtain the size for the ciphertext. Then use the returned size to allocate output buffer. Also add a test case to verify des3-wrap. Signed-off-by: Yihong Wang <yh.wang@ibm.com> PR-URL: #20370 Fixes: #19655 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis · 2018-05-08T07:54:27Z

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

For key wrapping algorithms, calling EVP_CipherUpdate() with null output could obtain the size for the ciphertext. Then use the returned size to allocate output buffer. Also add a test case to verify des3-wrap. Signed-off-by: Yihong Wang <yh.wang@ibm.com> PR-URL: #20370 Fixes: #19655 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

For key wrapping algorithms, calling EVP_CipherUpdate() with null output could obtain the size for the ciphertext. Then use the returned size to allocate output buffer. Also add a test case to verify des3-wrap. Signed-off-by: Yihong Wang <yh.wang@ibm.com> PR-URL: nodejs#20370 Fixes: nodejs#19655 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

For key wrapping algorithms, calling EVP_CipherUpdate() with null output could obtain the size for the ciphertext. Then use the returned size to allocate output buffer. Also add a test case to verify des3-wrap. Signed-off-by: Yihong Wang <yh.wang@ibm.com> Backport-PR-URL: #20706 PR-URL: #20370 Fixes: #19655 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Apr 27, 2018

yhwang force-pushed the crypto-invalid-write branch 2 times, most recently from 06718c4 to 4c995d4 Compare April 27, 2018 22:09

tniessen approved these changes Apr 27, 2018

View reviewed changes

tniessen added the security Issues and PRs related to security. label Apr 27, 2018

bnoordhuis reviewed Apr 28, 2018

View reviewed changes

yhwang force-pushed the crypto-invalid-write branch from 4c995d4 to 308d570 Compare May 1, 2018 23:08

tniessen approved these changes May 2, 2018

View reviewed changes

bnoordhuis approved these changes May 2, 2018

View reviewed changes

yhwang force-pushed the crypto-invalid-write branch from 308d570 to a9b2153 Compare May 2, 2018 16:50

yhwang added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 2, 2018

jasnell approved these changes May 4, 2018

View reviewed changes

tniessen closed this May 4, 2018

yhwang deleted the crypto-invalid-write branch May 4, 2018 16:30

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

MylesBorins mentioned this pull request May 9, 2018

[v10.x] src: more automatic memory management in node_crypto.cc #20609

Closed

2 tasks

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 10, 2018

targos mentioned this pull request May 12, 2018

Need help to backport crypto changes to v10.x-staging #20691

Closed

tniessen mentioned this pull request May 13, 2018

(v10.x backport) Backport 12 crypto commits #20706

Closed

addaleax mentioned this pull request May 14, 2018

v10.2.0 proposal #20724

Merged

targos added the backported-to-v10.x label May 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: allocate more memory for cipher.update() #20370

crypto: allocate more memory for cipher.update() #20370

yhwang commented Apr 27, 2018

yhwang commented Apr 27, 2018

addaleax commented Apr 27, 2018

ryzokuken commented Apr 27, 2018

ryzokuken commented Apr 27, 2018 •

edited

Loading

yhwang commented Apr 27, 2018

tniessen left a comment

tniessen commented Apr 27, 2018 •

edited

Loading

ryzokuken commented Apr 28, 2018

addaleax commented Apr 28, 2018

bnoordhuis left a comment

bnoordhuis Apr 28, 2018

tniessen commented Apr 28, 2018

bnoordhuis commented Apr 28, 2018

yhwang commented Apr 29, 2018

yhwang commented May 1, 2018

tniessen commented May 1, 2018

yhwang commented May 1, 2018

yhwang commented May 1, 2018

tniessen left a comment

tniessen May 2, 2018

yhwang May 2, 2018

bnoordhuis commented May 2, 2018

yhwang commented May 2, 2018

tniessen commented May 4, 2018

bnoordhuis commented May 8, 2018

crypto: allocate more memory for cipher.update() #20370

crypto: allocate more memory for cipher.update() #20370

Conversation

yhwang commented Apr 27, 2018

Checklist

yhwang commented Apr 27, 2018

addaleax commented Apr 27, 2018

ryzokuken commented Apr 27, 2018

ryzokuken commented Apr 27, 2018 • edited Loading

yhwang commented Apr 27, 2018

tniessen left a comment

Choose a reason for hiding this comment

tniessen commented Apr 27, 2018 • edited Loading

ryzokuken commented Apr 28, 2018

addaleax commented Apr 28, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Apr 28, 2018

Choose a reason for hiding this comment

tniessen commented Apr 28, 2018

bnoordhuis commented Apr 28, 2018

yhwang commented Apr 29, 2018

yhwang commented May 1, 2018

tniessen commented May 1, 2018

yhwang commented May 1, 2018

yhwang commented May 1, 2018

tniessen left a comment

Choose a reason for hiding this comment

tniessen May 2, 2018

Choose a reason for hiding this comment

yhwang May 2, 2018

Choose a reason for hiding this comment

bnoordhuis commented May 2, 2018

yhwang commented May 2, 2018

tniessen commented May 4, 2018

bnoordhuis commented May 8, 2018

ryzokuken commented Apr 27, 2018 •

edited

Loading

tniessen commented Apr 27, 2018 •

edited

Loading