Closed Bug 53229 Opened 20 years ago Closed 14 years ago

certutil should not use gets()

Categories

(NSS :: Tools, defect, P1)

x86
FreeBSD
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: lennox, Assigned: alvolkov.bgs)

References

Details

Attachments

(2 files, 4 obsolete files)

mozilla/security/nss/cmd/certutil/certutil.c uses gets() all over the place.
Not only is gets() universally deprecated in favor of fgets(), it also
generates link-time and run-time warnings on FreeBSD (and probably other
platforms as well).

In most of the cases where it's used in certutil.c, 'gets(buffer)' can be
simply replaced by 'fgets(buffer, sizeof(buffer), stdin)', since buffer is
usually allocated on the stack.  In some of the cases, the terminating newline
needs to be trimmed as well.
Ian, please take a stab at this after the 3.1 release.
Assignee: wtc → mcgreer
Target Milestone: --- → 3.2
have the fix in my tree, but too late (and too many perturbations) for 3.2.  
moving to 3.3.
Target Milestone: 3.2 → 3.3
Ian, do you still have the fix for this in your tree?
Target Milestone: 3.3 → 3.4
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
Set target milestone to NSS 3.5.
Target Milestone: 3.4 → 3.5
Priority: P3 → P2
Target Milestone: 3.5 → 3.7
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
Assignee: bugz → alexei.volkov.bugs
Adding Neil and myself to this bug.  I think Neil has touched some of the 
code that uses gets recently.  Considering this is a command line utility
used in QA scripts, I think this is not high priority, not a security risk.

But if there are violations of the principle of least astonishment, they
should be fixed.  We certainly don't want to be adding unknown OID values
to EKU extensions, for example.
Attached patch bug fix patch (obsolete) — Splinter Review
Attaching patch that will fix reported issue.

The only problem I found is with AddExtKeyUsage (void *extHandle).
Bug 287285 is filed for that problem.
Attachment #178285 - Attachment is obsolete: true
Comment on attachment 178285 [details] [diff] [review]
bug fix patch

sorry, patch was not tested
Comment on attachment 178285 [details] [diff] [review]
bug fix patch

The change from gets to fgets is not as
straightforward as it seems.

The obvious differences are the need to
specify the buffer size and input stream.

The subtle difference is that fgets includes
the newline character, but gets doesn't.  We
need to deal with this extra newline character
that may be in the buffer.

Another thing to watch out for is that the
current code doesn't check the return value
of gets.  If the new code does, it may need
to handle EOF and error differently.  (Both
result in a NULL return from fgets.)
Here's another unsolicited review comment :)
Any new place that returns SECFailure after an fgets failure needs to 
call PORT_SetError to set the error code before returning.
QA Contact: bishakhabanerjee → jason.m.reid
Attached patch fix (obsolete) — Splinter Review
The fix includes changes to escape infinite loop when creating general name object. Also, scanf calls replaced to __gets_s in combination with atoi as scanf leaves '\n' in the stdin stream after reading a number which makes fgets as well as gets to read empty line value without waiting for user input. GetYesNo modified to use libc function. GetYesNo is used where yes/no respond from user is needed.
Attachment #202586 - Flags: superreview?(wtchang)
Attachment #202586 - Flags: review?(julien.pierre.bugs)
Comment on attachment 202586 [details] [diff] [review]
fix

Alexei,  Your discovery about scanf was a good catch.
One problem with the existing function getYesNo: 
If it gets EOF, the buffer contents will be uninitialized,
and so the return value will be unpredictable.

Review comments:

>? cert8.db
>? certutil.c.getYesNo
>? certutil.c.or
>? certutil.c.scanf
>? db

Please remove lines like the above from patch files before
attaching them.  Thanks.

>+static char *
>+__gets_s(char *buff, PRInt32 size) {

I think that name doesn't meet our naming rules.
This is not library code.  
Please choose a name with no leading underscores.  

Here's another suggestion.  If you define gets as follows:

#undef gets
#define gets(x) myGets(x, sizeof(x))

then you can leave the existing calls to gets alone (I think).
Attachment #202586 - Flags: review-
Comment on attachment 202586 [details] [diff] [review]
fix

>+static char *
>+__gets_s(char *buff, PRInt32 size) {

Remove the __ in __gets_s.  Use size_t as the type for
the 'size' parameter.

Don't define gets as a macro as Nelson suggested.
That'll confuse people who look for gets calls in
our source code.  If you really want to use a macro,
name it GETS.

>+        if (buff[len - 1] == '\n' || buff[len - 1] == '\r')
>+            buff[len - 1] = '\0';

This should be written like this:

      /*
       * fgets() automatically converts native text file
       * line endings to '\n'.  Just in case stdin is put
       * in binary mode, we handle three native text file
       * line endings here:
       *   '\n'      Unix (including Linux and Mac OS X)
       *   '\r''\n'  DOS/Windows
       *   '\r'      Mac OS Classic
       */
      if (buff[len - 1] == '\n' || buff[len - 1] == '\r') {
          buff[len - 1] = '\0';
          if (len > 1 && buff[len - 2] == '\r')
              buff[len - 2] = '\0';
      }
Attachment #202586 - Flags: superreview?(wtchang)
Attachment #202586 - Flags: superreview-
Attachment #202586 - Flags: review?(julien.pierre.bugs)
Wan-Teh,

I'm the one who suggested that Alexei name his function __gets_s . This is because some recent compilers include a function called gets_s, but not all of them do, which is why it had to be implemented here in the tool . The purpose of __ is to avoid a symbol conflict. Hopefully at some point in the future, the code can be switched to using the libc/compiler-provided gets_s .
*** Bug 315503 has been marked as a duplicate of this bug. ***
Attached patch fix (obsolete) — Splinter Review
Added recommended in review changes.
Attachment #202586 - Attachment is obsolete: true
Attachment #202744 - Flags: superreview?(wtchang)
Attachment #202744 - Flags: review?(julien.pierre.bugs)
Attached patch certutil regression tests (obsolete) — Splinter Review
certutil extension tests
Attachment #202746 - Flags: review?(julien.pierre.bugs)
Comment on attachment 202744 [details] [diff] [review]
fix

This looks good. I suggest the following changes before checkins :

1) in the comments for Gets_S about the native line endings, note that '\r''\n' is DOS, Windows & OS/2

2) the new atoi calls should be PORT_Atoi . Unfortunately, the certutil source is already inconsistent - there is a mix of atoi and PORT_Atoi . I think the better one is PORT_Atoi . Replacing the existing atoi calls is outside the work for this bug, but at least the new code should be PORT_Atoi .
Attachment #202744 - Flags: review?(julien.pierre.bugs) → review+
Changing priority to have the bug on P1 bug list
Priority: P2 → P1
Target Milestone: --- → 3.11
Comment on attachment 202744 [details] [diff] [review]
fix

Alexei, I like this patch.  Thanks.

I have some comments and suggested changes.

1. Suggested change

>+static char *
>+Gets_s(char *buff, size_t size) {
>+    char *str;
>+    if ((str = fgets(buff, size, stdin)) != NULL) {
>+        int len = PORT_Strlen(str);
>+        /*
>+         * fgets() automatically converts native text file
>+         * line endings to '\n'.  Just in case stdin is put
>+         * in binary mode, we handle three native text file
>+         * line endings here:
>+         *   '\n'      Unix (including Linux and Mac OS X)
>+         *   '\r''\n'  DOS/Windows
>+         *   '\r'      Mac OS Classic
>+         */
>+        if (buff[len - 1] == '\n' || buff[len - 1] == '\r') {
>+          buff[len - 1] = '\0';
>+          if (len > 1 && buff[len - 2] == '\r')
>+              buff[len - 2] = '\0';
>+        }

Unless fgets has a bug or certutil puts stdin in binary
mode by mistake, we should not need to handle '\r'.  So
I suggest that we either make this point clear in the
comment:

  As defensive programming (just in case fgets has a bug
  or we put stdin in binary mode by mistake), we handle
  three native ...

or just trust that fgets is correctly implemented (which
is what I would do and would make the comment unnecessary):

    if (buff[len - 1] == '\n') {
        buff[len - 1] = '\0';
    }

I also like your original comment that points out that
len cannot be less than 1.

2. Commentary

> static SECStatus 
> GetString(PRArenaPool *arena, char *prompt, SECItem *value)
> {
>     char buffer[251];
> 
>+    buffer[0] = '\0';
>     value->data = NULL;
>     value->len = 0;
>     
>     puts (prompt);
>-    gets (buffer);
>+    Gets_s (buffer, sizeof(buffer));
>+    /* returned NULL here treated the same way as empty string */
>     if (strlen (buffer) > 0) {

and

> static PRBool 
> GetYesNo(char *prompt) 
> {
>     char buf[3];
> 
>-    PR_Sync(PR_STDIN);
>-    PR_Write(PR_STDOUT, prompt, strlen(prompt)+1);
>-    PR_Read(PR_STDIN, buf, sizeof(buf));
>+    buf[0] = 'n';
>+    puts(prompt);
>+    Gets_s(buf, sizeof(buf));
>     return (buf[0] == 'y' || buf[0] == 'Y') ? PR_TRUE : PR_FALSE;

It seems better to test the return value of Gets_s
rather than setting buffer[0] to '\0' (or buf[0] to 'n')
and hoping that fgets won't touch the buffer on failure.

3. Suggested change

> static SECStatus 
> AddBasicConstraint(void *extHandle)
> {
...
>-	buffer[0] = 'n';
>-	puts ("Is this a critical extension [y/n]? ");
>-	gets (buffer);	
>+	yesNoAns = GetYesNo ("Is this a critical extension [y/N]?\n");

Remove the "\n" from the GetYesNo call, unless you added
the "\n" intentionally.

4. Commentary
 
> static SECStatus 
> AddCrlDistPoint(void *extHandle)
> {
...
> 	/* Get the reason flags */
> 	puts ("\nSelect one of the following for the reason flags\n");
> 	puts ("\t0 - unused\n\t1 - keyCompromise\n\t2 - caCompromise\n\t3 - affiliationChanged\n");
> 	puts ("\t4 - superseded\n\t5 - cessationOfOperation\n\t6 - certificateHold\n");
> 	puts ("\tother - omit\t\tChoice: ");
> 	
>-	scanf ("%d", &intValue);
>+	if (Gets_s (buffer, sizeof(buffer)) == NULL) {
>+	    PORT_SetError(SEC_ERROR_INPUT_LEN);
>+            GEN_BREAK (SECFailure);
>+        }
>+        intValue = atoi (buffer);
> 	if (intValue >= 0 && intValue <8) {
> 	    current->reasons.data = PORT_ArenaAlloc (arena, sizeof(char));
> 	    if (current->reasons.data == NULL) {
> 		GEN_BREAK (SECFailure);
> 	    }
> 	    *current->reasons.data = (char)(0x80 >> intValue);
> 	    current->reasons.len = 1;
> 	}

If buffer does not contain the string representation
of an integer, atoi(buffer) returns 0, which is a value
value (>= 0 and < 8) and will cause us to set the reason
to "0 - unused", as if the user has entered "0".

The original code leaves garbage in intValue if the
user input is not an integer string.  So it could be
worse.
Attachment #202744 - Flags: superreview?(wtchang) → superreview+
added:
    * Gets_s return value check in GetString and GetYesNo functions.
    * additional check for '0' in input buffer where PORT_Atoi converts string to int
    * input arguments values check in Gets_s function
    * buffer null termination in case when fgets returns NULL
Attachment #202744 - Attachment is obsolete: true
Comment on attachment 202746 [details] [diff] [review]
certutil regression tests

The tests did not run for me when I tried this patch. There is a bug on this line :

+        if [ "$arg" = "=" ]; then


it should be

+        if [ "$arg" -eq "=" ]; then

Even after I fixed this, a bogus certutil command was invoked by the script, and the usage was displayed.
Attachment #202746 - Flags: review?(julien.pierre.bugs) → review-
Attachment #203680 - Attachment description: fix → fix as checked in
The bug was on another line where I used '==' in if statement. Solaris test utility still can not understand '==' and expects '=' instead.
Anyway, this is fixed and comments have been added into certext.txt file for better syntax understanding.
Attachment #202746 - Attachment is obsolete: true
Attachment #205424 - Flags: review?(julien.pierre.bugs)
Attachment #205424 - Flags: review?(julien.pierre.bugs) → review+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.