1161686 - libmar's |SECU_GetModulePassword| can leak allocated string (command line build utility)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Eric Rahm [:erahm]]

Coverity reports that libmar's |SECU_GetModulePassword| leaks the return value of |GetPasswordString| [1] in the PW_EXTERNAL case.

[1] https://hg.mozilla.org/mozilla-central/annotate/754579ec0e68/modules/libmar/sign/nss_secutil.c#l222

Comment 1

[Jose Rios]

Hi Eric and Robert, can I take this bug?

Comment 2

[Robert Strong (they/them - no direct email)]

Hi José, yes you can and thanks!

Comment 3

[Jose Rios]

Attachment: bug1161686_fix_leak_v1.patch

Robert, here's a patch for the solution. Since the returned value isn't used anyway, I just added a free directly in the line. Do you think it's better to create a new variable to receive the value?

Comment 4

[Robert Strong (they/them - no direct email)]

Hi Eric, have you also filed a bug against nss for this same issue?
http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/lib/secutil.c#246

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 8602489 [details] [diff] [review]
bug1161686_fix_leak_v1.patch

Hi Jose', I'd like this code to stay the same as the original nss code so I'm going to cancel the review for now and revisit this after nss has fixed it.

Comment 6

[Eric Rahm [:erahm]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Hi Eric, have you also filed a bug against nss for this same issue?
> http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/lib/secutil.
> c#246

I haven't seen a coverity issue for it. My understanding is that the code copied into libmar has been modified, which could mean that nss in fact does not leak.

Comment 7

[Robert Strong (they/them - no direct email)]

From security/nss/cmd/lib/secutil.c
char *
SECU_GetPasswordString(void *arg, char *prompt)
...
(void) SECU_GetPasswordString(NULL, prompt);

From modules/libmar/sign/nss_secutil.c
char *
GetPasswordString(void *arg, char *prompt)
...
(void) GetPasswordString(NULL, prompt);

Comment 8

[Eric Rahm [:erahm]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #7)
> From security/nss/cmd/lib/secutil.c
> char *
> SECU_GetPasswordString(void *arg, char *prompt)
> ...
> (void) SECU_GetPasswordString(NULL, prompt);
> 
> From modules/libmar/sign/nss_secutil.c
> char *
> GetPasswordString(void *arg, char *prompt)
> ...
> (void) GetPasswordString(NULL, prompt);

(SECU_)GetPasswordString has different implementations.

Comment 9

[Robert Strong (they/them - no direct email)]

GetPasswordString includes part of http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/lib/secpwd.c#56 which is called by SECU_GetPasswordString. They both use PORT_Strdup. Maybe I'm being dense but it isn't clear to me how GetPasswordString leaks and SECU_GetPasswordString doesn't.

Comment 10

[Eric Rahm [:erahm]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #9)
> GetPasswordString includes part of
> http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/lib/secpwd.
> c#56 which is called by SECU_GetPasswordString. They both use PORT_Strdup.
> Maybe I'm being dense but it isn't clear to me how GetPasswordString leaks
> and SECU_GetPasswordString doesn't.

You're not being dense! I think we're just talking past each other. From my view this code has been forked and modified from the original NSS version so we'll need to fix it anyhow. It's possible NSS doesn't have the same issue, AFAICT it hasn't been reported by Coverity.

I'll go ahead and file a bug there as well so someone on the NSS side can decide whether or not it's an issue. If you feel like we should wait to see what they say let's do that. It just might take a while :)

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 8602489 [details] [diff] [review]
bug1161686_fix_leak_v1.patch

Hey Brian, do you want to make the call on whether this should be fixed in nss first or not?

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 8602489 [details] [diff] [review]
bug1161686_fix_leak_v1.patch

Review of attachment 8602489 [details] [diff] [review]:
-----------------------------------------------------------------

I see some other code that does this, let's let it return a value and clear it before freeing:
if(pw) {
  memset(pw, 0, strlen(pw));
  PORT_Free(pw);
}

Please also link up the other bug to this bug.

Comment 13

[Jose Rios]

Attachment: bug1161686_fix_leak_v2.patch

I'm not sure whether I should wait or not to upload this patch, but here it is as Brian stated. One question, why do we need to memset 'pw'? It doesn't seem necessary to me.

Comment 14

[Brian R. Bondy [:bbondy]]

Comment on attachment 8604411 [details] [diff] [review]
bug1161686_fix_leak_v2.patch

Review of attachment 8604411 [details] [diff] [review]:
-----------------------------------------------------------------

The memset is what is done elsewhere so I'd like to keep it consistent. The reason it was done is because the memory is freed, doesn't necessarily mean it'll be cleared.  Since that location in memory contains a sensitive password it's better to clear it in case it's re-issued when someone else allocates a block and it's inspected before being re-initialized. 

I'm fine with keeping this fork separate / not in sync.

Comment 15

[Jose Rios]

Ok, got it. Thanks for your assistance :)

Comment 16

[Robert Strong (they/them - no direct email)]

I have another patch to push and will push this at the same time. Thanks again José!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f786057e4db3

Comment 18

[Jose Rios]

Robert, I'm not sure again whether this is the right place to comment this, but I realize that my name went wrong in the changesets. Since I didn't pushed the commit myself, is this a typo or some kind of hook that's truncating the author? Thanks for your assistance!

Comment 19

[Robert Strong (they/them - no direct email)]

I just copy your name and email from bugzilla and it is likely the é that is causing this. Sorry about that... I didn't notice.

If you have commit access I'll just let you land in the future. If not, I'm not sure how to resolve this but I will find a solution before I land any of your patches in the future.

Comment 20

[Jose Rios]

That's Ok, I'm just trying to avoid it on the next patches :)
Thank you again!

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f786057e4db3