293686 - signver has command-line options with optional arguments; and may crash if some arguments are omitted

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

This was originally discovered as part of bugzilla 292390 .
signver has command-line with optional arguments, which doesn't make sense. This
needs to be fixed.
Also, the tool will crash if options are omitted.

Once the usage is resolved, we can set the checkOptionArgs in
SECU_ParseCommandLine to PR_TRUE, which will enable checking for the presence of
arguments on the command-line from the parser .

------ Additional Comment #15 From Wan-Teh Chang  2005-05-09 16:31 PDT  [reply]
-------

OK, signver's -i and -s options also take an optional
argument.  If the argument is omitted, the -i and -s
option use stdin.
http://lxr.mozilla.org/security/source/security/nss/cmd/signver/signver.c#58

I also found the explanation for the weirdness with the
-n option.  The commands it is used with, -C and -S, are
not implemented:
http://lxr.mozilla.org/security/source/security/nss/cmd/signver/signver.c#416


------- Additional Comment #16 From Wan-Teh Chang 2005-05-09 16:39 PDT [reply]
-------

(From update of attachment 182832 [details] [diff] [review] [edit])
I will let Nelson decide what to do with this issue.

This patch has two minor problems.

>Index: lib/secutil.c
...
>@@ -3062,10 +3063,12 @@
> 	for (i=0; i<cmd->numCommands; i++) {
> 	    if (cmd->commands[i].flag == optstate->option) {
> 		cmd->commands[i].activated = PR_TRUE;
> 		if (optstate->value) {
> 		    cmd->commands[i].arg = (char *)optstate->value;
>+		} else if (checkOperands && cmd->commands[i].needsArg) {
>+		    return SECFailure;
> 		}
> 		found = PR_TRUE;
> 		break;
> 	    }
> 	}

This change is not necessary because the "commands" do not
have arguments by definition.  Only the "options" may have
arguments.

The "checkOperands" parameter needs to be documented in the
secutil.h header file.	I'd name the parameter "checkOptionArgs"
instead. ("Operand" is ambiguous in this context.)


------- Additional Comment #17 From Nelson Bolyard 2005-05-09 19:08 PDT [reply]
-------

I believe that argument to options should be required.  
Consider this option defining string "a:b:c:" 
This can be satisfied with   -a foo -b bar -c bif
but also with                -abc foo bar bif
and                          -cba bif bar foo

Order of arguments is generally irrelevant, and optargs may follow groups 
options.  

Now, what heppens when the coder of the -b option decides that the argument
to -b is optional, and the user decides to eliminat "bar". 
How are the cases above disambiguated?

   -abc foo bif  
is bif the argument to -b or to -c ?

   -a foo -b 0c bif
I think this has the same ambiguity as the previous example.

If the -i and -o options want a way to specify stdin or stdout, 
I think it is more customary to use "-" as the argument for that.  
Does the PL_GetNextOpt parser handle that correctly?
If so, I'd suggest changing signver to use - rather than nothing 
for that purpose.

Also, is signver a supported utility?

In any case, signver's usage should bew corrected to match the program's
true behavior.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

In the original comment above, (which appears to be comment 17) I asked
about code that declares the options "a:b:c:" and expects to interpret
  -a xxx -b yyy -c zzz
the same as
  -abc xxx yyy zzz

However, I have subsequently determined that NSPR's option parser does not 
permit the latter construct.  In the latter example shown above, the 
value yyy will not be parsed as the argument to the -b option.  

It would be helpful to the users of NSPR (including the NSS developers) 
if the full behavior of NSPR's command parsing would be documented.

Comment 2

[Julien Pierre]

Attachment: remove unimplemented options, and change usage to use - for stdin

Comment 3

[Julien Pierre]

Note that I'm not checking for missing argument values on purpose here. That
will be fixed in bug 292390 in SECU_ParseCommandLine .

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194732 [details] [diff] [review]
remove unimplemented options, and change usage to use - for stdin

r=nelson, for checkin after we make the alpha.	Comments below.

>Index: signver.c

>-	    !signver.options[opt_InputDataFile].arg &&
>-	    !signver.options[opt_InputSigFile].arg)
>+    	    !PL_strcmp("-", signver.options[opt_InputDataFile].arg) &&
>+	    !PL_strcmp("-", signver.options[opt_InputSigFile].arg))

>-		if (signver.options[opt_InputDataFile].arg)
>+		if (PL_strcmp("-", signver.options[opt_InputDataFile].arg))

This will crash if arg is NULL, but you said you're explicitly not fixing that
in this patch, because it is the subject of another bug.

>-	if (signver.commands[cmd_DisplayCertInfo].activated &&
>-	    !signver.options[opt_ItemNumber].arg)
>-		displayAllCerts = PR_TRUE;
>-
>-	if (signver.commands[cmd_DisplaySignerInfo].activated &&
>-	    !signver.options[opt_ItemNumber].arg)
>-		displayAllSigners = PR_TRUE;
>-
> #if 0
> 			case 'W':
> 				debugInfo = 1;
> 				break;
> #endif

May I suggest that you get rid of the "#if 0" code, too?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Unfortunately, this bug is not completely fixed by the patches now attached
to this bug and to bug 292390.  With both patches applied, signver will 
still crash when an option argument is omitted.  The problem is in this line
from signver.c

> 	SECU_ParseCommandLine(argc, argv, progName, &signver);

It ignores the return value, and goes ahead.  So, The SECFailure returned by
SECU_ParseCommandLine will not stop the code following this call from using
that NULL pointer.  

Comment 6

[Julien Pierre]

Nelson, I checked in this patch as is. Thanks for the review.

Checking in signver.c;
/cvsroot/mozilla/security/nss/cmd/signver/signver.c,v  <--  signver.c
new revision: 1.11; previous revision: 1.10
done

I will create another patch that removes the #if 0 code and checks the status
for SECU_ParseCommandLine . It appears to be the only tool under cmd that
doesn't check it.

Comment 7

[Julien Pierre]

Attachment: check status from SECU_ParseCommandLine and display usage ; remove one instance of #if 0 code

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 196695 [details] [diff] [review]
check status from SECU_ParseCommandLine and display usage ; remove one instance of #if 0 code

r=nelson for fixing the call to SECU_ParseCommandLine.

While you're at it, please get rid of all the #if 0 code in that function. 
There are numerous #if 0 blocks visible in your patch, adjoining the lines you
patched.

Comment 9

[Julien Pierre]

Thanks for the review, Nelson. I checked this in to the tip.

Checking in signver.c;
/cvsroot/mozilla/security/nss/cmd/signver/signver.c,v  <--  signver.c
new revision: 1.12; previous revision: 1.11
done

Comment 10

[Wan-Teh Chang]

The change to the way stdin is specified needs to be
documented in the NSS 3.1 Release Notes.  We should
also *try to* update the signver documentation in the CMS
documentation.

Comment 11

[Wan-Teh Chang]

Comment on attachment 194732 [details] [diff] [review]
remove unimplemented options, and change usage to use - for stdin

There are two instances of "(no arg == use stdin)" in the comments.
These need to be updated.  They are:

> 	/*  Open the input data file (no arg == use stdin). */
> 	if (signver.options[opt_InputDataFile].activated) {
>-		if (signver.options[opt_InputDataFile].arg)
>+		if (PL_strcmp("-", signver.options[opt_InputDataFile].arg))

and

> 	/*  Open the input signature file (no arg == use stdin).  */
> 	if (signver.options[opt_InputSigFile].activated) {
>-		if (signver.options[opt_InputSigFile].arg)
>+		if (PL_strcmp("-", signver.options[opt_InputSigFile].arg))