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

[Mimi Zohar <zohar@linux.ibm.com>]

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[])