247739 - certutil program fails to read PQG files

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

function getpqgfromfile, at label found_match frees the string, and then
immediately passes the freed string to a function.  This causes the value
returned by this function to always be null.  

Thus, certutil CANNOT succesfully read PQG params from any file.

CERTUTIL_FileForRNG ignores failure to open file, and does not report error
message.  Caller (Certutil_GeneratePrivateKey) ignores return value.

CERTUTIL_GeneratePrivateKey doesn't check that dsaparams returned from
getpqgfromfile is not NULL, and crashes or generates invalid output.

SECU_GetpqgString doesn't check return vallue from fopen, and crashes if 
fopen returns NULL.  Should report error if file cannot be opened.

SECU_GetpqgString calls strlen twice on the same string.

SECU_GetpqgString only reads the first line of the file.  
But Base64 encoded files are typically multiple lines, separated by EOLs.  
Perhaps getpqgfromfile needs to call SECU_FileToItem instead.

A related issue is that makepqg generates an encoded PQGParams struct
and outputs it into a file in any of 3 formsts:  binary, hex, base64.
But none of these reasonable formats is the format that certutil expects 
to read in.  certutil expects to read in a text file containing a single
very-long line that contains one or more base64-encoded PQGParams, 
separated by commas (not spaces).  I don't know of ANY program anywhere, 
NSS or not, that generates such a file.  requiring the PQGParams file to 
be in this screwey format seems senseless, and ensures that certutil cannot
read the output of makepqg.

I think a workaround can be made as follows:
a) read the whole file in, not just the first line.
b) the AtoB decoder will ignore EOL characters, I think.
c) commas can continue to be used to separate succecssive PQGParams in the
file.  

I have a patch for SOME of these problems.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - adress all problems reported above

This patch fixes all the problems reported above.  
Among other fixes, it makes certutil able to read the base64-encoded output 
of makepqg.  That is, the output of makepqg -a is now acceptable input to 
certutil -S -q filename.  I believe I have preserved backwards compatibility 
with the old file format wanted by certutil, in case any program actually 
does produce that.  

I also plugged leaks in the function that genrates the key pair, and 
eliminated some unnecessary string duplication.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151259 [details] [diff] [review]
patch v1 - adress all problems reported above

Bob, please review

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151259 [details] [diff] [review]
patch v1 - adress all problems reported above

removing review request for obsolete patch.  
I found a couple bugs, and also realized there's no need to duplicate the const
dsaparams.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - with additional fixes

The previous patch failed to close the PQG file, except in error cases.
This patch fixes that, and also doesn't duplicate the constant PQG 
params.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151296 [details] [diff] [review]
patch v2 - with additional fixes

Bob, please review this one instead.

Comment 6

[Robert Relyea]

Comment on attachment 151296 [details] [diff] [review]
patch v2 - with additional fixes

r+ = relyea.

The following are option nits that could be changed.

Combining the prStatus check and the buf allocation check in getPQGString is a
little weird. I would prefer just having 2 explicit if's there.

It should be safe to declare the parameters for pqg_prime_bits, getPQGString,
getpqgfromfile, and CERTUTIL_FileForRNG as const.

Patch is fine even without the above changes.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 - incorporate Bob's "const" suggestions

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151414 [details] [diff] [review]
patch v3 - incorporate Bob's "const" suggestions

Vipul,	This patch attempts to plug a memory leak of an allocated ECparams
(among other changes).	Please review this patch for correctness of the code to
free the ECparams.  Please feel free to point out any other leaks or errors you
find.  :-)

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Vipul,	This patch attempts to plug a memory leak of an allocated ECparams
(among other changes).	Please review this patch for correctness of the code to
free the ECparams.  Please feel free to point out any other leaks or errors you
find.  :-)

Comment 10

[Vipul Gupta]

(In reply to comment #9)
> Vipul,  This patch attempts to plug a memory leak of an allocated ECparams
> (among other changes).  Please review this patch for correctness of the code to
> free the ECparams.  Please feel free to point out any other leaks or errors you
> find.  :-)

  The code to free the allocated ECparams look correct. I don't have any other
leaks/errors to point out ... but I'll keep looking :-)

Comment 11

[Vipul Gupta]

(In reply to comment #9)
> Vipul,  This patch attempts to plug a memory leak of an allocated ECparams
> (among other changes).  Please review this patch for correctness of the code to
> free the ECparams.  Please feel free to point out any other leaks or errors you
> find.  :-)

I have reviewed parts of this patch that deal with ECC and they look correct.
I would mark this patch r+ if I could.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151414 [details] [diff] [review]
patch v3 - incorporate Bob's "const" suggestions

Marking r+ in slight of Vipul's comment 11 

Thanks Vipul

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Fixed on trunk.
/cvsroot/mozilla/security/nss/cmd/certutil/keystuff.c,v  <--  keystuff.c
new revision: 1.15; previous revision: 1.14
Thanks for the quick reviews.