341124 - Coverity 522 Callers of mp_unsigned_octet_size don't check for negative return value

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

mp_unsigned_octet_size can return a negative value.
Freebl functions DH_Derive and KEA_Derive call it and do not check its
return value for a negative number before using it.  
Coverity thinks they should

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Code is inside the FIPS boundary

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 430921 [details] [diff] [review]
proposal


>     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) {
>+        PORT_SetError(SEC_ERROR_INVALID_ARGS);
>+        return SECFailure;
>+    }

Uh, this will cause a leak of all the allocated mp_ints.  
Think about going to the "cleanup" label instead.

Comment 4

[timeless]

Attachment: cleanup

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 431045 [details] [diff] [review]
cleanup

I believe this change is correct, so I will give it r+, but I would
like to suggest that you consider a further improvement.
Rather than adding a new variable declaration in every function that
uses the macro MPINT_TO_SECITEM, I'd suggest you embed the 
declaration in that macro, e.g. something like this:


> #define MPINT_TO_SECITEM(mp, it, arena)                         \
>+ 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;              \
>+  } while (0)

Comment 6

[timeless]

Attachment: localized

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Checking in dh.c;      new revision: 1.8.8.1; previous revision: 1.8
Checking in secmpi.h;  new revision: 1.5.132.1; previous revision: 1.5
Checking in mpi/mpi.c; new revision: 1.45.56.1; previous revision: 1.45

Thanks Timeless