fix: ensure cached passphrase buffer is freed - #135
Conversation
Previously, ssh-tpm-agent would leak the buffer containing the passphrase as the buffer wrapper was dropped. This is hard to fix as the underlying tpm keyfile library expects a normal go byte slice as the passphrase, not an externally managed memory segment. This changes the signing module to wrap the passphrase fetching code so the buffer can also be returned, allowing it to be freed when present. It also leaves the option for the askpass machinery to provide similar buffers in the future.
|
|
||
| func NewSSHKeySigner(k SSHTPMKeys, keyring *keyring.ThreadKeyring, ownerAuth func() ([]byte, error), tpm func() transport.TPMCloser, auth func(*keyfile.TPMKey) ([]byte, error)) *SSHKeySigner { | ||
| // signKey wraps t.auth to ensure its buffer is wiped and freed before signKey returns. | ||
| func (t *SSHKeySigner) signKey(k *SSHTPMKey, r io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) { |
There was a problem hiding this comment.
Generally I think this is fine, however I was considering removing the memcall stuff to the new runtime/secret package as I was wondering if maybe it would be less invasive.
https://pkg.go.dev/runtime/secret#Do
So before accepting this I think it's a good idea to just rip the memory box stuff out and replace with secret.Do and see if we can achieve the same stuff.
There was a problem hiding this comment.
I think the one problem with secret.Do is that the GC isn't guaranteed to happen once we are done with the buffer. I can try putting it together, but I think the signKey wrapper is still needed to clear the returned buffer before returning, but secret.Do would ensure no other leaks occur during processing. I'll try putting together something quick.
It could also simplify the askpass refactoring. Otherwise the code would need to manually copy from the askpass stdout/TTY, which is painful.
There was a problem hiding this comment.
Oh, I just remembered the other concern with dropping memcall: We cannot easily mlock the entire process's memory, so using only Go byte slices reduces the security of the passphrase. I thinking wrapping it in secret.Do is still good (protecting stack/registers), but I don't think we can actually drop memcall.
We could mlockall the entire memory, but users would need to configure higher memory locking limits (usually it's only a couple pages, which memcall can work inside of). Or the process would need to have a capability added/be setuid, but that adds a lot of complication.
I'll still finish the PR dropping memcall so we can see the difference? But my vote would be to keep it.
2 participants
Previously, ssh-tpm-agent would leak the buffer containing the passphrase as the buffer wrapper was dropped. This is hard to fix as the underlying tpm keyfile library expects a normal go byte slice as the passphrase, not an externally managed memory segment.
This changes the signing module to wrap the passphrase fetching code so the buffer can also be returned, allowing it to be freed when present. It also leaves the option for the askpass machinery to provide similar buffers in the future.