Closed
Bug 1054069
Opened 10 years ago
Closed 10 years ago
Address coverity scan reported flaws
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.19
People
(Reporter: elio.maldonado.batiz, Assigned: elio.maldonado.batiz)
References
Details
Attachments
(1 file, 5 obsolete files)
7.51 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Priority: -- → P2
Target Milestone: --- → 3.17.1
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
This patch was applied downstream for nss-3.16.1 sources. I'll provide a version adapted to the current sources next.
Assignee | ||
Comment 2•10 years ago
|
||
Patch adapted to the current sources.
Assignee: nobody → emaldona
Attachment #8473326 -
Attachment is obsolete: true
Attachment #8473327 -
Flags: review?(wtc)
Comment 4•10 years ago
|
||
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-
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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-
Assignee | ||
Comment 7•10 years ago
|
||
(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.
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Target Milestone: 3.17.1 → 3.18
Assignee | ||
Comment 9•10 years ago
|
||
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)
Updated•10 years ago
|
Target Milestone: 3.18 → 3.18.1
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8571053 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: 3.18.1 → 3.19
Comment 13•10 years ago
|
||
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.
Description
•