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: implement basic secure heap support #36779

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 4, 2021

Adds two new command line arguments:

  • --secure-heap=n, which causes node.js to initialize
    an openssl secure heap of n bytes on openssl initialization.
  • --secure-heap-min=n, which specifies the minimum allocation
    from the secure heap.
  • A new method crypto.secureHeapUsed() that returns details
    about the total and used secure heap allocation.
node --secure-heap=16384 --secure-heap-min=128

The secure heap is an openssl feature that allows certain kinds
of potentially sensitive information (such as private key
BigNums) to be allocated from a dedicated memory area that is
protected against pointer over- and underruns.

The secure heap is a fixed size, so it's important that users
pick a large enough size to cover the crypto operations they
intend to utilize.

The secure heap is disabled by default.

Signed-off-by: James M Snell jasnell@gmail.com

Refs: #36729

/cc @nodejs/crypto @devsnek

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 4, 2021
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 4, 2021
@devsnek
Copy link
Member

devsnek commented Jan 4, 2021

Would it be possible to ask openssl if there's a way we can do this without having to manually specify buffer sizes? I realize resizing allocations is a scary thing in terms of memory security so if they're against it I'd be okay with this but still very sad.

@jasnell
Copy link
Member Author

jasnell commented Jan 4, 2021

You certainly could ask but it would likely be a very long time before that landed if at all.

@nodejs-github-bot

This comment has been minimized.

@addaleax addaleax added crypto Issues and PRs related to the crypto subsystem. cli Issues and PRs related to the Node.js command line interface. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 4, 2021
doc/api/cli.md Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
doc/node.1 Outdated Show resolved Hide resolved
doc/node.1 Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
@tniessen
Copy link
Member

tniessen commented Jan 5, 2021

When I started working on this feature, I decided not to use OpenSSL's implementation, but to rewrite OpenSSL to accept custom secure heap implementations instead. I never finished it and moved the repository around a bit so I can't find it right now, but that is still an option if OpenSSL's implementation is too restrictive. I used a buddy-style allocator with dynamic memory allocations and memory locking.

Allocation failures are a severe problem. OpenSSL will store all kinds of data in secure memory, and denial of service via secure memory exhaustion is a potential attack vector.

@jasnell
Copy link
Member Author

jasnell commented Jan 6, 2021

Yeah, the OpenSSL implementation leaves a bit to be desired. It may be worthwhile pulling @richsalz into the conversation here :-)

src/node_options.cc Outdated Show resolved Hide resolved
@richsalz
Copy link

richsalz commented Jan 6, 2021

The OpenSSL secure memory area implementation is limited (the code is a decade old, back when 512 and 1K RSA keys were all an https server needed). Probably the biggest limitations is that there is only a single fixed-size arena. That could be fixed by allocating additional heaps and linking them, all done at the "sh_xxx" layer in that one file. Secondarily fixing the "round to power of two" allocation would be a good idea.

OpenSSL doesn't put "all sorts of information" in the heap, only private key material. It is possible to have this as a DoS attack; in practice we haven't seen this at Akamai on our servers.

Hope this helps. If @tniessen finds his code, I'd be happy to review it and help push it upstream.

@jasnell
Copy link
Member Author

jasnell commented Jan 6, 2021

Thanks @richsalz :) one other question for you .. do you have any references for studies or papers that talk about akamai's experience with secure heap? It would be great to help inform the discussion (for instance, anything that discusses ideal secure heap sizes for various use cases)

@richsalz
Copy link

richsalz commented Jan 6, 2021

No, we never wrote anything up beyond the commit message, sorry. For a long time secure heap was a sales differentiator for us; after heartbleed we decided to go "all in on openssl" and pushed the code upstream, with improvements (all private key material) and cleanups (it was way too tricky, and knew about ASN1 field names in RSA keys).

@tniessen
Copy link
Member

tniessen commented Jan 6, 2021

OpenSSL doesn't put "all sorts of information" in the heap, only private key material.

I didn't mean any offense towards OpenSSL! :) I meant that, from the perspective of JavaScript, simple object allocations could use memory in the secure memory area if they wrap private keys, and that freeing secure memory can be delayed until garbage collection occurs. JavaScript allows very little control over memory management.

@richsalz
Copy link

richsalz commented Jan 6, 2021

No offense taken. :) Yes, if JS is going to put whole objects in the secure heap and can't do just key material it's not gonna be happy.

@jasnell
Copy link
Member Author

jasnell commented Jan 6, 2021

Well, there definitely won't be whole objects in there :-) ... We have KeyObject objects that are allocated across several heaps (there's the JS Object wrapper that is part of V8's heap, there's the C++ KeyObjectHandle that is created in the process heap, and there's the EVP_PKEY that is retained. Only the EVP_PKEY (if it's a private key) would be using the secure heap here. We're also considering use of the secure heap to isolate cached entropy (see #36729).

@richsalz
Copy link

richsalz commented Jan 6, 2021

For key material, there is a flag to the "BN/BIGNUM" datatype that indicates this data should be stored on the secure heap. All key objects use the flags correctly. So if you're using EVP_PKEY it's automatic.

As for storing other things, you're kinda on your own. :)

@jasnell
Copy link
Member Author

jasnell commented Jan 6, 2021

As for storing other things, you're kinda on your own. :)

Yep :-) ... the idea in this PR is to make use of the secure heap entirely opt-in, with the user determining the size on process startup. Really appreciate you jumping in @richsalz :-)

@tniessen @devsnek ... even if we add this now then change later to a dynamically sized heap, the command line arguments should be able to remain largely the same. With a dynamically allocated secure heap, --secure-heap=N could still indicate the initial size of the initial arena. We can introduce a --secure-heap-max=N later to enforce an upper-bound. If we eventually get around the power-of-two allocations when we decide whether to deprecate --secure-heap-min=N. But, until then, this still provides useful functionality I think. Even the proposed JS API bit should work well in a dynamic allocation approach later without requiring changes.

@jasnell
Copy link
Member Author

jasnell commented Jan 9, 2021

This is ready to go, just waiting for another signoff

doc/api/cli.md Outdated Show resolved Hide resolved
Signed-off-by: James M Snell <jasnell@gmail.com>
Adds two new command line arguments:

* `--secure-heap=n`, which causes node.js to initialize
  an openssl secure heap of `n` bytes on openssl initialization.
* `--secure-heap-min=n`, which specifies the minimum allocation
  from the secure heap.
* A new method `crypto.secureHeapUsed()` that returns details
  about the total and used secure heap allocation.

The secure heap is an openssl feature that allows certain kinds
of potentially sensitive information (such as private key
BigNums) to be allocated from a dedicated memory area that is
protected against pointer over- and underruns.

The secure heap is a fixed size, so it's important that users
pick a large enough size to cover the crypto operations they
intend to utilize.

The secure heap is disabled by default.

Signed-off-by: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Jan 11, 2021

Landed in a4fce32...03c0564

@jasnell jasnell closed this Jan 11, 2021
jasnell added a commit that referenced this pull request Jan 11, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36779
Refs: #36729
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
jasnell added a commit that referenced this pull request Jan 11, 2021
Adds two new command line arguments:

* `--secure-heap=n`, which causes node.js to initialize
  an openssl secure heap of `n` bytes on openssl initialization.
* `--secure-heap-min=n`, which specifies the minimum allocation
  from the secure heap.
* A new method `crypto.secureHeapUsed()` that returns details
  about the total and used secure heap allocation.

The secure heap is an openssl feature that allows certain kinds
of potentially sensitive information (such as private key
BigNums) to be allocated from a dedicated memory area that is
protected against pointer over- and underruns.

The secure heap is a fixed size, so it's important that users
pick a large enough size to cover the crypto operations they
intend to utilize.

The secure heap is disabled by default.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36779
Refs: #36729
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@richardlau
Copy link
Member

It looks like the new test added in this PR is consistently failing the ASAN builds.

danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36779
Refs: #36729
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Adds two new command line arguments:

* `--secure-heap=n`, which causes node.js to initialize
  an openssl secure heap of `n` bytes on openssl initialization.
* `--secure-heap-min=n`, which specifies the minimum allocation
  from the secure heap.
* A new method `crypto.secureHeapUsed()` that returns details
  about the total and used secure heap allocation.

The secure heap is an openssl feature that allows certain kinds
of potentially sensitive information (such as private key
BigNums) to be allocated from a dedicated memory area that is
protected against pointer over- and underruns.

The secure heap is a fixed size, so it's important that users
pick a large enough size to cover the crypto operations they
intend to utilize.

The secure heap is disabled by default.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36779
Refs: #36729
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
danielleadams added a commit that referenced this pull request Jan 12, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 12, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 13, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 13, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 13, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 14, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 14, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) (#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) (#36603)
* crypto:
  * implement basic secure heap support (James M Snell) (#36779)
  * fixup bug in keygen error handling (James M Snell) (#36779)
  * introduce X509Certificate API (James M Snell) (#36804)
  * implement randomuuid (James M Snell) (#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) (#36793)
  * add dnlup to collaborators (Daniele Belardi) (#36849)
  * add panva to collaborators (Filip Skokan) (#36802)
  * add yashLadha to collaborator (Yash Ladha) (#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) (#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) (#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) (#34291)
* v8:
  * fix native  constructors (ExE Boss) (#36549)
danielleadams added a commit that referenced this pull request Jan 15, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) (#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) (#36603)
* crypto:
  * implement basic secure heap support (James M Snell) (#36779)
  * fixup bug in keygen error handling (James M Snell) (#36779)
  * introduce X509Certificate API (James M Snell) (#36804)
  * implement randomuuid (James M Snell) (#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) (#36793)
  * add dnlup to collaborators (Daniele Belardi) (#36849)
  * add panva to collaborators (Filip Skokan) (#36802)
  * add yashLadha to collaborator (Yash Ladha) (#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) (#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) (#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) (#34291)
* v8:
  * fix native  constructors (ExE Boss) (#36549)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants