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)

3.11
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jonsmirl, Assigned: sciguyryan)

Details

(Keywords: coverity)

Attachments

(1 file, 4 obsolete files)

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:
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.
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
Attached patch Patch v1 (obsolete) — Splinter Review
Remove |rv = SECSuccess| per Jon Smirl's comment.
Attachment #242208 - Flags: superreview?(wtchang)
Attachment #242208 - Flags: review?(kaie)
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)
Attached patch Patch v2 (obsolete) — Splinter Review
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)
Whiteboard: FIPS
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+
Attached patch Patch for checkin (obsolete) — Splinter Review
Patch v2 with the correct indenting as suggested by Robert.
Attachment #242209 - Attachment is obsolete: true
Comment on attachment 245686 [details] [diff] [review]
Patch for checkin

Wrong file sorry!
Attachment #245686 - Attachment is obsolete: true
Attached patch Correct patch for checkin. (obsolete) — Splinter Review
Patch v2 with the correct indenting as suggested by Robert.
Attachment #242208 - Attachment is obsolete: true
Attachment #245687 - Attachment is obsolete: true
Attachment #245687 - Attachment is obsolete: false
Whiteboard: FIPS → FIPS [checkin needed]
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.
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.3 → 3.12
Whiteboard: FIPS [checkin needed] → FIPS
Whiteboard: FIPS
You need to log in before you can comment on or make changes to this bug.