On Wed, Aug 11, 2021 at 10:52:00AM -0400, Mimi Zohar wrote:
> Hi Bruno,
>
> On Tue, 2021-08-10 at 17:28 -0300, Bruno Meneguele wrote:
> > The variable "password" is not freed nor returned in case get_password()
> > succeeds. Instead of using an intermediary variable ("pwd") for returning
> > the value, use the same "password" var. Issue found by Coverity scan tool.
> >
> > src/evmctl.c:2565: leaked_storage: Variable "password" going out of scope
> > leaks the storage it points to.
> >
> > Signed-off-by: Bruno Meneguele <bmeneg@xxxxxxxxxx>
> > ---
> > src/evmctl.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/evmctl.c b/src/evmctl.c
> > index 7a6f2021aa92..b49c7910a4a7 100644
> > --- a/src/evmctl.c
> > +++ b/src/evmctl.c
> > @@ -2601,8 +2601,9 @@ static struct option opts[] = {
> > static char *get_password(void)
> > {
> > struct termios flags, tmp_flags;
> > - char *password, *pwd;
> > + char *password;
> > int passlen = 64;
> > + bool err = false;
> >
> > password = malloc(passlen);
> > if (!password) {
> > @@ -2622,16 +2623,24 @@ static char *get_password(void)
> > }
> >
> > printf("PEM password: ");
> > - pwd = fgets(password, passlen, stdin);
> > + if (fgets(password, passlen, stdin) == NULL) {
> > + perror("fgets");
> > + /* we still need to restore the terminal */
> > + err = true;
> > + }
>
> From the fgets manpage:
> fgets() returns s on success, and NULL on error
> or when end of file
> occurs while no characters have been read.
>
Yes, I was considering "end of file while no characters have been read"
as an invalid password. The error message is misleading though, which
can be fixed.
> > /* restore terminal */
> > if (tcsetattr(fileno(stdin), TCSANOW, &flags) != 0) {
> > perror("tcsetattr");
> > + err = true;
> > + }
> > +
> > + if (err) {
> > free(password);
> > return NULL;
> > }
> >
> > - return pwd;
> > + return password;
>
> Wouldn't a simpler fix be to test "pwd" here?
> if (!pwd)
> free(password);
> return pwd;
>
The problem is on success, when 'pwd' is actually not NULL.
With that, I can't free(password). I would need to asprintf(pwd, ...) or
strndup(password). Because of that, I thought it would be cleaner to
remove 'password' completely.
Your call ... :)
> thanks,
>
> Mimi
>
> > }
> >
> > int main(int argc, char *argv[])
>
>
--
bmeneg
PGP Key: http://bmeneg.com/pubkey.txt
Attachment:
signature.asc
Description: PGP signature
