search

conradkleinespel / rpassword

Cross platform Rust library to read a password in the terminal (Linux, BSD, OSX, Windows, WASM).
Apache License 2.0
243 stars 38 forks source link

Zero password using zeroize crate. #30

Closed dvush closed 5 years ago

dvush commented 5 years ago

Hello. I just noticed that for password zeroing you are using simple loop, which AFAIK can be discarded by optimizer.

Zeroize crate implements zeroing properly and it is small crate without dependencies.

dvush commented 5 years ago

ping @conradkdotcom

conradkleinespel commented 5 years ago

@dvush Hi ! Thanks for your PR and sorry for the delay.

Would you mind explaining in which case the loop I'm using would be discarded ? I'd like to understand this, because without understanding, I'm quite hesitant to add a dependency on a crate I do not know and will end in rpassword, which is then used by password managers and other quite critical applications.

dvush commented 5 years ago

@conradkdotcom

Thanks for responding!

You can check out example here https://rust.godbolt.org/z/8DgO4R and if you comment out zero_memory call inside test_zero it does not change because it is optimized out by compiler.

EDIT: Also you can check out documentation or code for zeroize crate. The code inside is quite simple https://github.com/iqlusioninc/crates/blob/master/zeroize/src/lib.rs

conradkleinespel commented 5 years ago

@dvush Thanks for the example ! And sorry for the delay.

I've taken a close look at the zeroize crate and it seems like fixing rpassword is quite simple, without the need for the dependency on the zeroize crate. Here's a proposed fix: https://github.com/conradkdotcom/rpassword/pull/34

Here's an example output with the code from my PR: https://rust.godbolt.org/z/DEOC2x. It looks like the ASM output does contain the zeroing code with this PR, so everything should be fine.

What do you think ?

/cc @colelawrence You've added a reaction to a comment, so I'm pinging you as well thinking you might be interested in this

conradkleinespel commented 5 years ago

@dvush I'll go ahead and merge the previously mentioned PR. Feel free to get back to me if you think I made a mistake. Thanks again for reporting this issue ! :+1:

dvush commented 5 years ago

Great, thanks! It looks like I missed email update somewhere in my inbox. I agree with you that it is much more clean to not have dependency.