247738 - makepqg outputs invalid PQGParams

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

makepqg utility does not detect or report any syntax errors.  
Parsing stops silently at the first error, and the program 
goes on, attempting to use the arguments it has parsed that far.

makepqg doesn't check the values returned by outputPQGParams or 
outputPQGVerify, so it doesn't detect or report failures to encode.

makepqg utility program creates invalid DER-encoded output.
leading zeros are missing from P, Q, and G, making them all negative numbers. 

The cause is that PK11_PQG_ParamGenSeedLen leaves the type fields in the
secitems it returns uninitialized.  They need to be initialized to
siUnsignedInteger, so that the ASN.1 encoder will properly encode them.
This is the subject of bug 247737

But makepqg can and should ENSURE that these secitems are of 
type siUnsignedInteger before encoding them. 

patch for all these problems forthcoming

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - addresses all these bugs

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151247 [details] [diff] [review]
patch v1 - addresses all these bugs

Wan-Teh, please review

Comment 3

[Wan-Teh Chang]

Comment on attachment 151247 [details] [diff] [review]
patch v1 - addresses all these bugs

>+    /* workaround bug in PK11_PQG_ParamGenSeedLen */
>+    pqgParams->prime.type    = siUnsignedInteger;
>+    pqgParams->subPrime.type = siUnsignedInteger;
>+    pqgParams->base.type     = siUnsignedInteger;

Since the fix for bug 247737 is so simple, why don't
we just fix that bug?

Also, it seems better to work around the bug in
PK11_PQG_ParamGenSeedLen right after the call to
PK11_PQG_ParamGenSeedLen than in outputPQGParams.
Adding the above workaround to outputPQGParams
would prevent us from declaring pqgParams as
a const PQGParams *.

>+    if (status == PL_OPT_BAD) {
>+        Usage();
>+    }
>     if (rv != 0) {
> 	return rv;
>     }
>+    PL_DestroyOptState(optstate);

It looks nicer to call PL_DestroyOptState right
after the while loop.  This is just my personal
preference and doesn't really matter.

>     o = outputPQGParams(pqgParams, output_binary, output_raw, outFile);
>+    if (o) {
>+    	fprintf(stderr, "%s: failed to output PQG params.\n", progName);
>+    }
>     o = outputPQGVerify(pqgVerify, output_binary, output_raw, outFile);
>+    if (o) {
>+    	fprintf(stderr, "%s: failed to output PQG Verify.\n", progName);
>+    }

Since outputPQGParams and outputPQGVerify always
return 0, the error handling code you added will
be dead code.  We should also document what 'o'
means.	Is it really a SECStatus (0, -1) or a
PRBool (0, 1)?

I'm also wondering if we need to 'goto loser'
after the fprintf statements you added.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v2

This patch addresses the above review comments.
Now that bug 247737 is fixed, this patch no longer implements the
"workaround" for that bug.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151378 [details] [diff] [review]
Patch v2

Wan-Teh, please mark the previous patch r- in light of your review comments,
and then please review this patch.

Comment 6

[Wan-Teh Chang]

Comment on attachment 151378 [details] [diff] [review]
Patch v2

>-	PK11_PQG_GetPrimeFromParams(pqgParams, &item);
>+	rv = PK11_PQG_GetPrimeFromParams(pqgParams, &item);
>+	if (rv) {
>+	    SECU_PrintError(progName, "PK11_PQG_GetPrimeFromParams");
>+	    return rv;
>+	}
> 	SECU_PrintInteger(outFile, &item,    "Prime",    1);
> 	SECITEM_FreeItem(&item, PR_FALSE);

Should we call SECITEM_FreeItem(&item, PR_FALSE) before
the "return rv" statement?  (Same question for all other
similar code.)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3

In answer to the above question, 

> Should we call SECITEM_FreeItem(&item, PR_FALSE) before
> the "return rv" statement?  (Same question for all other
> similar code.)

No, I think not.  PK11_PQG_GetPrimeFromParams (and its kin) is really just
SECITEM_CopyItem.  If it returns SECFailure, it is because it failed to 
allocate the memory for item.  So, we needn't free it if it failed to 
allocate.  In the success path (non error path), item->data is freed 
before being reused.

But, while reviewing the code for your comments, I found 4 other leaks,
and fixed them.  There are 4 new lines in this patch v3, which are calls
to PORT_FreeArena and PORT_Free.  Hence this third version of the patch.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151413 [details] [diff] [review]
patch v3

Please review this patch.  You can use bugzilla to diff it against the previous
patch, and you will then easily see the 4 new Free lines.

Comment 9

[Wan-Teh Chang]

Comment on attachment 151413 [details] [diff] [review]
patch v3

r=wtc.

I think the two printf("\n") statements in outputPQGParams
should be fprintf(outFile, "\n").  Nelson, do you agree?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Don't know about the \n's.  They either belong on stderr or in the output file
where the ASCII encoded (not binary) PQGParams were written, or both.  

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3 part 2 - incrementatl patch for \n's

I checked those \n's, and found one is superfluous, and one should go onto 
the base-64 encoded output.  This patch implements those changes.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151562 [details] [diff] [review]
patch v3 part 2 - incrementatl patch for \n's

Wan-Teh, do you agree?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: incremental patch v2 to be applied on top of v3 above

I noticed that we're always writing unix \n's to the files, even on windows.
This patch fixes that.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151563 [details] [diff] [review]
incremental patch v2 to be applied on top of v3 above

Wan-Teh, please review

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151378 [details] [diff] [review]
Patch v2

removing review request for obsolete patch.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 151562 [details] [diff] [review]
patch v3 part 2 - incrementatl patch for \n's

cancelling review request for obsolete patch

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, do you agree my analysis in comment 7 ?

Comment 18

[Wan-Teh Chang]

I agree.  I missed that.

Comment 19

[Wan-Teh Chang]

Comment on attachment 151563 [details] [diff] [review]
incremental patch v2 to be applied on top of v3 above

>+    if (!rv && outFileName) {
>+	outFile = fopen(outFileName, output_binary ? "wb" : "w");
>+	if (!outFile) {
>+	    fprintf(stderr, "%s: unable to open \"%s\" for writing\n",
>+		    progName, optstate->value);
>+	    rv = -1;
>+	}
>+    }

optstate->value should be changed to outFileName.

You should call PORT_Free(outFileName) when you
are done with outFileName.

>     if (g) 
> 	rv = PK11_PQG_ParamGenSeedLen((unsigned)j, (unsigned)(g/8), 
> 	                         &pqgParams, &pqgVerify);
>     else 
> 	rv = PK11_PQG_ParamGen((unsigned)j, &pqgParams, &pqgVerify);
>+    /* below here, must go to loser */

Since PK11_PQG_DestroyParams and PK11_PQG_DestroyVerify
ignore a null argument, it is actually safe to go to
loser above that line, too.  In fact, the very first
"goto loser" statement may pass a null argument to these
destroy functions.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Attachment: incremental patch v3

This patch addresses the review feedback.

The comment about going to loser does not say that error paths cannot go to 
loser before that point, but only that error paths must do so after that point.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 152076 [details] [diff] [review]
incremental patch v3

One more time, please.

Comment 22

[Wan-Teh Chang]

Comment on attachment 152076 [details] [diff] [review]
incremental patch v3

r=wtc.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the quick reviews.   I Combined both patches into one checkin.  

/cvsroot/mozilla/security/nss/cmd/makepqg/makepqg.c,v  <--  makepqg.c
new revision: 1.6; previous revision: 1.5