[PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[Bruno Meneguele]

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@redhat.com>
---
 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;
+	}
 
 	/* restore terminal */
 	if (tcsetattr(fileno(stdin), TCSANOW, &flags) != 0) {
 		perror("tcsetattr");
+		err = true;
+	}
+
+	if (err) {
 		free(password);
 		return NULL;
 	}
 
-	return pwd;
+	return password;
 }
 
 int main(int argc, char *argv[])
-- 
2.31.1

Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[Mimi Zohar]

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@redhat.com>
> ---
>  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. 

>  	/* 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;

thanks,

Mimi

>  }
>  
>  int main(int argc, char *argv[])

Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 2592 bytes --]

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@redhat.com>
> > ---
> >  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[Mimi Zohar]

On Wed, 2021-08-11 at 13:51 -0300, Bruno Meneguele wrote:
> On Wed, Aug 11, 2021 at 10:52:00AM -0400, Mimi Zohar wrote:
> 
> > > 
> > > -	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.

I see.  So instead of "return pwd" as suggested above,

        if (!pwd) {
                free(password);
                password = NULL;  <== set or return NULL
        }

        return password;

thanks,

Mimi

Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Wed, Aug 11, 2021 at 01:31:49PM -0400, Mimi Zohar wrote:
> On Wed, 2021-08-11 at 13:51 -0300, Bruno Meneguele wrote:
> > On Wed, Aug 11, 2021 at 10:52:00AM -0400, Mimi Zohar wrote:
> > 
> > > > 
> > > > -	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.
> 
> I see.  So instead of "return pwd" as suggested above,
> 
>         if (!pwd) {
>                 free(password);
>                 password = NULL;  <== set or return NULL
>         }
> 
>         return password;
> 

Ack. Will send a v2 with this change.

Thanks Mimi.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[Ken Goldman]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On 8/11/2021 1:31 PM, Mimi Zohar wrote:
> On Wed, 2021-08-11 at 13:51 -0300, Bruno Meneguele wrote:
>> On Wed, Aug 11, 2021 at 10:52:00AM -0400, Mimi Zohar wrote:
>>
>>>>
>>>> -	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.
> 
> I see.  So instead of "return pwd" as suggested above,
> 
>          if (!pwd) {
>                  free(password);
>                  password = NULL;  <== set or return NULL
>          }
> 
>          return password;

That looks cleaner to me.

My style would be

	if (pwd == NULL)

which compiles to the same binary, but it less prone to error.

In addition, since this is reading from stdin

1 - Do you want the newline to be part of the password?
2 = Why is an empty password an error?


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4490 bytes --]

Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password

[Bruno Meneguele]

On Wed, Aug 11, 2021 at 02:28:37PM -0400, Ken Goldman wrote:
> On 8/11/2021 1:31 PM, Mimi Zohar wrote:
> > On Wed, 2021-08-11 at 13:51 -0300, Bruno Meneguele wrote:
> > > On Wed, Aug 11, 2021 at 10:52:00AM -0400, Mimi Zohar wrote:
> > > 
> > > > > 
> > > > > -	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.
> > 
> > I see.  So instead of "return pwd" as suggested above,
> > 
> >          if (!pwd) {
> >                  free(password);
> >                  password = NULL;  <== set or return NULL
> >          }
> > 
> >          return password;
> 
> That looks cleaner to me.
> 
> My style would be
> 
> 	if (pwd == NULL)
> 
> which compiles to the same binary, but it less prone to error.
> 
> In addition, since this is reading from stdin
> 
> 1 - Do you want the newline to be part of the password?

I would say 'yes'. AFAIK OpenSSL preserves the newline if it's present
in the input from <stdin>:

"The returned string is always NUL-terminated and the '\n' is preserved
if present in the input data" (BIO_gets() manpage from OpenSSL)

Also, if the user passed the password to the PEM file creation through
the arguments list (no newline) it can also do the same with evmctl.

> 2 = Why is an empty password an error?
> 

Considering the item 1, I don't think we have an empty string in this
case.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt