file descriptor leak in "secu_InitSlotPassword" in 'nss/cmd/lib/secutil.c'

RESOLVED FIXED in 3.17.3

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sachingpt999, Unassigned)

Tracking

3.16.4
3.17.3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 8492584 [details] [diff] [review]
Patch for the above raised bug

File in which bug exists : nss/cmd/lib/secutil.c

Function name in which bug exists: secu_InitSlotPassword

The file descriptor 'input' has not been taken care of, when file descriptor 'output' is NULL and function terminates due to the return statement (line 293). 


Before calling return in 'output' NULL check, file descriptor input should be closed using fclose().

NSS 3.16.4 version code::

if (input == NULL) {
        PR_fprintf(PR_STDERR, "Error opening input terminal for read\n");
        return NULL;
    }

    /* we have no password, so initialize database with one */
    PR_fprintf(PR_STDERR,
        "Enter a password which will be used to encrypt your keys.\n"
        "The password should be at least 8 characters long,\n"
        "and should contain at least one non-alphabetic character.\n\n");

    output = fopen(consoleName, "w");
    if (output == NULL) {
        PR_fprintf(PR_STDERR, "Error opening output terminal for write\n");
        return NULL;
    }

Recommended code:

if (input == NULL) {
        PR_fprintf(PR_STDERR, "Error opening input terminal for read\n");
        return NULL;
    }

    /* we have no password, so initialize database with one */
    PR_fprintf(PR_STDERR,
        "Enter a password which will be used to encrypt your keys.\n"
        "The password should be at least 8 characters long,\n"
        "and should contain at least one non-alphabetic character.\n\n");

    output = fopen(consoleName, "w");
    if (output == NULL) {
        PR_fprintf(PR_STDERR, "Error opening output terminal for write\n");
#ifndef _WINDOWS
        fclose(input);
#endif
        return NULL;
    }
(Reporter)

Updated

4 years ago
Attachment #8492584 - Flags: review?(kaie)
(Reporter)

Comment 1

4 years ago
Reminder for bug and patch review.
(Reporter)

Comment 2

4 years ago
Reminder for bug review.
Flags: needinfo?(ryan.sleevi)
Flags: needinfo?(rrelyea)

Comment 3

4 years ago
Comment on attachment 8492584 [details] [diff] [review]
Patch for the above raised bug

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

r+ rrelyea
Attachment #8492584 - Flags: review?(kaie) → review+
(Reporter)

Comment 4

4 years ago
Reminder for code change check in.
(Reporter)

Comment 5

4 years ago
Reminder for code check in.
Flags: needinfo?(wtc)
Flags: needinfo?(kaie)

Comment 6

4 years ago
Thank you for your patch and the reminder!

Checked in:
https://hg.mozilla.org/projects/nss/rev/e982d1ffdca7
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(wtc)
Flags: needinfo?(ryan.sleevi)
Flags: needinfo?(rrelyea)
Flags: needinfo?(kaie)
Resolution: --- → FIXED
Target Milestone: --- → 3.17.3

Updated

4 years ago
Target Milestone: 3.17.3 → 3.18

Comment 7

4 years ago
Comment on attachment 8492584 [details] [diff] [review]
Patch for the above raised bug

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

::: cmd/lib/secutil.c
@@ +290,5 @@
>      output = fopen(consoleName, "w");
>      if (output == NULL) {
>  	PR_fprintf(PR_STDERR, "Error opening output terminal for write\n");
> +#ifndef _WINDOWS
> +	fclose(input);

It seems that the preexisting fclose(input) call at the end
of secu_InitSlotPassword should also be put inside #ifndef _WINDOWS.

Should we make that change?
Attachment #8492584 - Flags: review+

Updated

4 years ago
Target Milestone: 3.18 → 3.17.3
You need to log in before you can comment on or make changes to this bug.