Secure memory: Yay or nay

[tniessen]

OpenSSL has support for a concept that is referred to as secure memory or secure heap. Essentially, every time OpenSSL allocates memory, it indicates whether that memory should be allocated from the "normal" heap using malloc or from the "secure" heap. Allocations on the secure heap usually have the following security properties:

• Allocated memory is never swapped to the disk and never included in core dumps, making it less likely to leak sensitive information through those.
• Allocated memory is always overwritten on deallocation, also making it less likely to leak information.
• It is far more difficult to use buffer overflows to retrieve data from these allocations. (OpenSSL causes the process to terminate if memory surrounding secure allocations is accessed.)

Node.js does not use this feature, meaning that OpenSSL performs normal allocations. (OpenSSL still overwrites the memory on deallocation.) We can enable OpenSSL's implementation, but it is quite restrictive and will be difficult to configure for users of Node.js. The alternative is to provide a more suitable implementation within Node.js, which provides similar security properties while being easier to use and less restrictive. However, that requires:

• Upstream changes to OpenSSL's memory management, in order to allow overriding the built-in secure memory implementation. I have talked to some of the maintainers, and they would likely accept such changes.
• The actual secure heap implementation in Node.js. That is not super difficult, but the implementation is platform-specific and will not be easy to test.

I started working on this approach about a year ago, and never finished it. I got varying feedback at the Montreal summit, so let's discuss this in public before I either stop working on it or put in a lot more work. As someone pointed out, many cloud applications likely don't care about this level of security.

Even if we upstream the necessary changes in OpenSSL, it is unlikely to ever work with a shared OpenSSL library, so dynamic linking would disable the feature. Also, I don't want to break Electron (as I have done numerous times, sorry @codebytere!), so we would need to make sure to either propose the change to both OpenSSL and BoringSSL or to make it easy to opt out of the feature at compile time.

I also have a patch that solves #18896, but relies on this particular feature, and is super dangerous to use.

[mscdex]

What are the performance implications?

[tniessen]

This would only affect data that Node.js/OpenSSL deems sensitive (e.g., TLS private keys). For that data, allocation and deallocation are potentially slower, but accessing the allocated memory is at least as fast as accessing any other memory region.

Admittedly, that is only half the truth. If it does not negatively affect the performance, why would we not use this feature for all allocations? The problem is that secure memory cannot be swapped to the disk, and swapping is a crucial performance factor. While that makes accesses to these pages even faster than normal memory accesses, it can negatively affect the average system performance if a large number of pages is locked into memory, which is why the OS kernel usually restricts the amount of secure memory per process.

[devsnek]

it seems reasonable to me to explore using it in places like crypto and tls, and maybe also exposing something like Buffer.allocSecure. The downside there would be using too much allocSecure not being GC'd fast enough, so we could probably hold off on that.

[bnoordhuis]

1. Memory locked with mlock() is subject to the RLIMIT_MEMLOCK resource limit (ulimit -l), which at least on my system is restricted to 32 MB per process. Too much locked memory leads to out-of-memory errors, making it unsuitable for general purpose memory management.

2. mlock() works on page boundaries. Allocating 1 byte requires you to mmap 4k (or 64k on ppc) of memory. OpenSSL implements its own memory manager on top of mmap() to alleviate that but for storing sensitive data it's arguably better to use mmap/munmap directly (no chance of use-after-free or metadata corruption bugs.)

3. We likely want to set the MADV_DONTFORK flag when available, something openssl doesn't - but maybe it can be taught to do that, or maybe we can call madvise() on the return value from CRYPTO_secure_malloc().

[tniessen]

@bnoordhuis Thank you for sharing your thoughts!

1. That is a factor that we need to consider, and I have been thinking about ways to get around that. This is such a specific feature that users might want to configure that limit if they want to enable secure management. Another option might be to fall back to "normal" allocations once the limit is exceeded.

2. I am currently using a simple buddy allocator, similar to what OpenSSL has, except that my implementation supports dynamic heap sizes. mmaping individual allocations might provide some security properties that a buddy allocator doesn't, but it is likely slower, likely uses the limited amount of memory less efficiently, and makes the concept of guard pages almost impossible. (On the other hand, we don't have to use guard pages.)

3. My current plan is to allow replacing all the secure memory management functions in OpenSSL, similar to CRYPTO_set_mem_functions. That would make that possible :)

[bnoordhuis]

Another option might be to fall back to "normal" allocations once the limit is exceeded.

That seems like a bad idea. Users can always do something like this when node throws an exception:

let buf;
try {
  buf = Buffer.secureAlloc(32);
} catch (e) {
  buf = Buffer.alloc(32);
}

Or this when node returns null:

let buf = Buffer.secureAlloc(32) ?? Buffer.alloc(32);

That way it's explicit that you're okay with insecure storage.

("secure' and "insecure" are arguably misnomers. We need to come up with something better. :-))

[tniessen]

@bnoordhuis I agree, it seems to be a bad idea when done from within JavaScript. I was referring more to data that OpenSSL stores in secure memory, allocations that we cannot influence, and that might not be obvious to the user.

[tniessen]

("secure' and "insecure" are arguably misnomers. We need to come up with something better. :-))

My current patch calls it crypto.alloc :P

[sam-github]

So, its not clear to me how important a security issue this is, in realm of defence in depth, how deep does an attacker have to be before secure memory is an obstacle? I've no objection to it, though, it doesn't seem like it can hurt (well, if a js binding is exposed users could shoot themselves in the foot with it).

@nodejs/security-wg or anyone - is secure memory a common feature of comparable runtimes? chrome, electron, perl, python, ruby, go, rust, etc? That would give a sense of how many others thought it important in the past, but there isn't any reason js can't be more secure, even if we're the only one doing this.

[deian]

I think this is an important feature, but won't the GC and JIT mess with the intentions of trying to do this for JS?

[bnoordhuis]

@deian Anything in particular you're thinking of?

[addaleax]

@sam-github I certainly like the prospect of automatically excluding things like crypto keys from core dumps… having those inadvertently uploaded somewhere public seems like a plausible scenario to me.