[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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

On May 26, 2020, at 23:46, Vasyl Gello <vasek.gello@gmail.com> 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

Reply to: