Closed Bug 247739 Opened 20 years ago Closed 20 years ago

certutil program fails to read PQG files

Categories

(NSS :: Tools, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: nelson)

Details

Attachments

(1 file, 2 obsolete files)

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.
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → 3.10
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 on attachment 151259 [details] [diff] [review]
patch v1 - adress all problems reported above

Bob, please review
Attachment #151259 - Flags: review?(rrelyea0264)
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.
Attachment #151259 - Attachment is obsolete: true
Attachment #151259 - Flags: review?(rrelyea0264)
Attached patch patch v2 - with additional fixes (obsolete) — Splinter Review
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 on attachment 151296 [details] [diff] [review]
patch v2 - with additional fixes

Bob, please review this one instead.
Attachment #151296 - Flags: review?(rrelyea0264)
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.
Attachment #151296 - Flags: review?(rrelyea0264) → review+
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.  :-)
Attachment #151414 - Flags: review?(vipul.gupta)
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.  :-)
Status: NEW → ASSIGNED
(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 :-)

(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 on attachment 151414 [details] [diff] [review]
patch v3 - incorporate Bob's "const" suggestions

Marking r+ in slight of Vipul's comment 11 

Thanks Vipul
Attachment #151414 - Flags: review?(vipul.gupta) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: