320047 - mp_to_unsigned_octets copies nothing to the buffer if the mp_int is zero.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

The mp_to_unsigned_octets function has this prototype:

mp_err mp_to_unsigned_octets(const mp_int *mp, unsigned char *str, mp_size maxlen);

If the mp_int argument 'mp' has the value 0, the function
does not copy anything to the buffer 'str' and returns 0.
This is wrong.  The function should copy one zero byte to
the buffer 'str' and return 1.

I found this bug by code inspection while comparing the
mp_unsigned_octet_size function with mp_to_unsigned_octets.
mp_unsigned_octet_size returns 1 if the mp_int is zero.
I confirmed this bug with the following code snippet:

    mp_err err;
    mp_int zero;
    int zero_len;
    unsigned char zeroBytes[16];

    MP_DIGITS(&zero) = 0;
    CHECK_MPI_OK( mp_init(&zero) );
    mp_zero(&zero);
    zero_len = mp_unsigned_octet_size(&zero);
    printf("zero_len = %d\n", zero_len);
    memset(zeroBytes, 0xaf, sizeof zeroBytes);
    CHECK_MPI_OK( mp_to_unsigned_octets(&zero, zeroBytes, zero_len) );
    {
        int i;
        for (i = 0; i < sizeof zeroBytes; i++) {
            printf("%02X", (unsigned)zeroBytes[i]);
        }
        printf("\n");
    }
 cleanup:
+    mp_clear(&zero);

The output is:

zero_len = 1
AFAFAFAFAFAFAFAFAFAFAFAFAFAFAFAF

which shows mp_to_unsigned_octets didn't write anything
to the buffer.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Upon closer look, I found that mp_to_signed_octets has the
same bug.  Since mp_to_signed_octets is unused, we could also
just remove it.

In mp_to_unsigned_octets, I added an assertion to ensure
that mp_unsigned_octet_size and mp_to_unsigned_octets return
consistent results.  Please let me know if it is fine to use
Standard C library's assert macro in mpi.

Unfortunately I can't add an equivalent assertion in
mp_to_signed_octets because there is no mp_signed_octet_size
function.  mp_unsigned_octet_size is off by one if the most
significant bit is 1 and we need to add an extra leading 0
byte to make the result positive.

Comment 2

[Wan-Teh Chang]

Comment on attachment 205674 [details] [diff] [review]
Proposed patch

With this patch applied, the output of the code snippet is:

zero_len = 1
00AFAFAFAFAFAFAFAFAFAFAFAFAFAFAF

So mp_to_unsigned_octets write one zero byte to the buffer
and returns 1 (because the assertion does not fail).

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch v2

I found that mp_to_fixlen_octets has the same bug.
This patch fixes that, too.

I am willing to do without the assertions.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 205678 [details] [diff] [review]
Proposed patch v2

This patch is fine ... except possibly for the new assertions.

IIRC, some platforms don't support assert.h.  I don't recall which.  
Perhaps WinCE?  If we're sure that this new assertion won't break the 
builds on any platforms, including WinCE, then I'm fine with keeping the 
new asserts.  Otherwise, I suggest you use ARGCHK instead.  The ARGCHK 
macro expands to an assertion or to another expression depending on a 
configurable parameter.  On platforms where assert isn't supported,
we need only configure ARGCHK to expand to another expression.

In comment 1 you wrote:
> mp_unsigned_octet_size is off by one if the most
> significant bit is 1 and we need to add an extra leading 0
> byte to make the result positive.

As the function name implies, the octet string it outputs is 
UNsigned, meaning that the most significant bit of the first octet is 
NOT interpreted as a sign bit, so we never have to prepend a
zero byte.  In fact, the first byte of the octet string output
by that function should never be zero, unless it is a one-octet
string representing the value zero.

So, r+ if the new assert doesn't break any platforms. r- otherwise.

Comment 5

[Wan-Teh Chang]

Attachment: Patch as checked in

I can live without those assertions, so I removed
them.  I don't want to use ARGCHK because neither
'pos' nor 'bytes' is a function argument.

I'm requesting two reviews so this patch can be
checked in on the NSS_3_11_BRANCH.  If you think
this bug is not serious enough, please let me know.
(Apparently it is uncommon for our bignum arithmetic
result to be zero.)

Nelson, I rewrote the paragraph you asked about below
to make it more clear:

  Unfortunately I can't add an equivalent assertion in
  mp_to_signed_octets because there is no mp_signed_octet_size
  function.  mp_to_signed_octets uses mp_unsigned_octet_size
  as the closest substitute, but the value returned by
  mp_unsigned_octet_size is one less than what
  mp_signed_octet_size would return when the most significant
  bit is 1 and we need to add an extra leading 0 byte to make
  the result positive.

Comment 6

[Wan-Teh Chang]

Comment on attachment 205787 [details] [diff] [review]
Patch as checked in

I checked in this patch on the NSS trunk (NSS 3.12).

Checking in mpi.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi.c,v  <--  mpi.c
new revision: 1.44; previous revision: 1.43
done

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 205787 [details] [diff] [review]
Patch as checked in

r=nelson

Comment 8

[Robert Relyea]

Comment on attachment 205787 [details] [diff] [review]
Patch as checked in

I find use the !pos syntax a bit misleading (it gives the strong impression that pos is a supposed to be a bool, not an int), but that's the provailing style of this file, so it's the right decision to use it here.

Comment 9

[Wan-Teh Chang]

I checked in the patch on the NSS_3_11_BRANCH (3.11.1).

Checking in mpi.c;
/cvsroot/mozilla/security/nss/lib/freebl/mpi/mpi.c,v  <--  mpi.c
new revision: 1.43.2.1; previous revision: 1.43
done