Closed
Bug 247738
Opened 20 years ago
Closed 20 years ago
makepqg outputs invalid PQGParams
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.10
People
(Reporter: nelson, Assigned: nelson)
References
Details
Attachments
(2 files, 4 obsolete files)
7.90 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
Assignee | ||
Comment 2•20 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•20 years ago
|
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → 3.10
Comment 3•20 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•20 years ago
|
||
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•20 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•20 years ago
|
Attachment #151247 -
Flags: review?(wchang0222) → review-
Comment 6•20 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•20 years ago
|
||
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•20 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•20 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•20 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•20 years ago
|
||
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•20 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•20 years ago
|
||
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•20 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•20 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•20 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•20 years ago
|
||
Wan-Teh, do you agree my analysis in comment 7 ?
Comment 18•20 years ago
|
||
I agree. I missed that.
Assignee | ||
Updated•20 years ago
|
Whiteboard: incremental patch awaiting review
Comment 19•20 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•20 years ago
|
||
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•20 years ago
|
||
Comment on attachment 152076 [details] [diff] [review] incremental patch v3 One more time, please.
Attachment #152076 -
Flags: review?(wchang0222)
Updated•20 years ago
|
Attachment #152076 -
Flags: review?(wchang0222) → review+
Comment 22•20 years ago
|
||
Comment on attachment 152076 [details] [diff] [review] incremental patch v3 r=wtc.
Assignee | ||
Comment 23•20 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
Closed: 20 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.
Description
•