search

awnumar / memguard

Secure software enclave for storage of sensitive information in memory.
Apache License 2.0
2.5k stars 123 forks source link

Use finalizers to prevent leaks #27

Closed CAFxX closed 6 years ago

CAFxX commented 6 years ago

As suggested in https://www.reddit.com/r/golang/comments/6s84ky/memory_security_in_go/dlcmzp3/

Some cosmetic/ux changes may be needed. LockedBuffers() was removed to allow implementing automatic destruction. This shouldn't be much of a problem since I can't think of any good use cases outside of DeleteAll().

CAFxX commented 6 years ago

ugh test -race works on windows, but hangs on linux/mac, need to check tomorrow

CAFxX commented 6 years ago

Should work now

awnumar commented 6 years ago

Could you open a PR against the development branch instead, please. I should probably formalise and write down the process somewhere lol.

I'll review the changes soon, but from an initial look, I have a few comments.

CAFxX commented 6 years ago

Could you open a PR against the development branch instead, please. I should probably formalise and write down the process somewhere lol.

Done (although to be fair it seems that right now master is ahead of development...)

Further changes to the API are less fine, but acceptable if usability is not adversely affected.

The deletion of LockedBuffers() should be the only real API change. The only other thing that changed is that before the following was valid (but I'd argue rather useless) and now it's not valid anymore (it panics):

var b *LockedBuffer // nil
b.Destroy()

The implementation looks messy, but take this comment with a grain of salt, as I've hardly looked properly.

I tried to go for the minimum amount of changes. It can be made cleaner at the cost of adding explicit wrappers for most public functions.

awnumar commented 6 years ago

Oh, and don't forget to add yourself to the CONTRIBUTORS file.

CAFxX commented 6 years ago

but then add a layer of abstraction over them

not sure I understand what you mean here

awnumar commented 6 years ago

not sure I understand what you mean here

I'm not 100% sure myself yet. I guess we'll see.

awnumar commented 6 years ago

Thank you for contributing. 👍

CAFxX commented 6 years ago

I didn't finish renaming lockedBuffer -> container. Do you want a separate PR?

awnumar commented 6 years ago

You can if you wish, but I figured I could apply the "cosmetic" changes myself.