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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Jon Smirl]

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

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 2

[Jon Smirl]

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.

Comment 3

[Jon Smirl]

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

Comment 4

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Remove |rv = SECSuccess| per Jon Smirl's comment.

Comment 6

[Wan-Teh Chang]

Comment on attachment 242208 [details] [diff] [review]
Patch v1

Please submit a new patch and ask [e-mail] 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;

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch changed too Wan-Teh Chang's comment and requesting review from Bob Relyea <[e-mail]>.

Comment 8

[Robert Relyea]

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.

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch for checkin

Patch v2 with the correct indenting as suggested by Robert.

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

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

Wrong file sorry!

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Correct patch for checkin.

Patch v2 with the correct indenting as suggested by Robert.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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