Closed
Bug 302262
Opened 19 years ago
Closed 19 years ago
dsa.c should use the macros defined in secmpi.h
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11
People
(Reporter: wtc, Assigned: wtc)
Details
Attachments
(3 files, 1 obsolete file)
11.33 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
Details | Diff | Splinter Review | |
633 bytes,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
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 2•19 years ago
|
||
Are we sure this change will be compatible with the TFM bignum library integration for AMD64 ?
Assignee | ||
Comment 3•19 years ago
|
||
This patch is more important; it fixes an incorrect comment and failure to set the error code when signature verification fails.
Attachment #190641 -
Flags: review?(nelson)
Comment 4•19 years ago
|
||
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 */ >+ }
Attachment #190641 -
Flags: review?(nelson) → review+
Comment 5•19 years ago
|
||
Regarding comment 2: Since we haven't integrated TFM into dsa.c (only rsa.c) this change should not affect the integration at all.
Assignee | ||
Comment 6•19 years ago
|
||
I added the comment Nelson suggested and checked in the patch on the NSS trunk (NSS 3.11).
Attachment #190641 -
Attachment is obsolete: true
Comment 7•19 years ago
|
||
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
Attachment #190628 -
Flags: review?(nelson) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #190628 -
Attachment description: Proposed patch → Proposed patch (checked in)
Assignee | ||
Comment 8•19 years ago
|
||
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.
Attachment #191241 -
Flags: review?(nelson)
Comment 9•19 years ago
|
||
Comment on attachment 191241 [details] [diff] [review] Check for SECITEM_AllocItem failure in MPINT_TO_SECITEM (checked in) r=nelson.boyard. Thanks, Wan-Teh
Attachment #191241 -
Flags: review?(nelson) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #191241 -
Attachment description: Check for SECITEM_AllocItem failure in MPINT_TO_SECITEM → Check for SECITEM_AllocItem failure in MPINT_TO_SECITEM (checked in)
Assignee | ||
Comment 10•19 years ago
|
||
All three patches have been checked in on the NSS trunk for NSS 3.11.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11
You need to log in
before you can comment on or make changes to this bug.
Description
•