> On May 26, 2020, at 23:46, Vasyl Gello <[email protected]> wrote:
>
> Hi Matthew!
>
>> I would suggest adding one as well as fuzzing this code before exposing the
>> downstream public to it.
>
> Will fix the issues and add testsuite && fuzzcorp ASAP.
>
> BTW I fixed all the stuff GCC 8.3.0 reported me with FORTIFY_SOURCE=2 before
> pushing code to GitHub.
> Did you use GCC 10?
I just used manual inspection to identify this.
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.
[1]:
https://github.com/basilgello/cryptopass/blob/master/src/cryptopass.c#L375-L384
<https://github.com/basilgello/cryptopass/blob/master/src/cryptopass.c#L375-L384>
