| Message ID | 20210810202852.236354-1-bmeneg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [ima-evm-utils] evmctl: fix memory leak in get_password | expand |
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[])
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[]) > >
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
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.
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?
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.
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[])
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(-)