Closed
Bug 337088
Opened 18 years ago
Closed 17 years ago
Coverity 405, PK11_ParamToAlgid() in mozilla/security/nss/lib/pk11wrap/pk11mech.c
Categories
(NSS :: Libraries, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12
People
(Reporter: jonsmirl, Assigned: sciguyryan)
Details
(Keywords: coverity)
Attachments
(1 file, 4 obsolete files)
1.51 KB,
patch
|
Details | Diff | Splinter Review |
This looks like a bug. At the top of the routine rc is set to SECFailure then two lines later it is set to SECSuccess. Setting it to SECSuccess makes the whole routine do nothing. Someone who knows more will need to decide if it is exploitable. ECStatus PK11_ParamToAlgid(SECOidTag algTag, SECItem *param, PRArenaPool *arena, SECAlgorithmID *algid) { CK_RC2_CBC_PARAMS *rc2_params; sec_rc2cbcParameter rc2; CK_RC5_CBC_PARAMS *rc5_params; sec_rc5cbcParameter rc5; CK_MECHANISM_TYPE type = PK11_AlgtagToMechanism(algTag); SECItem *newParams = NULL; SECStatus rv = SECFailure; unsigned long rc2version; rv = SECSuccess; switch (type) { case CKM_RC4: case CKM_AES_ECB: case CKM_DES_ECB: case CKM_DES3_ECB:
Comment 1•18 years ago
|
||
There does seem to be at least one case in this switch that will do the wrong thing, but I don't think this is in any sense exploitable.
Group: security
Priority: -- → P3
Did you notice that it only returns SECSuccess no matter what the select decides? Is that correct? Coverity flagged all of the rv = SECSuccess; in the cases as dead code because of this. At the top: SECStatus rv = SECFailure; unsigned long rc2version; rv = SECSuccess; The tabs are different on the rv = SECSuccess; so it was probably added later. The routine makes more sense if that line is removed.
Pointer to the source: http://lxr.mozilla.org/seamonkey/source/security/nss/lib/pk11wrap/pk11mech.c#1435
Assignee | ||
Comment 5•18 years ago
|
||
Remove |rv = SECSuccess| per Jon Smirl's comment.
Attachment #242208 -
Flags: superreview?(wtchang)
Attachment #242208 -
Flags: review?(kaie)
Comment 6•18 years ago
|
||
Comment on attachment 242208 [details] [diff] [review] Patch v1 Please submit a new patch and ask rrelyea@redhat.com to review it. Bob Relyea is most likely the author of this function. I took the opportunity to review the whole function. In the last case of the switch statement, we need to check newParams for NULL. if (newParams == NULL) break;
Attachment #242208 -
Flags: superreview?(wtchang)
Attachment #242208 -
Flags: superreview+
Attachment #242208 -
Flags: review?(kaie)
Assignee | ||
Comment 7•18 years ago
|
||
Patch changed too Wan-Teh Chang's comment and requesting review from Bob Relyea <rrelyea@redhat.com>.
Assignee: nobody → sciguyryan+bugzilla
Status: NEW → ASSIGNED
Attachment #242209 -
Flags: review?(rrelyea)
Updated•18 years ago
|
Whiteboard: FIPS
Comment 8•18 years ago
|
||
Comment on attachment 242209 [details] [diff] [review] Patch v2 r+ =relyea if you fix the tab level for the if statement. NSS uses TS=8 with a 4 character indent.
Attachment #242209 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 9•18 years ago
|
||
Patch v2 with the correct indenting as suggested by Robert.
Attachment #242209 -
Attachment is obsolete: true
Assignee | ||
Comment 10•18 years ago
|
||
Comment on attachment 245686 [details] [diff] [review] Patch for checkin Wrong file sorry!
Attachment #245686 -
Attachment is obsolete: true
Assignee | ||
Comment 11•18 years ago
|
||
Patch v2 with the correct indenting as suggested by Robert.
Attachment #242208 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #245687 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #245687 -
Attachment is obsolete: false
Assignee | ||
Updated•18 years ago
|
Whiteboard: FIPS → FIPS [checkin needed]
Comment 12•18 years ago
|
||
I'm not really familiar with the NSS checkin rules, so I don't know whether this has sufficient review for landing on the trunk, or whether it should also be landed on an NSS release branch. It's probably a good idea to get an NSS peer to land this.
Comment 13•17 years ago
|
||
The patch had bit rotted slightly due to checkin of camellia ciphers. This is the patch I committed. Checking in pk11mech.c; new revision: 1.7; previous revision: 1.6
Attachment #245687 -
Attachment is obsolete: true
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.3 → 3.12
Updated•17 years ago
|
Whiteboard: FIPS [checkin needed] → FIPS
Updated•17 years ago
|
Whiteboard: FIPS
You need to log in
before you can comment on or make changes to this bug.
Description
•