Bug#961429: RFS: cryptopass/1.0.0-1 [ITP] -- CLI utility for generating long, unguessable passwords.

[Vasyl Gello <vasek.gello@gmail.com>]

Hi Matthew!

Thanks for the continued review! You read my mind now?)

>
>Now that I read the remainder of the main source file, I spotted a completely separate issue, src/cryptopass.c:375-384 [1]:
>
>    /* Clean up everything */
>
>    for (counter = 0; counter < 10; counter++) {
>      memset(derivedpassword, 0, PASSWORD_BUFFER_SIZE);
>      memset(domain, 0, MAX_INPUT_SIZE);
>      memset(masterpassword, 0, MAX_INPUT_SIZE);
>      memset(passlenbuf, 0, PASSWORD_LENGTH_BUFFER_SIZE);
>      memset(salt, 0, SALT_BUFFER_SIZE);
>      memset(username, 0, MAX_INPUT_SIZE);
>    }
>
>This does not do what you think it does. Either these duplicated memsets are redundant or the compiler will optimize them all away observing the targets are unused after this. The way to erase something in a way the compiler cannot elide is a single memset_s(). However, the program is about to exit and be torn down by the operating system, so even this would be redundant.
>
>Your intent (I am guessing) is to prevent an attacker reading these values out of program memory. However, an attacker with this ability can simply ptrace cryptopass or attach to it with a debugger. I think some thought should be put into the threat model for this program and it should probably have a more thorough security review before it is packaged.

The threat model is obviously not against an attacker with live root or hypervisor access. Rather not to leave unwanted things for possible cold-boot attacks.

Thanks for mentioning memset_s. I see it is C11-specific so if I target older standard on source level, I will have to do cleanup manually.
-- 
Vasyl Gello