53229 - certutil should not use gets()

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Jonathan Lennox]

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.

Comment 1

[Wan-Teh Chang]

Ian, please take a stab at this after the 3.1 release.

Comment 2

[Ian McGreer]

have the fix in my tree, but too late (and too many perturbations) for 3.2.  
moving to 3.3.

Comment 3

[Wan-Teh Chang]

Ian, do you still have the fix for this in your tree?

Comment 4

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 5

[Wan-Teh Chang]

Set target milestone to NSS 3.5.

Comment 6

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[Alexei Volkov]

Attachment: bug fix patch

Attaching patch that will fix reported issue.

The only problem I found is with AddExtKeyUsage (void *extHandle).
Bug 287285 is filed for that problem.

Comment 10

[Alexei Volkov]

Comment on attachment 178285 [details] [diff] [review]
bug fix patch

sorry, patch was not tested

Comment 11

[Wan-Teh Chang]

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.)

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 13

[Alexei Volkov]

Attachment: fix

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.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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).

Comment 15

[Wan-Teh Chang]

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';
      }

Comment 16

[Julien Pierre]

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 .

Comment 17

[Daniel Veditz [:dveditz]]

*** Bug 315503 has been marked as a duplicate of this bug. ***

Comment 18

[Alexei Volkov]

Attachment: fix

Added recommended in review changes.

Comment 19

[Alexei Volkov]

Attachment: certutil regression tests

certutil extension tests

Comment 20

[Julien Pierre]

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 .

Comment 21

[Alexei Volkov]

Changing priority to have the bug on P1 bug list

Comment 22

[Wan-Teh Chang]

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.

Comment 23

[Alexei Volkov]

Attachment: fix as checked in

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

Comment 24

[Julien Pierre]

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.

Comment 25

[Alexei Volkov]

Attachment: regression test patch

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.