search

dchest / tweetnacl-js

Port of TweetNaCl cryptographic library to JavaScript
https://tweetnacl.js.org
The Unlicense
1.75k stars 292 forks source link

Generating keypair leaks memory #222

Closed varjolintu closed 2 years ago

varjolintu commented 2 years ago

It's possible that nacl.box.keyPair() leaks memory every time it's called. In KeePassXC-Browser that functions is called repeatedly in a reconnect situation.

Details in the related issue: https://github.com/keepassxreboot/keepassxc-browser/issues/1113

CMEONE commented 2 years ago

Hello @varjolintu I'll look into this issue, but do you have any more details such as what specific circumstances cause this leak (like how many times are you generating keys, in what time period, etc.)?

Also, are you using the latest version of TweetNaCl.js? If not, try updating to the latest version and see if that resolves the issue.

varjolintu commented 2 years ago

@CMEONE For now KeePassXC-Browser with Auto-Reconnect enabled generates a new keypair in one second intervals. Latest version didn't solve the problem. If testing this with KeePassXC-Browser is too much to handle, I can also make a simple browser extension where this bug can be easily reproduced.

CMEONE commented 2 years ago

@varjolintu So, I've run some tests and haven't found any significant increases in memory usage as shown by the constantly updating memory tab in devtools. The memory usage seems to just go up 1MB and then back down 1MB, oscillating between those two values with no real signs of leaks. I am running the key generation 10 times a second instead of once per second to try to make any results of the tests more noticeable. 

setInterval(() => {
    let keypair = nacl.box.keyPair();
}, 100);

This is the code that I am using to test. As you can see, I keep the keypair scoped within the function call in the interval so that it becomes garbage collected eventually (the reason for the oscillation in memory usage) and doesn't consume a significant amount of memory, this way we can isolate any memory issues specifically to nacl.box.keyPair() (there doesn't seem to be any issue that I can see currently).

Could you please show a small sample of code that reproduces your issue so that we can further investigate?

varjolintu commented 2 years ago

@CMEONE You're correct. I made an example extension and it doesn't have the memory leak. So this is definitely a KeePassXC-Browser issue. Something is done in a way that the GC doesn't trigger. Sorry for the inconvenience. Closing the issue.

CMEONE commented 2 years ago

No problem, glad I was able to help isolate this issue to KeePassXC-Browser.

dchest commented 2 years ago

@CMEONE thank you for helping with this!

@varjolintu I noticed keys are stored in a "global" (namespaced) variable in KeePassXC-Browser extension, maybe this has something to do with the leak.

CMEONE commented 2 years ago

@dchest No problem! I did indeed see this "global" variable in KeePassXC-Browser and thought it was being overwritten with the new keys, but this could potentially be part of the issue.

CMEONE commented 2 years ago

@varjolintu Try using the memory tab in devtools to see what isn't being garbage collected (if you take a snapshot and find a ton of keys in the heap under the Uint8Array class, it's probably an issue with storing keys, if you don't see a lot of keys but memory usage is abnormally high, then this might be an issue with another part of KeePassXC-Browser).

varjolintu commented 2 years ago

I tried my example with a "global" variable, but that didn't reproduce the issue. Thanks for the tips.

EDIT: Another user did a performance analysis in the original issue and it shows the following: https://user-images.githubusercontent.com/23653/100321818-f6c67580-2fcb-11eb-9449-dfcdae4c3f10.png