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.alloc() for encryption key memory management #18896

Closed
jorangreef opened this issue Feb 21, 2018 · 7 comments
Closed

crypto.alloc() for encryption key memory management #18896

jorangreef opened this issue Feb 21, 2018 · 7 comments
Assignees
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. memory Issues and PRs related to the memory management or memory footprint. stale

Comments

@jorangreef
Copy link
Contributor

@indutny and everyone working on the crypto module:

At present, using a Buffer for a crypto key exposes the key to core dumps, swapping, and forking. The user also has to remember to erase the key once done. Granted, core dumps and swapping can be disabled system wide, but some libraries such as libsodium take care of this automatically, without the user having to know the fine details.

Would you be open to a crypto.alloc() method to allocate buffers for use as crypto keys and help with crypto memory management, this would:

  1. Set the platform equivalents of MADV_DONTFORK , MADV_DONTDUMP, and mlock as far as possible.

  2. Automatically zero buffers at GC time before freeing, along the lines of https://github.com/jedisct1/libsodium/blob/be58b2e6664389d9c7993b55291402934b43b3ca/src/libsodium/sodium/utils.c#L78:L101

  3. Hopefully do guarded heap allocations, but 1 and 2 would be enough for a start.

@addaleax addaleax added crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. memory Issues and PRs related to the memory management or memory footprint. labels Feb 21, 2018
@bnoordhuis
Copy link
Member

I know it's been discussed before but I can't find the issue so I'll just summarize. :-)

It's not that useful while we're still using openssl 1.0.2 because even if node.js takes precautions, openssl won't - it doesn't mlock/madvise its copy of the key. That functionality only exists in 1.1.0 and above and, caveat emptor, openssl doesn't set MADV_DONTFORK because it has to support the pre-fork model.

It might be possible to make it work with 1.0.2 by means of CRYPTO_set_mem_ex_functions but it's unclear if that covers everything. There are a number of places where openssl calls plain malloc/free instead of CRYPTO_malloc/CRYPTO_free (but you would hope only where it doesn't matter) and there might be places where it keeps sensitive data in non-heap storage, like on the stack.

@indutny
Copy link
Member

indutny commented Feb 21, 2018

I'm afraid that it won't work as well as we could have hoped for (as @bnoordhuis said). Even if we'd make guarantees for such buffer very strong, there's still on-stack access and V8 heap values that could ruin them.

@bnoordhuis
Copy link
Member

That functionality only exists in 1.1.0

We're at 1.1.0 now so that's one blocker less.

@tniessen I suspect you may have ideas on how to do key management?

@tniessen
Copy link
Member

@bnoordhuis I have been experimenting with a secure heap implementation for node and OpenSSL, but I need to patch OpenSSL to make it work. Theoretically, we could consider a secure heap (in the OpenSSL sense) separately from crypto.alloc, but I guess it makes sense to do both the same way.

@jasnell
Copy link
Member

jasnell commented Jan 9, 2021

Just updating on this... once #36779 lands we'll have basic support for openssl secure heap. When enabled, all openssl BIGNUM's are allocated off the secure heap. There are caveats, such as the secure heap having a fixed size right now. Hopefully @tniessen's approach for dynamic allocation is something we can pursue in the future, but at the very least #36779 gives us a place to start.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 22, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. memory Issues and PRs related to the memory management or memory footprint. stale
Projects
None yet
Development

No branches or pull requests

6 participants