Avoid leaking secrets in to deallocated system memory about tweetnacl-js

[RainaBatwing]

I'll also mention in passing that the nacl.util encode/decode functions are all leaky, but because they work with strings there is no way to make them not leaky. A suitable warning in their section of the readme would be a good solution.

[dchest]

First of all, thank you for filing this issue.

I specifically didn't consider not leaving stuff in memory as a goal for a simple reason: we cannot be sure what JS VMs do with memory at all. For example, copying garbage collectors can copy our secrets before we overwrite them, or some compiler magic will keep a piece of memory somewhere else, making overwriting a lie.

Even Go, a "lower level" language, doesn't overwrite secrets in its standard crypto libraries, because the language doesn't guarantee that a piece of memory is the real piece of memory. For example, recently introduced stack copying in Go can make overwriting things sometimes pointless, because you'll overwrite a piece of copied memory, but the previous stack may have been left somewhere (to be fair, I didn't look into details, but that's my understanding).

Another point is that we can't access mlock from JavaScript to protect memory from being written to swap, so we'll happily overwrite RAM, while our secret is already somewhere on a disk.

The choice is: should we try to overwrite secrets and hope that it works, or not? I chose not to, but I don't have a strong opinion on this, so may be convinced to go with the first choice :-)

As for *.keyPair.fromSecretKey functions: the array is copied deliberately. It wasn't like that from the beginning, but after I spent a day debugging my application, which used tweetnacl-js, trying to find why signature verification failed, and realizing that keyPair.fromSecretKey sliced array, not copied it, I changed it. If I, the author of API, couldn't use it properly, what would you expect from normal users? :-)

[RainaBatwing]

One of the core ideas of Typed Arrays in modern javascript is that they work by referencing a single ArrayBuffer. The way DataViews and typed arrays are used together to transform data encourage this kind of thinking. I have a hard time imagining why a garbage collector would ever copy anything. The garbage collector shouldn't be doing anything to objects that are still in use, aside from mark and sweep. Because of the way DataView and Typed Arrays manipulate data together and the specified behaviour of subarray in creating new deliberately editable offset representations of the same information, it seems needlessly complicated and pointless for a javascript VM to ever copy that buffer data and then have to deal with complex cache invalidation logic. RAM reads should be just as fast if they are from the heap or from the stack. I had understood that one of the reasons Javascript crypto is getting to be a really great thing these days is precisely because you can reliably zero-out data in typed arrays. I think I may have seen that claim in one of @KAepora's talks? I'm not sure on that.

Buffer in node-like platforms is something we can depend on, being a native extension. It's memory management will remain true to it's own internal data representation regardless of future V8 changes.

Not sure what Go has to do with Javascript.

mlock is a good critique. *.keyPair.fromSecretKey isn't a major thing. A user can still zero-out those returned Uint8Arrays when they're done. I still think it would be worthwhile to have the random number generators not be leaky, and avoid any other instances of high level functions creating copies of secrets and then leaving them for the garbage collector to free without writing over them or returning them to the user. For Node.JS applications I feel this change is especially important, considering how easy it is to create bugs using Buffer that leak parts of previously unallocated heap memory to remote peers.

[dchest]

I have a hard time imagining why a garbage collector would ever copy anything. The garbage collector shouldn't be doing anything to objects that are still in use, aside from mark and sweep.

Compacting, generational garbage collectors can move pieces of memory around, for example, to avoid memory fragmentation.

I had understood that one of the reasons Javascript crypto is getting to be a really great thing these days is precisely because you can reliably zero-out data in typed arrays.

Unfortunately, nothing in the language standard guarantees this. So, we're currently left to trust implementation to not do anything we wouldn't like.

I've asked compiler/VM guys here: https://twitter.com/dchest/status/543429317934714880. I was glad to learn that currently in V8 we can rely on zeroing out things: "in V8 array buffers backing stores are allocated externally to the heap and never move".

I'd like to start with the change to PRNG you mentioned, but perhaps, instead of zeroing out inline, maybe let's have a cleanup() function to zero-out arrays and use it in PRNG? (I hope it won't get dropped by compiler as unused code). Will you send pull request or leave it for me to do?

Thanks!

[RainaBatwing]

I can do a pull request :)