* [PATCH ima-evm-utils] evmctl: fix memory leak in get_password
@ 2021-08-10 20:28 Bruno Meneguele
2021-08-11 14:52 ` Mimi Zohar
0 siblings, 1 reply; 7+ messages in thread
From: Bruno Meneguele @ 2021-08-10 20:28 UTC (permalink / raw)
To: vt, zohar; +Cc: linux-integrity, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password
2021-08-10 20:28 [PATCH ima-evm-utils] evmctl: fix memory leak in get_password Bruno Meneguele
@ 2021-08-11 14:52 ` Mimi Zohar
2021-08-11 16:51 ` Bruno Meneguele
0 siblings, 1 reply; 7+ messages in thread
From: Mimi Zohar @ 2021-08-11 14:52 UTC (permalink / raw)
To: Bruno Meneguele, vt; +Cc: linux-integrity
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[])
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password
2021-08-11 14:52 ` Mimi Zohar
@ 2021-08-11 16:51 ` Bruno Meneguele
2021-08-11 17:31 ` Mimi Zohar
0 siblings, 1 reply; 7+ messages in thread
From: Bruno Meneguele @ 2021-08-11 16:51 UTC (permalink / raw)
To: Mimi Zohar; +Cc: vt, linux-integrity
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password
2021-08-11 16:51 ` Bruno Meneguele
@ 2021-08-11 17:31 ` Mimi Zohar
2021-08-11 17:52 ` Bruno Meneguele
2021-08-11 18:28 ` Ken Goldman
0 siblings, 2 replies; 7+ messages in thread
From: Mimi Zohar @ 2021-08-11 17:31 UTC (permalink / raw)
To: Bruno Meneguele; +Cc: vt, linux-integrity
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password
2021-08-11 17:31 ` Mimi Zohar
@ 2021-08-11 17:52 ` Bruno Meneguele
2021-08-11 18:28 ` Ken Goldman
1 sibling, 0 replies; 7+ messages in thread
From: Bruno Meneguele @ 2021-08-11 17:52 UTC (permalink / raw)
To: Mimi Zohar; +Cc: vt, linux-integrity
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password
2021-08-11 17:31 ` Mimi Zohar
2021-08-11 17:52 ` Bruno Meneguele
@ 2021-08-11 18:28 ` Ken Goldman
2021-08-16 15:10 ` Bruno Meneguele
1 sibling, 1 reply; 7+ messages in thread
From: Ken Goldman @ 2021-08-11 18:28 UTC (permalink / raw)
To: Mimi Zohar, Bruno Meneguele; +Cc: vt, linux-integrity
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH ima-evm-utils] evmctl: fix memory leak in get_password
2021-08-11 18:28 ` Ken Goldman
@ 2021-08-16 15:10 ` Bruno Meneguele
0 siblings, 0 replies; 7+ messages in thread
From: Bruno Meneguele @ 2021-08-16 15:10 UTC (permalink / raw)
To: Ken Goldman; +Cc: Mimi Zohar, vt, linux-integrity
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-16 15:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 20:28 [PATCH ima-evm-utils] evmctl: fix memory leak in get_password Bruno Meneguele
2021-08-11 14:52 ` Mimi Zohar
2021-08-11 16:51 ` Bruno Meneguele
2021-08-11 17:31 ` Mimi Zohar
2021-08-11 17:52 ` Bruno Meneguele
2021-08-11 18:28 ` Ken Goldman
2021-08-16 15:10 ` Bruno Meneguele
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.