crypto: fix memory leak, reduce heap allocations, clean up #14122

bnoordhuis · 2017-07-07T13:36:15Z

Also of note:

The EVP_CIPHER can be reconstructed from the EVP_CIPHER_CTX instance,
no need to store it separately.

This brought to light the somewhat dubious practice of accessing the
EVP_CIPHER after the EVP_CIPHER_CTX instance had been destroyed.

It's mostly harmless due to the static nature of built-in EVP_CIPHER
instances but it segfaults when the cipher is provided by an ENGINE
and the ENGINE is unloaded because its reference count drops to zero.

I'll clean up the commit logs some more before landing.

CI: https://ci.nodejs.org/job/node-test-pull-request/9027/

bnoordhuis · 2017-07-10T11:00:27Z

I added a commit that fixes the DH memory leak reported in #8377.

CI: https://ci.nodejs.org/job/node-test-pull-request/9056/

jasnell

Largely rubber-stamp LGTM. Looked through the code and didn't see anything that stood out but didn't do a thorough review.

bnoordhuis · 2017-07-12T16:55:42Z

Bumped DH key size in test to 1024 to pacify FIPS. New CI: https://ci.nodejs.org/job/node-test-pull-request/9094/

Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.Cipher#final() - crypto.Cipher#update() - crypto.Cipheriv#final() - crypto.Cipheriv#update() - crypto.Decipher#final() - crypto.Decipher#update() - crypto.Decipheriv#final() - crypto.Decipheriv#update() - crypto.privateDecrypt() - crypto.privateEncrypt() - crypto.publicDecrypt() - crypto.publicEncrypt()

Put the 8 kB initial buffer on the stack first and don't copy it to the heap until its exact size is known (which is normally much smaller.)

Fix a memory leak by removing the heap allocation altogether. Fixes: nodejs#13917

The EVP_CIPHER can be reconstructed from the EVP_CIPHER_CTX instance, no need to store it separately. This brought to light the somewhat dubious practice of accessing the EVP_CIPHER after the EVP_CIPHER_CTX instance had been destroyed. It's mostly harmless due to the static nature of built-in EVP_CIPHER instances but it segfaults when the cipher is provided by an ENGINE and the ENGINE is unloaded because its reference count drops to zero.

The cipher kind doesn't change over the lifetime of the cipher so make it const.

Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.CryptoStream#getSession() - tls.TLSSocket#getSession()

Add a test that ensures the second call to .digest() returns an empty HMAC, like it did before. No comment on whether that is the right behavior or not.

Replace allocate + Encode() + free patterns by calls to Malloc + the Buffer::New() overload that takes ownership of the pointer. Avoids unnecessary heap allocations and copying around of data. DRY the accessor functions for the prime, generator, public key and private key properties; deletes about 40 lines of quadruplicated code.

This also renames a misnamed variable `error_` to `success_`.

Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: nodejs#8377

bnoordhuis · 2017-07-17T11:29:50Z

One more rebase + CI: https://ci.nodejs.org/job/node-test-pull-request/9192/

bnoordhuis · 2017-07-17T12:11:23Z

Stress test to see if the async-hooks/test-tlswrap failure on the osx1010 bot is a flake or not: https://ci.nodejs.org/job/node-stress-single-test/1319/

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: #8377 PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.Cipher#final() - crypto.Cipher#update() - crypto.Cipheriv#final() - crypto.Cipheriv#update() - crypto.Decipher#final() - crypto.Decipher#update() - crypto.Decipheriv#final() - crypto.Decipheriv#update() - crypto.privateDecrypt() - crypto.privateEncrypt() - crypto.publicDecrypt() - crypto.publicEncrypt() PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Put the 8 kB initial buffer on the stack first and don't copy it to the heap until its exact size is known (which is normally much smaller.) PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fix a memory leak by removing the heap allocation altogether. Fixes: #13917 PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

The EVP_CIPHER can be reconstructed from the EVP_CIPHER_CTX instance, no need to store it separately. This brought to light the somewhat dubious practice of accessing the EVP_CIPHER after the EVP_CIPHER_CTX instance had been destroyed. It's mostly harmless due to the static nature of built-in EVP_CIPHER instances but it segfaults when the cipher is provided by an ENGINE and the ENGINE is unloaded because its reference count drops to zero. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

The cipher kind doesn't change over the lifetime of the cipher so make it const. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.CryptoStream#getSession() - tls.TLSSocket#getSession() PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Add a test that ensures the second call to .digest() returns an empty HMAC, like it did before. No comment on whether that is the right behavior or not. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Replace allocate + Encode() + free patterns by calls to Malloc + the Buffer::New() overload that takes ownership of the pointer. Avoids unnecessary heap allocations and copying around of data. DRY the accessor functions for the prime, generator, public key and private key properties; deletes about 40 lines of quadruplicated code. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

This also renames a misnamed variable `error_` to `success_`. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: #8377 PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-08-16T07:29:00Z

@bnoordhuis is this something we would want to backport to v6.x?

bnoordhuis · 2017-08-16T09:08:40Z

Yes.

MylesBorins · 2017-09-19T17:13:59Z

@bnoordhuis this is going to require a manual backport. Would you be willing to help with this? We could choose to simply not land the commits that have conflicts, up for suggestion on best path forward

MylesBorins · 2017-10-16T21:19:52Z

ping @bnoordhuis

bnoordhuis · 2017-10-29T12:20:09Z

Depends on #16587, the new internal APIs need to be back-ported first.

MylesBorins · 2017-11-14T19:18:17Z

@bnoordhuis #16587 has landed, would you be able to backport this now?

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 Jul 7, 2017

addaleax approved these changes Jul 7, 2017

View reviewed changes

bnoordhuis force-pushed the cleanup-crypto branch from 8047566 to 67911e3 Compare July 10, 2017 10:58

bnoordhuis mentioned this pull request Jul 10, 2017

Memory leak in DiffieHellman.set{Privat,Public}Key() and some suggestions #8377

Closed

jasnell approved these changes Jul 10, 2017

View reviewed changes

bnoordhuis force-pushed the cleanup-crypto branch from 67911e3 to eaf2825 Compare July 12, 2017 16:54

bnoordhuis added 18 commits July 17, 2017 12:35

src: remove unneeded const_cast

df1e3e8

src: avoid heap allocation in sign.final()

87fc516

Put the 8 kB initial buffer on the stack first and don't copy it to the heap until its exact size is known (which is normally much smaller.)

src: don't heap allocate GCM cipher auth tag

647370a

Fix a memory leak by removing the heap allocation altogether. Fixes: nodejs#13917

src: remove unused Local

5ae2cb8

src: make CipherBase::kind_ const

dd6f4a9

The cipher kind doesn't change over the lifetime of the cipher so make it const.

src: remove extra heap allocation in GetSession()

d383042

Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.CryptoStream#getSession() - tls.TLSSocket#getSession()

src: avoid heap allocation in hmac.digest()

496ef4b

Add a test that ensures the second call to .digest() returns an empty HMAC, like it did before. No comment on whether that is the right behavior or not.

src: refactor PBKDF2Request

5fd09df

src: make array arg length compile-time checkable

160b60f

src: avoid heap allocation in crypto.pbkdf2()

3818792

src: remove PBKDF2Request::release()

0ce90a9

src: simplify PBKDF2Request

1cfd02e

This also renames a misnamed variable `error_` to `success_`.

src: guard against double free in randomBytes()

b8e3a3e

src: reduce allocations in exportPublicKey()

083ccd2

src: fix memory leak in DH key setters

e4ac1b6

Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: nodejs#8377

bnoordhuis force-pushed the cleanup-crypto branch from eaf2825 to e4ac1b6 Compare July 17, 2017 11:28

addaleax pushed a commit that referenced this pull request Jul 18, 2017

src: guard against double free in randomBytes()

e4b7019

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

addaleax pushed a commit that referenced this pull request Jul 18, 2017

src: reduce allocations in exportPublicKey()

0bbdb78

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: remove unneeded const_cast

63a640c

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: remove unused Local

e59858b

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: refactor PBKDF2Request

3286f6a

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: make array arg length compile-time checkable

36e1351

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: avoid heap allocation in crypto.pbkdf2()

2d2d759

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: remove PBKDF2Request::release()

7d44590

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: simplify PBKDF2Request

5caaf83

This also renames a misnamed variable `error_` to `success_`. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: guard against double free in randomBytes()

62be8ef

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

src: reduce allocations in exportPublicKey()

83234db

PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

refack mentioned this pull request Jul 22, 2017

test: test-crypto-dh-leak flaky #14424

Closed

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

MylesBorins added the backport-requested-v6.x label Sep 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: fix memory leak, reduce heap allocations, clean up #14122

crypto: fix memory leak, reduce heap allocations, clean up #14122

bnoordhuis commented Jul 7, 2017 •

edited

Loading

bnoordhuis commented Jul 10, 2017 •

edited

Loading

jasnell left a comment

bnoordhuis commented Jul 12, 2017

bnoordhuis commented Jul 17, 2017

bnoordhuis commented Jul 17, 2017

MylesBorins commented Aug 16, 2017

bnoordhuis commented Aug 16, 2017

MylesBorins commented Sep 19, 2017

MylesBorins commented Oct 16, 2017

bnoordhuis commented Oct 29, 2017

MylesBorins commented Nov 14, 2017

crypto: fix memory leak, reduce heap allocations, clean up #14122

crypto: fix memory leak, reduce heap allocations, clean up #14122

Conversation

bnoordhuis commented Jul 7, 2017 • edited Loading

bnoordhuis commented Jul 10, 2017 • edited Loading

jasnell left a comment

Choose a reason for hiding this comment

bnoordhuis commented Jul 12, 2017

bnoordhuis commented Jul 17, 2017

bnoordhuis commented Jul 17, 2017

MylesBorins commented Aug 16, 2017

bnoordhuis commented Aug 16, 2017

MylesBorins commented Sep 19, 2017

MylesBorins commented Oct 16, 2017

bnoordhuis commented Oct 29, 2017

MylesBorins commented Nov 14, 2017

bnoordhuis commented Jul 7, 2017 •

edited

Loading

bnoordhuis commented Jul 10, 2017 •

edited

Loading