makepqg outputs invalid PQGParams

RESOLVED FIXED in 3.10

Status

NSS
Tools
P2
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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
(Assignee)

Comment 1

14 years ago
Created attachment 151247 [details] [diff] [review]
patch v1 - addresses all these bugs
(Assignee)

Comment 2

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

Wan-Teh, please review
Attachment #151247 - Flags: review?(wchang0222)
(Assignee)

Updated

14 years ago
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → 3.10

Comment 3

14 years ago
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.
(Assignee)

Comment 4

14 years ago
Created attachment 151378 [details] [diff] [review]
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.
Attachment #151247 - Attachment is obsolete: true
(Assignee)

Comment 5

14 years ago
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.
Attachment #151378 - Flags: review?(wchang0222)

Updated

14 years ago
Attachment #151247 - Flags: review?(wchang0222) → review-

Comment 6

14 years ago
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.)
(Assignee)

Comment 7

14 years ago
Created attachment 151413 [details] [diff] [review]
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.
Attachment #151378 - Attachment is obsolete: true
(Assignee)

Comment 8

14 years ago
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.
Attachment #151413 - Flags: review?(wchang0222)

Comment 9

14 years ago
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?
Attachment #151413 - Flags: review?(wchang0222) → review+
(Assignee)

Comment 10

14 years ago
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.  
Status: NEW → ASSIGNED
(Assignee)

Comment 11

14 years ago
Created attachment 151562 [details] [diff] [review]
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.
(Assignee)

Comment 12

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

Wan-Teh, do you agree?
Attachment #151562 - Flags: review?
(Assignee)

Comment 13

14 years ago
Created attachment 151563 [details] [diff] [review]
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.
Attachment #151562 - Attachment is obsolete: true
(Assignee)

Comment 14

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

Wan-Teh, please review
Attachment #151563 - Flags: review?(wchang0222)
(Assignee)

Comment 15

14 years ago
Comment on attachment 151378 [details] [diff] [review]
Patch v2

removing review request for obsolete patch.
Attachment #151378 - Flags: review?(wchang0222)
(Assignee)

Comment 16

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

cancelling review request for obsolete patch
Attachment #151562 - Flags: review?
(Assignee)

Comment 17

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

Comment 18

14 years ago
I agree.  I missed that.
(Assignee)

Updated

14 years ago
Whiteboard: incremental patch awaiting review

Comment 19

14 years ago
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.
Attachment #151563 - Flags: review?(wchang0222) → review-
(Assignee)

Comment 20

14 years ago
Created attachment 152076 [details] [diff] [review]
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.
Attachment #151563 - Attachment is obsolete: true
(Assignee)

Comment 21

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

One more time, please.
Attachment #152076 - Flags: review?(wchang0222)

Updated

14 years ago
Attachment #152076 - Flags: review?(wchang0222) → review+

Comment 22

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

r=wtc.
(Assignee)

Comment 23

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Whiteboard: incremental patch awaiting review
You need to log in before you can comment on or make changes to this bug.