SEC_ASN1EncodeInteger cannot correctly encode some integer values

RESOLVED FIXED in 3.12.10

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

SEC_ASN1EncodeInteger cannot correctly encode integers in these ranges:

0x00000080-0x000000ff
0x00008000-0x0000ffff
0x00800000-0x00ffffff

It incorrectly concludes that those numbers are negative numbers. 
It does not ensure that a zero byte is placed in front of them  
in the encoding to keep them positive.
Created attachment 451476 [details] [diff] [review]
Patch v1 for NSS trunk

This seems to fix the problem, but I want to make sure it causes no 
regressions before requesting review.
Created attachment 451487 [details] [diff] [review]
Patch v2 for NSS trunk

This is even better.
Created attachment 451498 [details] [diff] [review]
Patch v3 for NSS trunk (with .def file)
Attachment #451487 - Attachment is obsolete: true

Updated

7 years ago
Target Milestone: 3.12.7 → 3.12.9

Updated

7 years ago
Duplicate of this bug: 613670

Comment 5

7 years ago
Another (minor?) problem with SEC_ASN1EncodeInteger is that it
doesn't encode negative values in the minimum number of octets.

For example, -129 should be encoded as the following value
octets:
    FF 7F

But SEC_ASN1EncodeInteger encodes -129 as the following value
octects on a 32-bit system:
    FF FF FF 7F

Similarly, -128 should be
    80
but is encoded as
    FF FF FF 80

Comment 6

7 years ago
Created attachment 503244 [details] [diff] [review]
Patch v4

Fix the incorrect encoding of a nonnegative value.

Fix a confusing comment that seems to imply there is
an *unsigned* DER encoding of INTEGER.

The 'make_unsigned' parameter is renamed 'is_unsigned'
to suggest that it means the 'value' argument is truly
an unsigned type.  (Perhaps 'value_is_unsigned' or even
'value_is_signed', with the opposite sense, is a better
name.)

I also added a test program.
Attachment #451476 - Attachment is obsolete: true
Attachment #451498 - Attachment is obsolete: true
Attachment #503244 - Flags: superreview?(rrelyea)
Attachment #503244 - Flags: review?(nelson)

Updated

7 years ago
Target Milestone: 3.12.9 → 3.12.10
Comment on attachment 503244 [details] [diff] [review]
Patch v4

I like this MUCH better than my previous patch!  Thanks.
Attachment #503244 - Flags: review?(nelson) → review+

Comment 8

7 years ago
Patch checked in on the NSS trunk (NSS 3.13).

Checking in lib/util/secasn1e.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1e.c,v  <--  secasn1e.c
new revision: 1.22; previous revision: 1.21
done
RCS file: /cvsroot/mozilla/security/nss/cmd/tests/encodeinttest.c,v
done
Checking in cmd/tests/encodeinttest.c;
/cvsroot/mozilla/security/nss/cmd/tests/encodeinttest.c,v  <--  encodeinttest.c
initial revision: 1.1
done
Checking in cmd/tests/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/tests/manifest.mn,v  <--  manifest.mn
new revision: 1.10; previous revision: 1.9
done

The bug fix (but not the test program) checked in on
the NSS_3_12_BRANCH (3.12.10).

Checking in secasn1e.c;
/cvsroot/mozilla/security/nss/lib/util/secasn1e.c,v  <--  secasn1e.c
new revision: 1.21.66.1; previous revision: 1.21
done

Filed bug 625239 for the problem I described in comment 5.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Attachment #503244 - Flags: superreview?(rrelyea)
You need to log in before you can comment on or make changes to this bug.