[RFC] Zero out private keys on Drop

[d-e-s-o]

Should we zero out sensitive key material? Various (meta) sources seem to indicate that this is still a thing.

I drafted something up, but I am not sure if it's a great solution: private keys have public fields and implementing drop for the key itself prevents a simple move of attributes out of it, which makes handling awkward.

Do you have an opinion on the matter?

[fmpomar]

You know, I didn't take this into account because we don't actually need it in SeKey (we only deal with public keys, private keys are stored in the secure enclave), but, now that you brought this to my attention, I think it could be a big deal if this library is to be used more broadly.

Your solution, at first sight, looks OK, but I'm not really convinced that the call to write_volatile with a zeroed() value of the struct will actually erase the whole vector from memory.

My gut says is that It'll just set the vector length, and maybe the first few bytes of the key to zero. Or maybe even a pointer, which could result in a memory leak, but I'm just guessing here, I don't know how Vec is implemented. In any case, I think there's the possibility that the key will still stay in memory, since the zeroed() value is probably smaller than the value containing the key, in memory.

We also should:

• Look into making sure that it is stored in a page that doesn't get swapped into disk, since this happening would render our efforts useless.

• Implement a custom Debug for PrivateKey (I just realized that the current one just prints everything naively, and could be dangerous).

Do you know of any crates that implement any of these security features in a reasonably solid/portable way? Maybe we could just use them (if they don't add a lot of dependencies) or just use them for inspiration

[d-e-s-o]

Hm, that's a good point. Now that you mention it my gut seems to tell me the same thing :-) I wanted to add a test but then realized that I am probably way out of Rust's guarantees just poking in a dropped object's remains. But that can certainly be tried out.

I don't know of any crates that would hold our hands here, but they may very well be out there. We should probably check the popular crypto crates and check what they do/use.

[fmpomar]

About the crates, this is what I found (from a quick search):

• https://github.com/quininer/seckey
• https://github.com/quininer/memsec (Used by the above)
• https://github.com/myfreeweb/secstr (Uses libsodium)

I also went full paranoid and hypothesized that the key could also remain in tokio's send/receive buffers. For instance, I'm quite sure this line won't do anything more than advance a pointer, and maybe we should overwrite the BytesMut with zeros for the message length prior to it

ssh-agent.rs/src/agent.rs

Line 41 in 8732fcc

src.advance(size_of::<u32>() + length);

But I'm not sure we can do the same for the Encoder part (at least without rewriting everything), so I guess there's a limit to what we can do to guarantee that the key doesn't stay recorded somewhere.

[d-e-s-o]

Interesting. Thanks for digging up those crates! They seem to be exactly what we need. Do you have plans to hook them up?

But I'm not sure we can do the same for the Encoder part (at least without rewriting everything), so I guess there's a limit to what we can do to guarantee that the key doesn't stay recorded somewhere.

Hm. I guess any leak is as bad as the other, but I'd say one step at a time.

But I am not sure I am following here. On which paths are we decoding secret key material? I can't see that being embedded in the Message, and isn't that the main user of this path? I actually use a different crate for loading of keys (which has the same problem...) and then just create a PrivateKey from the in-memory data. I did not find a way to load keys directly with this crate.

[fmpomar]

Do you have plans to hook them up?

Nico and I will take a closer look on them as soon as we can. Also, I'd like to make it as transparent as possible (It's not good if the user has to handle objects from these crates directly, since we might want to change them in the future)

On which paths are we decoding secret key material? I can't see that being embedded in the Message

One of the messages is AddIdentity which has a PrivateKey embedded in it, so the key will end up stored in the Decode buffer, If you're using the library to implement an agent.
On the other hand, if you use it to implement a client, when you transmit the private key, it'll end up in the Encode buffer.

ssh-agent.rs/src/proto/message.rs

Line 81 in 8732fcc

AddIdentity(AddIdentity),

ssh-agent.rs/src/proto/message.rs

Line 20 in 8732fcc

pub privkey: PrivateKey,

(But I don't know if this is exactly what you meant)

Hm. I guess any leak is as bad as the other, but I'd say one step at a time.

I agree, I think the way to go for now is to solve what we can and then document it the best we can for users to know what are the risks involved.

I mean, in the end the keys are usually stored on disk, accessible to any process running under the owner's uid, and AFAIK modern OSs zero-out the memory when they allocate it to new process, so the risks would be limited to the same process (the SSH agent) which, if compromised, there's not much we can do about. Nevertheless, it being written in Rust and all, should be fairly resilient to exploits.