Last Comment Bug 337088 - Coverity 405, PK11_ParamToAlgid() in mozilla/security/nss/lib/pk11wrap/pk11mech.c
: Coverity 405, PK11_ParamToAlgid() in mozilla/security/nss/lib/pk11wrap/pk11m...
Status: RESOLVED FIXED
: coverity
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11
: All All
: P3 normal (vote)
: 3.12
Assigned To: Ryan Jones
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-07 21:57 PDT by Jon Smirl
Modified: 2007-05-04 00:03 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (970 bytes, patch)
2006-10-13 10:20 PDT, Ryan Jones
wtc: superreview+
Details | Diff | Splinter Review
Patch v2 (1.57 KB, patch)
2006-10-13 10:49 PDT, Ryan Jones
rrelyea: review+
Details | Diff | Splinter Review
Patch for checkin (2.30 KB, patch)
2006-11-15 12:17 PST, Ryan Jones
no flags Details | Diff | Splinter Review
Correct patch for checkin. (1.38 KB, patch)
2006-11-15 12:19 PST, Ryan Jones
no flags Details | Diff | Splinter Review
patch as checked in on trunk (1.51 KB, patch)
2007-05-02 20:34 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review

Description Jon Smirl 2006-05-07 21:57:06 PDT
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) 2006-05-10 18:52:33 PDT
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 2006-05-11 08:29:50 PDT
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 4 Julien Pierre 2006-06-20 16:32:05 PDT
Retargetting all P2s to 3.11.3 .
Comment 5 Ryan Jones 2006-10-13 10:20:46 PDT
Created attachment 242208 [details] [diff] [review]
Patch v1

Remove |rv = SECSuccess| per Jon Smirl's comment.
Comment 6 Wan-Teh Chang 2006-10-13 10:32:04 PDT
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;
Comment 7 Ryan Jones 2006-10-13 10:49:05 PDT
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>.
Comment 8 Robert Relyea 2006-11-15 12:06:48 PST
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 2006-11-15 12:17:52 PST
Created attachment 245686 [details] [diff] [review]
Patch for checkin

Patch v2 with the correct indenting as suggested by Robert.
Comment 10 Ryan Jones 2006-11-15 12:18:26 PST
Comment on attachment 245686 [details] [diff] [review]
Patch for checkin

Wrong file sorry!
Comment 11 Ryan Jones 2006-11-15 12:19:19 PST
Created attachment 245687 [details] [diff] [review]
Correct patch for checkin.

Patch v2 with the correct indenting as suggested by Robert.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-22 09:30:16 PST
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) 2007-05-02 20:34:43 PDT
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

Note You need to log in before you can comment on or make changes to this bug.