449332 - SECU_ParseCommandLine does not validate its inputs

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

SECU_ParseCommandLine takes, as its fourth argument, an array of 
secuCommandFlag structures.  It does not validate the contents of that array.
It does not detect that the array contains duplicate definitions for some
command letters, of which all but the first will never be honored.  
It should check for such duplicates and ASSERT that no duplicates exist.

This is arguably an enhancement request, but given that we have actually had
such duplicates checked in, I consider the failure to detect these duplicates
to be a bug in SECU_ParseCommandLine.

Comment 1

[Elio Maldonado]

Attachment: Detects duplicate options and commands

Added a function (and two macros) to detect duplicate command line options. The test for duplicates is invoked via assertions. This a crude linear search over the commands and options arrays with no attempt to be informative to the calling tool. It is invoked via assertion. Found such duplicate in pk12util.

Comment 2

[Elio Maldonado]

Use of temporary variables in HasNoDuplicates
char t = a[i].flag; and char *target = a[i].longform;
is unnecessary and doesn't even add to readability.

Defined "friendly"  macros
+#define HasNoDuplicateCommands(c) HasNoDuplicates((c)->commands,(c)->numCommands)
+#define HasNoDuplicateOptions(c) HasNoDuplicates((c)->options,(c)->numOptions)

but below called the function directly
+    PR_ASSERT(HasNoDuplicates(cmd->commands, cmd->numCommands));
+    PR_ASSERT(HasNoDuplicates(cmd->options, cmd->numOptions));

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Elio's patch, ignoring white space changes

Elio, Apparently you used a text editor that removes trailing white space
on all lines of the files it edits.  That causes your patches to appear to 
be HUGE, and hides the changes you really intend to make.  

I used cvs diff -puw to produce a new version of your patch, without all
the white space changes.  It is MUCH smaller than the original patch.  
I will review it. 

Please see if you can change the behavior of your editor so that it no longer
makes all those unnecessary white space changes.  I think patches with all
those unnecessary white space changes will generally get r-.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 376360 [details] [diff] [review]
Elio's patch, ignoring white space changes

There are several issues with this patch.

1) Function HasNoDuplicates is only called from with the PR_ASSERT() macro, 
which means that it will not be called in optimized builds.  So, it should be
enclosed inside an #ifdef to avoid building it when it will not be used.
The correct ifdef to use is:

#if defined(DEBUG) || defined(FORCE_PR_ASSERT)

2) Function HasNoDuplicates seems to assume that short, 1-character flag names
and longer "longform" names are mutually exclusive.  It checks for duplicates of
one, or the other, but not both.  They are not mutually exclusive.  It's quite 
OK for an option to have both a long and a short form name.  This function 
should detect duplicates of both forms, without regard to the presence or absence
of the other form. 

3) This patch defines two macros, named HasNoDuplicateCommands and 
HasNoDuplicateOptions, but does not use them anywhere.  Please just 
eliminate them.  I think the direct invocations of HasNoDuplicates are fine.

4) This patch mysteriously adds the string "ssbb" to one of the usage messages.

5. Please don't use option '0' as the short flag name for opt_CertKeyLength.
'0' and 'O' are too easily confused.  Please find another lower case letter to use,
and be sure it appears in the usage message.

Comment 5

[Elio Maldonado]

Attachment: Detects duplicate options and fixes duplicate option in pk12util

Addresses comments in review. Note that we allow duplicated short option of '0' as long as the long forms are different. This occurs in certutil where some of the new long form options have no corresponding short form.

Comment 6

[Elio Maldonado]

Attachment: Detects duplicate options

Fix some errors pointed out by Nelson. Fixed an inaccurate comment on secutil.c, it's 0 ('\0') the value used with long options that don't have a short version and not '0'. pk12util now respect the 80-char limit and both code and the output of the usage message. Notice also that the --key-len and --cert-key-len now have  only the long form as -m and -q are hardly mnemonic in this two cases. These are likely seldom used and hopefully in this short time no automation scripts have become addicted to -m :-).

Comment 7

[Elio Maldonado]

Comment on attachment 376944 [details] [diff] [review]
Detects duplicate options

Now the message should come out right :-)

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 376944 [details] [diff] [review]
Detects duplicate options

Elio, this patch is good, except for the removal of the -m flag
for the key_len option.  Since -m appeared in NSS 3.12.3, which is
now installed on customers' systems, we cannot remove it. 
So, please put that -m option back.  It's OK for that option to 
have two alternative ways of being specified.

Comment 9

[Elio Maldonado]

Ah, some inner voice told me earlier that dropping the -m would not pass muster but I couldn't resist trying. New patch coming soon.

Comment 10

[Elio Maldonado]

Attachment: Detects duplicate options

Restored -m option short opetion to preserve binary compatibility with previously shipped version

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 377091 [details] [diff] [review]
Detects duplicate options

r=nelson

Comment 12

[Elio Maldonado]

Fix commited
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.97; previous revision: 1.96
done
Checking in pk12util/pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.42; previous revision: 1.41
done