293687 - many NSS command-line tools may crash with missing argument values

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

The following tools may crash when passed command-line options without values.
Their code needs to be tested and reviewed. This is because the PL_GetNextOpt
function does not check for argument values .

SSLsample
addbuiltin
atob
bltest
btoa
certcgi
certutil
cgi
checkcert
cipher
cmdlib
crlutil
crmf-cgi
crmftest
dbck
dbdump
dbtest
derdump
digest
doc
fipstest
ilock
include
keyutil
lib
modutil
nssutil
ocspclnt
oidcalc
p7content
p7env
p7sign
p7verify
pk11util
pk12util
pkiutil
pp
preenc
pwdecrypt
regress
replacer
reporter
revoker
rsaperf
samples
sdrtest
selfserv
shlibsign
signtool
signver
smimetools
sslstrength
ssltap
strsclnt
swfort
symkeyutil
tests
tstclnt
ttformat
vfychain
vfyserv
zlib

Comment 1

[Julien Pierre]

Woops. I was a bit too quick when pasting this list. The following tools should
not be in this list :

certutil, addbuiltin, blapitest, pk12util, symkeyutil, dbck - see bug 292390
signver - see bug 293686

Comment 2

[Julien Pierre]

We should consider not using PL_GetNextOpt in any tool that doesn't check optstate->value . There should be an alternative function that checks for the argument value presence, and the tools should be changed to use it. Raising priority.

Comment 3

[Julien Pierre]

This doesn't need to be P1, since this type of crash is not exploitable.

Comment 4

[Julien Pierre]

Reassigning to Alexei .

Comment 5

[Julien Pierre]

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

Comment 6

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 7

[Alexei Volkov]

Change to parser of cmd line argument will happen in 3.12. Retargeting this bug to 3.12.

Comment 8

[Biswatosh Chakraborty]

Julien,

1)Certain commands(e.g client) don't crash upon not giving any 
argument value. 
The reason is, client needs a hostname as the last
argument and if we run "$client -n myhost", it takes myhost as nickname
and doesnot find the hostname agrument and returns error. Similarly,
many other commands may not crash. But certain commands do crash, such 
as "$crlutil -L -n" as there is no such "saver" argument, which can
help avoid a crash.

2)Even if a command is not crashing for such interesting reasons, I think
it is wrong and we should solve it. 
One way is, for each such command which uses PL_GetNextOpt(), check for 
any missing argument value. 
Another way is,
don't use PL_GetnextOpt() but use SECU_ParseCommandLine() instead, just as
certutil and a few other cmds are doing. 
And the third option is to write
a function similar to PL_GetNextOpt(), but which checks for argument values.
In comment#2, was that your idea? 

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 10

[Elio Maldonado]

The lack of validation for missing required arguments to command line options by the tools has been independently brought to my attention by Red Hat Quality Engineering folks as well. It's about time we addressed this so I'm taking this bug.

Comment 11

[Elio Maldonado]

Attachment: Add option argument validation to various tools

This patch ensures that when a command line option requires an argument the
value isn't NULL before trying a string duplicate on the value. Upon failure
the tools print which option requires and argument followed by the general usage
message and exit. This patch still keeps the tools use of PL_GetOpt for now.

Switching to SECU_ParseCommand seems preferable and if you wish I am willing to pursue that approach.

Comment 12

[Elio Maldonado]

s/PL_GetOpt/PL_GetNextOpt and s/SECU_ParseCommand/SECU_ParseCommandLine in the comments above.

Comment 13

[Elio Maldonado]

Because of other commitments I don't currently have the time to do the preferred fix style, change the tools to use SECU_ParseCommandLine, I am going to request a review of this patch as it is. We can always enter a separate bug for the more elaborate fix at a later time.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Elio, In light of the recent resolution of Bug 547677, it's possible that
with the new version of NSPR (which contains the fix for that bug), THIS
NSS bug will no longer be reproducible.  It might be a good use of your 
time to test that theory quickly with a build of NSS and NSPR from the 
tip of the trunk.

Comment 15

[Robert Relyea]

Comment on attachment 430880 [details] [diff] [review]
Add option argument validation to various tools

I'm cancelling unless the NSPR fix can be shown to be incorrrect. The only potential surviving elements of this patch is the addition of const for the Usage argument.

bob

Comment 16

[Elio Maldonado]

Attachment: shell script for verifying fix

This was submitted with the original bug report. I used it to verify the fix.

Comment 17

[Elio Maldonado]

(In reply to comment #14) I can verify that the NSPR fix is sufficient and this patch is not needed.