The default bug view has changed. See this FAQ.

Coverity 405, PK11_ParamToAlgid() in mozilla/security/nss/lib/pk11wrap/pk11mech.c

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P3
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Jon Smirl, Assigned: Ryan Jones)

Tracking

({coverity})

3.11
3.12
coverity

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 2

11 years ago
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.
(Reporter)

Comment 3

11 years ago
Pointer to the source:
http://lxr.mozilla.org/seamonkey/source/security/nss/lib/pk11wrap/pk11mech.c#1435

Comment 4

11 years ago
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
(Assignee)

Comment 5

11 years ago
Created attachment 242208 [details] [diff] [review]
Patch v1

Remove |rv = SECSuccess| per Jon Smirl's comment.
Attachment #242208 - Flags: superreview?(wtchang)
Attachment #242208 - Flags: review?(kaie)

Comment 6

11 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

11 years ago
Created attachment 242209 [details] [diff] [review]
Patch v2

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 8

11 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

11 years ago
Created attachment 245686 [details] [diff] [review]
Patch for checkin

Patch v2 with the correct indenting as suggested by Robert.
Attachment #242209 - Attachment is obsolete: true
(Assignee)

Comment 10

11 years ago
Comment on attachment 245686 [details] [diff] [review]
Patch for checkin

Wrong file sorry!
Attachment #245686 - Attachment is obsolete: true
(Assignee)

Comment 11

11 years ago
Created attachment 245687 [details] [diff] [review]
Correct patch for checkin.

Patch v2 with the correct indenting as suggested by Robert.
Attachment #242208 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #245687 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #245687 - Attachment is obsolete: false
(Assignee)

Updated

11 years ago
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.
Created attachment 263552 [details] [diff] [review]
patch as checked in on trunk

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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.3 → 3.12

Updated

10 years ago
Whiteboard: FIPS [checkin needed] → FIPS

Updated

10 years ago
Whiteboard: FIPS
You need to log in before you can comment on or make changes to this bug.