Closed Bug 1054069 Opened 10 years ago Closed 10 years ago

Address coverity scan reported flaws

Categories

(NSS :: Libraries, defect, P2)

3.16.1
x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: elio.maldonado.batiz, Assigned: elio.maldonado.batiz)

References

Details

Attachments

(1 file, 5 obsolete files)

Coverity scans done at Red Hat report secveral errorss on: nss-3.16.1-2.el6 compared to nss-3.15.1-15.el6. Here is the reprt with my own comments added interspersed 1. Defect type: CHECKED_RETURN 19. nss-3.16.1/nss/lib/smime/cmsmessage.c:50:unchecked_value – No check of the return value of "NSS_CMSContentInfo_Private_Init(&cmsg->contentInfo)". Yes, defect. Should check return and either return NUll or have an assertion 2. Defect type: NULL_RETURNS 12. nss-3.16.1/nss/cmd/modutil/install.c:124:dereference – Dereferencing a null pointer "new_this". yes, defect. 3. Defect type: CHECKED_RETURN 74. nss-3.16.1/nss/cmd/checkcert/checkcert.c:417:unchecked_value – No check of the return value of "SECOID_SetAlgorithmID_Util(arena, &rsaEncryption, SEC_OID_PKCS1_RSA_ENCRYPTION, NULL)". Yes, file a bug 4. Defect type: CHECKED_RETURN 74. nss-3.16.1/nss/cmd/checkcert/checkcert.c:414:unchecked_value – No check of the return value of "SECOID_SetAlgorithmID_Util(arena, &sha1WithRSAEncryption, SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION, NULL)". Yes, file a bug 5. Defect type: CHECKED_RETURN 74. nss-3.16.1/nss/cmd/checkcert/checkcert.c:411:unchecked_value – No check of the return value of "SECOID_SetAlgorithmID_Util(arena, &md2WithRSAEncryption, SEC_OID_PKCS1_MD2_WITH_RSA_ENCRYPTION, NULL)". Yes, file a bug 6. Defect type: CHECKED_RETURN 74. nss-3.16.1/nss/cmd/checkcert/checkcert.c:408:unchecked_value – No check of the return value of "SECOID_SetAlgorithmID_Util(arena, &md5WithRSAEncryption, SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION, NULL)". Yes, file a bug 7. Defect type: CHECKED_RETURN 13. nss-3.16.1/nss/cmd/certutil/certext.c:1661:unchecked_value – No check of the return value of "PrintChoicesAndGetAnswer("Enter access method type for Authority Information Access extension:\n\t1 - CA Issuers\n\t2 - OCSP\n\tAnyother number to finish\n\tChoice", buffer, 512)". Yes, file a bus 8. Defect type: CHECKED_RETURN 15. nss-3.16.1/nss/cmd/certutil/certext.c:822:unchecked_value – No check of the return value of "PrintChoicesAndGetAnswer("Type of Name Constraint?\n\t1 - permitted\n\t2 - excluded\n\tAnyother number to finish\n\tChoice", buffer, 512)". Yes, file a bug for rhel-6.6 and upstream.
Priority: -- → P2
Target Milestone: --- → 3.17.1
This patch was applied downstream for nss-3.16.1 sources. I'll provide a version adapted to the current sources next.
Patch adapted to the current sources.
Assignee: nobody → emaldona
Attachment #8473326 - Attachment is obsolete: true
Attachment #8473327 - Flags: review?(wtc)
Comment on attachment 8473327 [details] [diff] [review] Addresses most of the defects reported by covscan - V2 Review of attachment 8473327 [details] [diff] [review]: ----------------------------------------------------------------- Elio: thanks a lot for the patch. Unfortunately this patch needs work. ::: cmd/certutil/certext.c @@ +992,4 @@ > "\t1 - permitted\n\t2 - excluded\n\tAny" > "other number to finish\n\tChoice", > buffer, sizeof(buffer)); > + if (status != SECSuccess) { We can just use the 'rv' variable declared in the outer scope, and just use the break statement: rv = PrintChoicesAndGetAnswer("Type of Name Constraint?\n" ... if (rv != SECSuccess) { break; } Then we don't need the 'status' local variable. Another solution is: if (PrintChoicesAndGetAnswer("Type of Name Constraint?\n" ... buffer, sizeof(buffer)) != SECSuccess) { GEN_BREAK(SECFailure); } @@ +1812,5 @@ > return SECFailure; > } > > do { > + SECStatus status; Same here. ::: cmd/checkcert/checkcert.c @@ +243,5 @@ > int invalid=0; > char *inFileName = NULL, *issuerCertFileName = NULL; > PLOptState *optstate; > PLOptStatus status; > + SECStatus status1; At first I thought 'status1' is too similar to 'status'. Upon looking at the code closer, I found that we are supposed to use the existing 'rv' variable, even though it is declared as the 'int' type. I suggest you declare 'rv' as SECStatus and use it. @@ +410,2 @@ > SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION, NULL); > + PORT_Assert(status1 == SECSuccess); Please add proper error handling. Imitate the existing code. ::: cmd/modutil/install.c @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "install.h" > #include "install-ds.h" > +#include "secerr.h" Nit: we can just use PR_OUT_OF_MEMORY_ERROR, then we don't need to include "secerr.h". But see my suggestion below. Maybe we don't need to call PORT_SetError at all. @@ +120,5 @@ > > StringNode* StringNode_new() > { > StringNode* new_this; > new_this = (StringNode*)malloc(sizeof(StringNode)); 1. Please change this malloc call to PR_Malloc. Alternatively, change StringNode_delete to call free(). The allocation and deallocation functions should be matched. 2. I think it would be sufficient to just add an assertion here that new_this != NULL. The reason is that the caller of StringNode_new is not checking a null return value either. Alternatively, we can fix the caller of StringNode_new to also handle a null return. But that caller (StringList_Append) returns void, so it can't report the error up the call chain. ::: lib/smime/cmsmessage.c @@ +49,5 @@ > return NULL; > } > + status = NSS_CMSContentInfo_Private_Init(&(cmsg->contentInfo)); > + if (status != SECSuccess) { > + PORT_Assert(status == SECSuccess); Please indent this line. Also, I think you should just call PORT_Assert(0). Is it difficult to handle this failure properly?
Attachment #8473327 - Flags: review?(wtc) → review-
Attached patch Address review requests (obsolete) — Splinter Review
Not sure of having properly handled failure in that last part of the patch.
Attachment #8473327 - Attachment is obsolete: true
Attachment #8533223 - Flags: review?(wtc)
Comment on attachment 8533223 [details] [diff] [review] Address review requests Review of attachment 8533223 [details] [diff] [review]: ----------------------------------------------------------------- Elio: thank you for the patch. Please fix the problems I spotted and upload a new patch. ::: cmd/checkcert/checkcert.c @@ +10,5 @@ > > /* maximum supported modulus length in bits (indicate problem if over this) */ > #define MAX_MODULUS (1024) > > +#define CHECKERROR(rv, ln) \ We should either use the new CHECKERROR macro to check all function call errors, or not add the macro. Consistency is good. I would probably add custom error messages for the SECOID_SetAlgorithmID failures. @@ +15,5 @@ > + if (rv) { \ > + PRErrorCode prerror = PR_GetError(); \ > + fprintf(stderr, "%s: ERR %d (%s) at line %d.\n", progName, \ > + prerror, PORT_ErrorToString(prerror), ln); \ > + exit(-1); \ Please pass 1 to exit(). The argument to exit() must be in the range [0, 255] inclusive. ::: cmd/modutil/install.c @@ +120,5 @@ > StringNode* StringNode_new() > { > StringNode* new_this; > + new_this = (StringNode*)PR_Malloc(sizeof(StringNode)); > + PORT_Assert(new_this == NULL); IMPORTANT: this should be PORT_Assert(new_this != NULL), right? ::: lib/smime/cmsmessage.c @@ +56,5 @@ > + if (mark) { > + PORT_ArenaUnmark(poolp, mark); > + PORT_ZFree(cmsg, sizeof(NSSCMSMessage)); > + return NULL; > + } 1. Lines 53-60 seem to be over-indented. 2. IMPORTANT: lines 56-60 should read: if (!poolp_is_ours) { if (mark) { PORT_ArenaRelease(poolp, mark); } } else PORT_FreeArena(poolp, PR_FALSE); return NULL; The PORT_ZFree call you added on line 58 should be unnecessary because at this point, |cmsg| doesn't contain any sensitive data. To avoid duplicating code, we can do something like this: cmsg = (NSSCMSMessage *)PORT_ArenaZAlloc (poolp, sizeof(NSSCMSMessage)); if (cmsg == NULL || NSS_CMSContentInfo_Private_Init(...) != SECSuccess) { if (!poolp_is_ours) { if (mark) { PORT_ArenaRelease(poolp, mark); } } else PORT_FreeArena(poolp, PR_FALSE); return NULL; }
Attachment #8533223 - Flags: review?(wtc) → review-
(In reply to Wan-Teh Chang from comment #6) > Comment on attachment 8533223 [details] [diff] [review] > Address review requests > > Review of attachment 8533223 [details] [diff] [review]: > ----------------------------------------------------------------- > > Elio: thank you for the patch. Please fix the problems I spotted > and upload a new patch. > > ::: cmd/checkcert/checkcert.c > @@ +10,5 @@ > > > > /* maximum supported modulus length in bits (indicate problem if over this) */ > > #define MAX_MODULUS (1024) > > > > +#define CHECKERROR(rv, ln) \ > > We should either use the new CHECKERROR macro to check all > function call errors, or not add the macro. Consistency is > good. > At thought about that but the problem was that CHECKERROR was inside cmd/btest.c wch is inacceble. The solution would have been to move it to cmd/lib/basicutil.h to make it accessible to other tools. At the time I wanted to limit the scope of the changes and that's why ended copying it. I will move it to the there. > I would probably add custom error messages for the > SECOID_SetAlgorithmID failures. > > @@ +15,5 @@ > > + if (rv) { \ > > + PRErrorCode prerror = PR_GetError(); \ > > + fprintf(stderr, "%s: ERR %d (%s) at line %d.\n", progName, \ > > + prerror, PORT_ErrorToString(prerror), ln); \ > > + exit(-1); \ > > Please pass 1 to exit(). The argument to exit() must be in > the range [0, 255] inclusive. Will do. > > ::: cmd/modutil/install.c > @@ +120,5 @@ > > StringNode* StringNode_new() > > { > > StringNode* new_this; > > + new_this = (StringNode*)PR_Malloc(sizeof(StringNode)); > > + PORT_Assert(new_this == NULL); > > IMPORTANT: this should be PORT_Assert(new_this != NULL), right? Yes. > > ::: lib/smime/cmsmessage.c > @@ +56,5 @@ > > + if (mark) { > > + PORT_ArenaUnmark(poolp, mark); > > + PORT_ZFree(cmsg, sizeof(NSSCMSMessage)); > > + return NULL; > > + } > > 1. Lines 53-60 seem to be over-indented. Indeed, lot in that file is over-indented as it uses tabs. I'll fix that, at least in the functions I am working on. I should probably file a separte bug to clean it up in teh whole file. > > 2. IMPORTANT: lines 56-60 should read: > > if (!poolp_is_ours) { > if (mark) { > PORT_ArenaRelease(poolp, mark); > } > } else > PORT_FreeArena(poolp, PR_FALSE); > return NULL; > > The PORT_ZFree call you added on line 58 should be unnecessary > because at this point, |cmsg| doesn't contain any sensitive > data. > > To avoid duplicating code, we can do something like this: > > cmsg = (NSSCMSMessage *)PORT_ArenaZAlloc (poolp, sizeof(NSSCMSMessage)); > if (cmsg == NULL || NSS_CMSContentInfo_Private_Init(...) != SECSuccess) { > if (!poolp_is_ours) { > if (mark) { > PORT_ArenaRelease(poolp, mark); > } > } else > PORT_FreeArena(poolp, PR_FALSE); > return NULL; > } Yes, I like your suggestion.
Attached patch Address review requests - V3 (obsolete) — Splinter Review
I should point out that your suggestion > I would probably add custom error messages for the > SECOID_SetAlgorithmID failures. > is something I didn't quite understand and wasn't addressed in this patch.
Attachment #8533223 - Attachment is obsolete: true
Attachment #8552035 - Flags: review?(wtc)
Target Milestone: 3.17.1 → 3.18
Attached patch Address review requests - V4 (obsolete) — Splinter Review
Like V3 but added custom error messages for the SECOID_SetAlgorithmID failures.
Attachment #8552035 - Attachment is obsolete: true
Attachment #8552035 - Flags: review?(wtc)
Attachment #8571053 - Flags: review?(wtc)
Target Milestone: 3.18 → 3.18.1
Comment on attachment 8571053 [details] [diff] [review] Address review requests - V4 Review of attachment 8571053 [details] [diff] [review]: ----------------------------------------------------------------- Hi Elio, Thank you for the new patch. I suggest some changes. You can commit this CL after making the suggested changes. Please attach a new patch before you check it in. ::: cmd/bltest/blapitest.c @@ +395,5 @@ > /* read and convert params */ > key->ecParams.arena = arena; > key_from_filedata(arena, &key->ecParams.DEREncoding, 0, 1, filedata); > rv = SECOID_Init(); > + CHECKERROR(rv, __LINE__, NULL); I suggest reverting all changes to this file and basicutil.h. ::: cmd/checkcert/checkcert.c @@ +410,2 @@ > SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION, NULL); > + CHECKERROR(rv, __LINE__, "Algorithm check failed for SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION"); In this file, I suggest we imitate the error handling in the existing code and not introduce the use of the CHECKERROR macro. In general I think consistency is important. It would be inconsistent if we only used CHECKERROR to handle the failure of these four SECOID_SetAlgorithmID calls. Also, the error messages are wrong. These SECOID_SetAlgorithmID calls don't check the algorithms. The algorithm check is performed by the three SECOID_CompareAlgorithmID calls below. I suggest we handle the failure of these SECOID_SetAlgorithmID calls as follows (the error message needs to be updated for the other three calls): if (rv) { fprintf(stderr, "%s: failed to set algorithm ID for SEC_OID_PKCS1_MD5_WITH_RSA_ENCRYPTION.\n", progName); exit(1); } ::: cmd/lib/basicutil.h @@ +22,5 @@ > #else > typedef int (*SECU_PPFunc)(FILE *out, SECItem *item, char *msg, int level); > #endif > > +#define CHECKERROR(rv, ln, msg) \ I suggest we move this macro back to the blapitest.c file and restrict its use in that file. ::: cmd/modutil/install.c @@ +120,5 @@ > StringNode* StringNode_new() > { > StringNode* new_this; > + new_this = (StringNode*)PR_Malloc(sizeof(StringNode)); > + PORT_Assert(new_this == NULL); BUG: we should assert new_this != NULL. @@ +122,5 @@ > StringNode* new_this; > + new_this = (StringNode*)PR_Malloc(sizeof(StringNode)); > + PORT_Assert(new_this == NULL); > + new_this->str=NULL; > + new_this->next=NULL; Nit: In these two lines, please add spaces before and after the equal sign '='.
Attachment #8571053 - Flags: review?(wtc) → review+
Attachment #8571053 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 3.18.1 → 3.19
Comment on attachment 8593735 [details] [diff] [review] implements wtc review requests - V5 Review of attachment 8593735 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. Thanks.
Attachment #8593735 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: