search

iqlusioninc / crates

A collection of open source Rust crates from iqlusion
https://www.iqlusion.io
Apache License 2.0
463 stars 73 forks source link

Reading secret from a file #557

Open cecton opened 4 years ago

cecton commented 4 years ago

I was wondering if this code could potentially leave a copy of a secret in memory:

let password = SecretString::new(fs::read_to_string(file_path)?);

Because the underlying implementation of read_to_string might re-allocate the String and not zeroes the partially-read secret.

Do you have any suggestion?

I would be happy to provide a PR that convert an impl Read to a SecretString that would zeroes things between reads if you think that is a good implementation.

tony-iqlusion commented 4 years ago

@cecton being able to read from a file directly into a SecretString would definitely be interesting!

c-git commented 2 years ago

@cecton did you ever implement the reading in a secret from a file?

cecton commented 2 years ago

@c-git sorry I totally forgot about this ticket, Feel free to go ahead with the implementation if you would likd

c-git commented 2 years ago

Ok thanks. If I do I'll submit it but I'm not quite sure how to securely accomplish that task.

cecton commented 2 years ago

tbh I'm not sure either, My colleagues back in the days mentioned something special to do but I forgot what exactly... @tomaka maybe you can help with some advice here? (I think it was you)

cecton commented 2 years ago

I would be happy to provide a PR that convert an impl Read to a SecretString that would zeroes things between reads if you think that is a good implementation.

oh wait, probably that was a not so innocent suggestion, I think the idea was to copy the code of fs::read_to_string from std and do something special like zeroing the buffer before the exit.

c-git commented 2 years ago

Thanks that looks like something I might be able to follow up on.

c-git commented 2 years ago

Disclaimer I'm new to Rust (but not programing). So if someone can look over what I did that would be great. (I used PyCharm to follow the code from one point to the next in the standard library and it looks like it was navigating correctly to me. Was able to match it in the online github repo)

Didn't seem possible for a full copy to be left in a buffer somewhere in memory

That said I think no separate buffers are used other than the one created to store the string in the first call. And this owned string is moved into the Secret wrapper. There was one point in the code here where it looked like another buffer was allocated.

code screen shot

However when I checked what a ReadBuf was it turns out it's just a pointer to part of an already existing buffer (the one created earlier). That is based on the comments that I found here.

Screen shot of supporting comment

Partial copies seemed possible (but under what seemed like unlikely situations)

I did see two ways that part of the file could be loaded into memory and then not cleared. I'm not sure of the viability of either approach but from reading the comments and following the code logic I was lead to believe that it might be possible.

  1. If when allocating the reserved space in the string here if the size of the file was not able to be determined so that the required space was not allocated then it is possible that the Vec will need to grow during the reading process and the old layouts could contain partial amounts of the characters from the file.
  2. If an error occurs during the file read then the string loaded into memory thus far might not be zeroed out when the function returns.