Closed Bug 1161686 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: joseriosneto, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1293420][lang=c++][good first bug])

Attachments

(1 file, 1 obsolete file)

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
Summary: libmar's |SECU_GetModulePassword| can leak allocated string → libmar's |SECU_GetModulePassword| can leak allocated string (command line build utility)
Mentor: erahm
Whiteboard: [CID 1293420] → [CID 1293420][lang=c++][good first bug]
Hi Eric and Robert, can I take this bug?
Hi José, yes you can and thanks!
Attached patch bug1161686_fix_leak_v1.patch (obsolete) — Splinter Review
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?
Attachment #8602489 - Flags: review?(robert.strong.bugs)
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
Flags: needinfo?(erahm)
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.
Attachment #8602489 - Flags: review?(robert.strong.bugs)
(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.
Flags: needinfo?(erahm)
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);
(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.
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.
(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 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?
Attachment #8602489 - Flags: review?(netzen)
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.
Attachment #8602489 - Flags: review?(netzen)
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.
Attachment #8602489 - Attachment is obsolete: true
Attachment #8604411 - Flags: review?(netzen)
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.
Attachment #8604411 - Flags: review?(netzen) → review+
Ok, got it. Thanks for your assistance :)
I have another patch to push and will push this at the same time. Thanks again José!
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!
Flags: needinfo?(robert.strong.bugs)
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.
Assignee: nobody → joseriosneto
Flags: needinfo?(robert.strong.bugs)
That's Ok, I'm just trying to avoid it on the next patches :)
Thank you again!
https://hg.mozilla.org/mozilla-central/rev/f786057e4db3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1170360
No longer depends on: 1170360
You need to log in before you can comment on or make changes to this bug.