Bug 341124: Coverity 522 Callers of mp_unsigned_octet_size SOFTOKEN_3_13_BRANCH
authornelson%bolyard.com
Mon, 15 Mar 2010 07:03:23 +0000
branchSOFTOKEN_3_13_BRANCH
changeset 9585 b586b8af68a2ae833b9eeac477c8d8c6376faeb5
parent 9581 f4eca79c40ab8a55333fecd1dee2c66900317dba
child 9589 ad61fbb2d5b629d12595e167e6f619410c73f363
push idunknown
push userunknown
push dateunknown
bugs341124
Bug 341124: Coverity 522 Callers of mp_unsigned_octet_size don't check for negative return value Patch contributed by Timeless <timeless@mozdev.org>, r=nelson
security/nss/lib/freebl/dh.c
security/nss/lib/freebl/mpi/mpi.c
security/nss/lib/freebl/secmpi.h
--- a/security/nss/lib/freebl/dh.c
+++ b/security/nss/lib/freebl/dh.c
@@ -214,17 +214,18 @@ SECStatus
 DH_Derive(SECItem *publicValue, 
           SECItem *prime, 
           SECItem *privateValue, 
           SECItem *derivedSecret, 
           unsigned int maxOutBytes)
 {
     mp_int p, Xa, Yb, ZZ;
     mp_err err = MP_OKAY;
-    unsigned int len = 0, nb;
+    int len = 0;
+    unsigned int nb;
     unsigned char *secret = NULL;
     if (!publicValue || !prime || !privateValue || !derivedSecret) {
 	PORT_SetError(SEC_ERROR_INVALID_ARGS);
 	return SECFailure;
     }
     memset(derivedSecret, 0, sizeof *derivedSecret);
     MP_DIGITS(&p)  = 0;
     MP_DIGITS(&Xa) = 0;
@@ -236,16 +237,20 @@ DH_Derive(SECItem *publicValue,
     CHECK_MPI_OK( mp_init(&ZZ) );
     SECITEM_TO_MPINT(*publicValue,  &Yb);
     SECITEM_TO_MPINT(*privateValue, &Xa);
     SECITEM_TO_MPINT(*prime,        &p);
     /* ZZ = (Yb)**Xa mod p */
     CHECK_MPI_OK( mp_exptmod(&Yb, &Xa, &p, &ZZ) );
     /* number of bytes in the derived secret */
     len = mp_unsigned_octet_size(&ZZ);
+    if (len <= 0) {
+        err = MP_BADARG;
+        goto cleanup;
+    }
     /* allocate a buffer which can hold the entire derived secret. */
     secret = PORT_Alloc(len);
     /* grab the derived secret */
     err = mp_to_unsigned_octets(&ZZ, secret, len);
     if (err >= 0) err = MP_OKAY;
     /* Take minimum of bytes requested and bytes in derived secret,
     ** if maxOutBytes is 0 take all of the bytes from the derived secret.
     */
--- a/security/nss/lib/freebl/mpi/mpi.c
+++ b/security/nss/lib/freebl/mpi/mpi.c
@@ -4742,17 +4742,17 @@ mp_err
 mp_to_unsigned_octets(const mp_int *mp, unsigned char *str, mp_size maxlen)
 {
   int  ix, pos = 0;
   int  bytes;
 
   ARGCHK(mp != NULL && str != NULL && !SIGN(mp), MP_BADARG);
 
   bytes = mp_unsigned_octet_size(mp);
-  ARGCHK(bytes <= maxlen, MP_BADARG);
+  ARGCHK(bytes >= 0 && bytes <= maxlen, MP_BADARG);
 
   /* Iterate over each digit... */
   for(ix = USED(mp) - 1; ix >= 0; ix--) {
     mp_digit  d = DIGIT(mp, ix);
     int       jx;
 
     /* Unpack digit bytes, high order first */
     for(jx = sizeof(mp_digit) - 1; jx >= 0; jx--) {
@@ -4774,17 +4774,17 @@ mp_err
 mp_to_signed_octets(const mp_int *mp, unsigned char *str, mp_size maxlen)
 {
   int  ix, pos = 0;
   int  bytes;
 
   ARGCHK(mp != NULL && str != NULL && !SIGN(mp), MP_BADARG);
 
   bytes = mp_unsigned_octet_size(mp);
-  ARGCHK(bytes <= maxlen, MP_BADARG);
+  ARGCHK(bytes >= 0 && bytes <= maxlen, MP_BADARG);
 
   /* Iterate over each digit... */
   for(ix = USED(mp) - 1; ix >= 0; ix--) {
     mp_digit  d = DIGIT(mp, ix);
     int       jx;
 
     /* Unpack digit bytes, high order first */
     for(jx = sizeof(mp_digit) - 1; jx >= 0; jx--) {
@@ -4814,17 +4814,17 @@ mp_err
 mp_to_fixlen_octets(const mp_int *mp, unsigned char *str, mp_size length)
 {
   int  ix, pos = 0;
   int  bytes;
 
   ARGCHK(mp != NULL && str != NULL && !SIGN(mp), MP_BADARG);
 
   bytes = mp_unsigned_octet_size(mp);
-  ARGCHK(bytes <= length, MP_BADARG);
+  ARGCHK(bytes >= 0 && bytes <= length, MP_BADARG);
 
   /* place any needed leading zeros */
   for (;length > bytes; --length) {
 	*str++ = 0;
   }
 
   /* Iterate over each digit... */
   for(ix = USED(mp) - 1; ix >= 0; ix--) {
--- a/security/nss/lib/freebl/secmpi.h
+++ b/security/nss/lib/freebl/secmpi.h
@@ -42,20 +42,23 @@
 
 #define OCTETS_TO_MPINT(oc, mp, len) \
     CHECK_MPI_OK(mp_read_unsigned_octets((mp), oc, len))
 
 #define SECITEM_TO_MPINT(it, mp) \
     CHECK_MPI_OK(mp_read_unsigned_octets((mp), (it).data, (it).len))
 
 #define MPINT_TO_SECITEM(mp, it, arena)                         \
-    SECITEM_AllocItem(arena, (it), mp_unsigned_octet_size(mp)); \
+ do { int mpintLen = mp_unsigned_octet_size(mp);                \
+    if (mpintLen <= 0) {err = MP_RANGE; goto cleanup;}          \
+    SECITEM_AllocItem(arena, (it), mpintLen);                   \
     if ((it)->data == NULL) {err = MP_MEM; goto cleanup;}       \
     err = mp_to_unsigned_octets(mp, (it)->data, (it)->len);     \
-    if (err < 0) goto cleanup; else err = MP_OKAY;
+    if (err < 0) goto cleanup; else err = MP_OKAY;              \
+  } while (0)
 
 #define MP_TO_SEC_ERROR(err)                                          \
     switch (err) {                                                    \
     case MP_MEM:    PORT_SetError(SEC_ERROR_NO_MEMORY);       break;  \
     case MP_RANGE:  PORT_SetError(SEC_ERROR_BAD_DATA);        break;  \
     case MP_BADARG: PORT_SetError(SEC_ERROR_INVALID_ARGS);    break;  \
     default:        PORT_SetError(SEC_ERROR_LIBRARY_FAILURE); break;  \
     }