292390 - NSS tools that use SECU_ParseCommandLine crash when option arguments are omitted - certutil, addbuiltin, blapitest, pk12util, symkeyutil

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

When using the -P option without specifying a prefix, certutil dumps core .
certutil should display the usage instead in this case .

Comment 1

[Julien Pierre]

Attachment: patch for certutil, as checked in to the tip

Comment 2

[Julien Pierre]

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.95; previous revision: 1.94
done

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Wait a minute.  
I think this is a bug in either SECU_ParseCommandLine or PL_GetNextOpt.

It should not be necessary for EVERY case that uses an option argument 
in EVERY nss utility to check for NULL.  

When PL_CreateOptState is given a string with a letter followed by a colon, 
e.g. "P:", if PL_GetNextOpt finds the option letter, but does not find the 
associated argument, then it should return PL_OPT_BAD, not PL_OPT_OK with a 
null optstate->value argument.  

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

(I hit commit before I intended to do so)

If we determine that it is correct for PL_GetNextOpt to return PL_OPT_OK with 
a null optstate->value argument in those cases, then SECU_ParseCommandLine
should enforce that required arguments are always present when the option is
present.

Comment 5

[Wan-Teh Chang]

Since PL_GetNextOpt is not documented, the current
implementation is the de facto specification and we
need to live with its limitations.

The Unix getopt function has separate return values
(':' and '?') for the "option without an operand"
and "unrecognized option" error cases.  In contrast,
PL_GetNextOpt has only one error return value,
PL_OPT_BAD.  This alone implies PL_GetNextOpt can't
do what Nelson wants.

Looking at the source code, I found that PL_GetNextOpt
returns PL_OPT_OK with opt->value set to NULL if the
option's operand is missing.  If we want to give the
user the most informative error message, this is not
really a limitation.  (See how the first example in the
Unix getopt man page cited above handles the ':' case.
If we want to give specific error messages like "missing
input file name" or "missing output file name", we'll
still need to check the value of optopt, and the amount
of code will be similar to that required by PL_GetNextOpt.)
But the ':' return value of getopt does allow a generic
"missing operand" error message to be done easily.

You can modify SECU_ParseCommandLine to return SECFailure
if it gets a NULL optstate->value for an option whose
needsArg field is PR_TRUE.   When SECU_ParseCommandLine
fails, certutil calls Usage, which is exactly what Julien's
patch does.  (That is, Julien's patch does not give a
more specific error message, so modifying SECU_ParseCommandLine
can reduce the amount of code.)

Comment 6

[Wan-Teh Chang]

Attachment: Patch for SECU_ParseCommandLine

This patch should do the trick.  But it may change the
error message that NSS tools return on missing operand.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

So then, apparently my review comment about optstate->value was wrong in 
https://bugzilla.mozilla.org/show_bug.cgi?id=266941#c13
And, It is also the case that there are numerous places in NSS utilities 
that fail to test optstate->value for NULL before using it.  
How sad that all of these cases must now be changed individually to make 
them correct.  :(

Comment 8

[Julien Pierre]

Nelson, Wan-Teh,

Maybe this calls for a new parse function which strictly enforces the presence
of options, if the application doesn't want to give detailed error messages on
missing arguments. That should be an RFE to cmd/lib, and then there should be
bugs filed on the various tools to use it.

Comment 9

[Wan-Teh Chang]

Yes, PL_GetNextOpt is a very short function.  You
can write your own version that meets your requirements.
http://lxr.mozilla.org/nspr/source/nsprpub/lib/libc/src/plgetopt.c

Comment 10

[Julien Pierre]

Wan-Teh,

I was referring to changing the SECU_ParseCommandLine parse function. An extra
"checkOperands" argument can be added to it to enhance your patch. It's OK since
the function is in a static library. Then, all existing programs can pass
PR_FALSE, and programs that want to take advantage of the strict check can pass
PR_TRUE. The good news is that there are only 7 callers of this function under
all of nss/cmd . The bad news is that every one of them would have to be
reviewed to see if the new option would be applicable and wouldn't hide messages
specific to missing operands.

Looking just at certutil, which was the original subject of this bug, it appears
to never check that the "arg" field is non-NULL - they always get passed in to
other functions, which may or may not check them - more likely not . So this
would be an applicable fix. I will attach a patch .

Comment 11

[Julien Pierre]

Attachment: Add checkOperands argument to SECU_ParseCommandLine, and use it in all applicable tools

This patch ensures that there is always a value passed in from
SECU_ParseCommandLine, if the new checkOperands argument is set to PR_TRUE .

I did a very brief reviews of all the tools and how they use their options. I
concluded all are OK to do the strict checking except for signver. I could have
missed some. A more thorough review is appreciated.

all.sh passes with this patch.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Julien, What option of signver allows its argument to be omitted?
Can we simply change signver to require the argument to that option?
I'd prefer that to adding this new function argument.

If we really want to allow some options to have optional arguments, but
most other options to have required arguments, then I think having a single
flag that says "all options' arguments are required" (or none are) does
not achieve that.  Instead, I'd suggest changing the type of the 
"needsArg" member in struct secuCommandFlag from a boolean type to another 
type, (int or an enum) with the values 
  0 == no argument
  1 == argument required
  (other) == argument optional

Comment 13

[Wan-Teh Chang]

Nelson, it's the -n option of signver:
http://lxr.mozilla.org/security/source/security/nss/cmd/signver/signver.c#64

The -n option can be used with the -C or -S command.

Comment 14

[Wan-Teh Chang]

I found that the needsArg field for the -n option
is PR_FALSE.  This means Julien's patch is a no-op
for signver's -n option!

I looked at PL_GetNextOpt again and confirmed that
it will only set opt->value to a non-NULL value if
the option is followed by a colon (:).  So unless
I missed something, signver's -n option can't take
an argument.

Comment 15

[Wan-Teh Chang]

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

Comment 16

[Wan-Teh Chang]

Comment on attachment 182832 [details] [diff] [review]
Add checkOperands argument to SECU_ParseCommandLine, and use it in all applicable tools

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

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

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 18

[Julien Pierre]

Wan-Teh,

I will make the minor changes you suggested and attach a new patch.

Re: signver,

I agree there are pre-existing problems in the command-line interface for that
tool. But let's please not try to fix every single tool problem in a single bug.
Remember that this defect was originally opened only against one particular
option of certutil. The patch at
https://bugzilla.mozilla.org/attachment.cgi?id=182832 goes far beyond that - it
fixes all the options of addbuiltin, blapitest, certutil, dbck (though it's
theoritical, since that tool doesn't compile), pk12util and symkeyutil .
I don't have time to get more involved into this, so I think the scope of this
bug should be limited to these tools. I have updated the summary to reflect
that. A separate bug should be filed on signver, as well as for the tools that
don't use SECU_ParseCommandLine .

Comment 19

[Julien Pierre]

Attachment: incorporate Wan-Teh's feedback

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 183213 [details] [diff] [review]
incorporate Wan-Teh's feedback

Pleasse take the larger comment in comment 12 above as my review comment for
this patch.  In short, I don't think one new artgument that measn "require all"
or "require none" does what we need.

Comment 21

[Julien Pierre]

Nelson,

My take on this is that we shouldn't have options with optional arguments, as it
makes the command-line syntax order-dependent, ie. it works correctly only if
there is one such option specified on a given command line, and if that option
is specified as the end of the command-line. IMO, it just doesn't make sense.
If you accept that it's a bogus requirement, then I think the patch I submitted
yesterday does what we need.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

> we shouldn't have options with optional arguments,
I think that's what I've been saying all along.  If we accept that the 
requirement to support options with optional arguments is a bugus requirement,
then there is no need for a new argument to SECU_ParseCommandLine.  
SECU_ParseCommandLine should always behave as if this argument was true,
that is, always require the presence of the option arguments (when the 
options are present).  And signver should change to work that way too.

BTW, does anyone know what signver DOES?  signatures don't have versions.
Does it mean signature verifier?  should it be signverf?  or verifysig? 

Comment 23

[Wan-Teh Chang]

My patch (attachment 182420 [details] [diff] [review]) does exactly what you want.

Comment 24

[Julien Pierre]

Nelson,

I couldn't tell you what signver does.

Wan-Teh,

Your patch depends on the resolution of signver issues, which are now tracked in
bug 293686 . If somebody wants to take that bug, great, but I can't.

I believe my patch has no side-effect, and it doesn't have that dependency.
We could check it in, and remove the unneeded argument to SECU_ParseCommandLine
after such time that the signver bug is resolved. Or we could do nothing, and
wait for signver to be fixed.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 182832 [details] [diff] [review]
Add checkOperands argument to SECU_ParseCommandLine, and use it in all applicable tools

removing review request  from obsolete patch.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 182420 [details] [diff] [review]
Patch for SECU_ParseCommandLine

I would give this bug r+ except that it doesn't set the error code before
returning SECFailure.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 183213 [details] [diff] [review]
incorporate Wan-Teh's feedback

I think Julien and I agreed, in comments 21 and 22 that the approach taken in
this patch is not the one we want to take. r-

Comment 28

[Julien Pierre]

Unfortunately, this missed the 3.10 train . Our customers will continue to
experience crashes with the command-line tools because we wouldn't fix an
obscure one (signver). Thus, I'm making the signver bug a dependency of this one .
Retargetting this bug for 3.11 .

Comment 29

[Julien Pierre]

Attachment: simpler patch. Does not allow optional arguments

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 194735 [details] [diff] [review]
simpler patch. Does not allow optional arguments

This patch attempts to solve the problem of missing arguments to options by
making SECU_ParseCommandLine return SECFailure in such cases.  IMO, that's
fine,
but not all NSS commands pay attention to that return value.  
The NSS command signver (subject of bug 293686) ignores the value returned 
by SECU_ParseCommandLine.  
See http://lxr.mozilla.org/security/search?string=SECU_ParseCommandLine

So, this patch is fine, but another patch to signver.c is needed to avoid the
crash in signver.

Comment 31

[Julien Pierre]

Nelson, thanks for the review. I checked in this patch on the trunk .

Checking in secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.70; previous revision: 1.69
done

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

As originally written, this bug potentially applied to all NSS commands 
whose options take arguments.  
A fix was made that solved this problem for NSS commands that parse 
their command lines using SECU_ParseCommandLine.  
But that fix does not benefit the NSS commands that do not use 
SECU_ParseCommandLine, but rather call PL_GetNextOpt directly. 
Bug 310499 cites one example of such an NSS program.
So I am changing the summary of this bug to narrow its scope to 
include only NSS commands that use SECU_ParseCommandLine.

Perhaps we need an umbrella bug (as this one was) that covers all
the NSS commands that use PL_GetNextOpt but not SECU_ParseCommandLine
and that do not check their option arguments for NULL before using them.