> On May 27, 2020, at 08:15, Vasyl Gello <vasek.ge...@gmail.com> wrote:
>
> 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.
The capabilities of a post-reboot userspace attacker are not affected by these
measures.
> 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
