136804 - certutil -A crashes unless -i argument is given

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

If you run the certutil -A command to add a cert to a db, and you 
don't specify -i to open an input file, then it crashes because inFile
is NULL instead of being PR_STDIN.  I think there used to be some code
that set infile to PR_STDIN if no -i argument was given, but now certutil
doesn't seem to contain code to do that.  

I reproduced this using these steps:
1. create a new certDB in directory ./tmp
       certutil -N -d tmp
2. attempt to add a cert to the db using an RFC 1113 format file on stdin.
       certutil -A -d tmp -n foo -a -t "" < /tmp/foocert.txt
result: segv

stack at crash is:  

PR_GetOpenFileInfo(PRFileDesc * 0x00000000, PRFileInfo * 0x0012fe10) line 166 + 
11 bytes
SECU_FileToItem(SECItemStr * 0x0012fe40, PRFileDesc * 0x00000000) line 545 + 14 
bytes
SECU_ReadDERFromFile(SECItemStr * 0x0012fe80, PRFileDesc * 0x00000000, int 
0x00000001) line 626 + 13 bytes
AddCert(PK11SlotInfoStr * 0x0051fe48, NSSTrustDomainStr * 0x00519430, char * 
0x00494538, char * 0x004944b9, PRFileDesc * 0x00000000, int 0x00000001, int 
0x00000000, void * 0x0012fef8) line 285 + 17 bytes
main(int 0x00000009, char * * 0x00494440) line 2808 + 49 bytes


You can clearly see all the null PRFileDesc pointers there.  
Using -i works around this.

Comment 1

[Wan-Teh Chang]

It should be easy to fix the crash if -A is specified without -i.
crlutil has a similar logic (in cmd/crlutil/crlutil.c) to ensure
that -I must be specified with -i:
    if (importCRL && !inFile) Usage (progName);

As for whether certutil should read from PR_STDIN if no input
file is specified, I'll let you decide.  I checked NSS 3.3.2
and found that it does not set inFile to PR_STDIN either if
no -i argument is given.  So the code to do that may have
been lost for several releases or may not have existed before. 

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The documented behavior of cerutil -A is that it reads from stdin 
unless the -i argument is given.  Therefore, the correct behavior 
would be to set inFile to PR_STDIN if -i is not specified.  

A trivial one-line fix would be to initialize inFile with PR_STDIN
rather than with NULL.

Comment 3

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 4

[Wan-Teh Chang]

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

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch

This is the fix that Nelson suggested in comment #2.
I added two assertions to catch errors (inFile should
be the certreqfile and the certfile at those two places).

Comment 6

[Wan-Teh Chang]

Fix checked in.