302262 - dsa.c should use the macros defined in secmpi.h

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

dsa.c defines a few macros that are also defined in
secmpi.h.  dsa.c should just include secmpi.h and
use the macros defined in secmpi.h instead.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch (checked in)

Comment 2

[Julien Pierre]

Are we sure this change will be compatible with the TFM bignum library
integration for AMD64 ?

Comment 3

[Wan-Teh Chang]

Attachment: Fix comment, set error code if signature verification fails

This patch is more important; it fixes an incorrect comment
and failure to set the error code when signature verification
fails.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190641 [details] [diff] [review]
Fix comment, set error code if signature verification fails

r=nelson

>     if (mp_cmp_z(&r_) <= 0 || mp_cmp_z(&s_) <= 0 ||
>-        mp_cmp(&r_, &q) >= 0 || mp_cmp(&s_, &q) >= 0)
>+        mp_cmp(&r_, &q) >= 0 || mp_cmp(&s_, &q) >= 0) {
>+	PORT_SetError(SEC_ERROR_BAD_SIGNATURE);

A comment, pointing out that err is zero here, might help.

> 	goto cleanup; /* will return verified == SECFailure */
>+    }

Comment 5

[Saul Edwards]

Regarding comment 2:

Since we haven't integrated TFM into dsa.c (only rsa.c) this change should not
affect the integration at all.

Comment 6

[Wan-Teh Chang]

Attachment: Fix comment, set error code if signature verification fails, v1.1 (checked in)

I added the comment Nelson suggested and checked in the
patch on the NSS trunk (NSS 3.11).

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190628 [details] [diff] [review]
Proposed patch (checked in)

With this patch applied, the code is as correct as it was before.
However, while reviewing this, I found a bug that is present both 
in the unpatched code, and in macro MPINT_TO_SECITEM.

>     /* Store y in public key */
>-    y_len = mp_unsigned_octet_size(&y);
>-    SECITEM_AllocItem(arena, &key->publicValue, y_len);
>-    err = mp_to_unsigned_octets(&y, key->publicValue.data, y_len);
>-    /*   mp_to_unsigned_octets returns bytes written (y_len) if okay */
>-    if (err < 0) goto cleanup; else err = MP_OKAY;
>+    MPINT_TO_SECITEM(&y, &key->publicValue, arena);

The problem is that SECITEM_AllocItem can fail to allocate,
setting key->publicValue.data to NULL.	When that happens, the code
crashes in mp_to_unsigned_octets, which relies on its caller to 
pass it a non-NULL pointer (2nd arg) and does not check it for NULL.

Fixing that bug is not a prerequisite to checking this patch in.
But after applying this patch, I'd suggest changing macro MPINT_TO_SECITEM
to check value of that pointer for NULL before calling mp_to_unsigned_octets

Comment 8

[Wan-Teh Chang]

Attachment: Check for SECITEM_AllocItem failure in MPINT_TO_SECITEM (checked in)

Nelson, please review this patch.  Alternatively, I could
do the NULL checking like this:

    if ((it)->data == NULL) err = MP_MEM;
    else err = mp_to_unsigned_octets(...);

Another question is whether you want me to write the if
statements in multiple lines.

Let me know what you think.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 191241 [details] [diff] [review]
Check for SECITEM_AllocItem failure in MPINT_TO_SECITEM (checked in)

r=nelson.boyard.  Thanks, Wan-Teh

Comment 10

[Wan-Teh Chang]

All three patches have been checked in on the NSS trunk for
NSS 3.11.