NSS tools display public exponent as negative number

RESOLVED FIXED in 3.12

Status

NSS
Tools
P3
normal
RESOLVED FIXED
13 years ago
11 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Bug seen in personal build of trunk (3.10 alpha).

If you use pp to print out the first cert shown in bug 277797,
it displays that the public exponent in the cert's public key is
                Exponent: -14979 (0xffffc57d)
But in fact, the public exponent in that cert is a properly encoded 
positive INTEGER, e.g. 
    <02 03 00 C5 7D>      INTEGER 50557

The issue occurs because the SECItem for the decoded SPKI pub exponent
contains only 2 bytes, the first of which is 0xC5.  DER_GetInteger 
interprets this as negative.  Question is, why is the leading zero 
byte being removed from this value?  

Not clear to me, at the moment, if this bug only affects the cert printing
code in pp and certutil, or if it also would negatively affect NSS's ability
to use this public key.  (The cert is rejected for unknown critical extensions.)

Updated

13 years ago
Assignee: wtchang → alexei.volkov.bugs
(Reporter)

Updated

12 years ago
QA Contact: bishakhabanerjee → jason.m.reid
(Assignee)

Comment 1

12 years ago
nss asn1 decoder was implemented to remove leading zeroes bytes 
when decoding SECItem target has "type" field equals to siUnsignedInteger.

To get the public key nss uses private function from lib/cryptohi/seckey.c
seckey_ExtractPublicKey. This function sets siUnsignedInteger value into
"modulus" and "publicExponent" fields of RSA public key structure.

This makes decoder remove leading zeroes and, since we use 
SECU_PrintInteger to print sign integer, prints it as negative number.

Julien proposed that the fix should be made in secutil.c to replace
SECU_PrintInteger to SECU_PrintUInteger in secu_PrintRSAPublicKey and
other key printing functions that suppose to print unsigned int(or make
SECU_PrintInteger to print unsigned integer when ever siUnsignedInteger
is set). In order to do it, I need to use DEC_GetUInteger. But here are
two problems:

   * DEC_GetUInteger is internal nss function that should be exported
   in order to be used in secutil.c

   * DEC_GetUInteger returns an error if the highest bit of the first
   byte is set.


Any advises? Thanks!

Comment 2

12 years ago
When using siUnsignedInteger in the type, the decoder actually stores non-DER data, since it strips the leading zeroes. So, neither DER_GetInteger or DER_GetUInteger are appropriate. Can this case possibly be dealt with by prepending a leading zero and then calling DER_GetUInteger ?
(Assignee)

Comment 3

12 years ago
Julien, yes it is possible to check if SECItem.type is set to siUnsignedInteger and then prepend a leading zero. The implementation will fix problem associated with printing integer that has the highest bit of the first byte is set. But what about "first star" item. Will you be willing to export the DER_GetUInteger function?

Comment 4

12 years ago
Yes, we can export DER_GetUInteger .

Updated

12 years ago
Depends on: 314115
(Assignee)

Comment 5

12 years ago
Created attachment 201535 [details] [diff] [review]
fix
Attachment #201535 - Flags: review?(julien.pierre.bugs)

Comment 6

12 years ago
Comment on attachment 201535 [details] [diff] [review]
fix

Looks mostly OK, but the symbols in nss.def need to be in alphabetical order within each section. You added DER_GetInteger before  CERT_CompareValidityTimes.

Also, please do universal diffs and provide some context (10 or 15 lines are good). The rest of the patch looks OK.
Attachment #201535 - Flags: review?(julien.pierre.bugs) → review-
(Assignee)

Comment 7

12 years ago
Created attachment 201560 [details] [diff] [review]
fix: added requested changes

diff generated with -U10 and changes in position of DER_GetUInteger in nss.def
Attachment #201560 - Flags: review?(julien.pierre.bugs)

Comment 8

12 years ago
While taking a closer look at this, I'm not wild about exporting the current implementation of DER_GetUInteger . The main problem is that it returns an unsigned long, which is a type that varies between platforms. Unfortunately, part of the damage has already been done, since DER_GetInteger is already exported.

Wan-Teh, do you have any suggestion on this one ?

Comment 9

12 years ago
Since DER_GetInteger is already exported, I think it is fine
to export DER_GetUInteger.  But we should add comments that
describe the limitations of these two functions:

1. For portability, the return value (a long or unsigned long)
should be considered 32-bit.  Note: this interpretation of
the return type is reinforced by the corresponding "set"
functions DER_SetInteger and DER_SetUInteger, which use
int32/uint32 instead of long/unsigned long.

2. The integer/unsigned integer stored in the SECItem must
be representable in 32 bits.

Note that SEC_ASN1EncodeInteger and SEC_ASN1DecodeInteger
(both are exported) also use long as the type of the integer.
This limits the size of the integer we can encode or decode.
(See bug 265003 comment 20.)  They also seem to do the same
thing as DER_SetInteger and DER_GetInteger.  The more I look,
the more confused I am.

Comment 10

12 years ago
Wan-Teh,

Currently, DER_GetUInteger as implemented does not return a 32-bit value, it returns a value up to the size of the native unsigned long. The implementation needs to be changed in order to limit it to 32 bits. I suspect DER_GetInteger has the same issue, though I didn't look at it.

This is confusing. I wish we could obsolete a few APIs and replace them with something consistent.
(Assignee)

Comment 11

12 years ago
Created attachment 201708 [details] [diff] [review]
fix

makes DER_GetInteger and DER_GetUInteger to return values that can be represented in 32 bits.
Attachment #201535 - Attachment is obsolete: true
Attachment #201560 - Attachment is obsolete: true
Attachment #201708 - Flags: review?(julien.pierre.bugs)
Attachment #201560 - Flags: review?(julien.pierre.bugs)

Comment 12

12 years ago
Comment on attachment 201708 [details] [diff] [review]
fix

Instead of adding the comment
    Set integer size in bytes to be unique on all platforms.
it is better to change "machine integral value" to "32-bit machine
integral value" in the existing comments.

The SIZE_OF_INT32 macro is not necessary.  Just replace sizeof(ival)
by sizeof(int32) or sizeof(uint32).

It may be better to fix this bug by declaring 'ival' as int32/uint32
and 'overflow' and 'ofloinit' as uint32.

Comment 13

12 years ago
You will note that the code that is using this value checks to make sure that we are dealing with a 4 byte integer or less before it uses DER_Decode, even though in this case the raw hex value is probably more meaningful.

If the exponent were greater than 4 bytes, the function outputs raw hex digits (say if you tried to print out a private exponent).

bob
(Assignee)

Comment 14

12 years ago
Created attachment 201888 [details] [diff] [review]
fix

added: use int 32 type instead of long in dersubr.c
Attachment #201708 - Attachment is obsolete: true
Attachment #201888 - Flags: review?(julien.pierre.bugs)
Attachment #201708 - Flags: review?(julien.pierre.bugs)

Comment 15

12 years ago
Comment on attachment 201888 [details] [diff] [review]
fix

> long
> DER_GetInteger(SECItem *it)
> {
>-    long ival = 0;
>+    PRUint32 ival = 0;

This ival should be PRInt32.  You need to
change a signed type to a signed type, and
an unsigned type to an unsigned type.

This kind of patch looks harmless but sometimes
introduces subtle bugs.  So I'm having second
thought whether we should make DER_GetInteger
and DER_GetUInteger behave consistently across
32-bit and 64-bit operating systems.  At least,
this patch must be thoroughly tested on a
64-bit platform.  (The long->PRInt32 and unsigned
long->PRUint32 changes are no-ops on 32-bit platforms.)
(Reporter)

Comment 16

12 years ago
I think we should not change the types used/returned by those DER (u)integer 
functions.  The author of those functions chose to use long intentionally.
His intent was to produce a function that would handle the largest size
integer natively supported by the CPU.  Use of types of ambiguous size is no
longer in fashion, but IMO we should not change the definitions of some of 
the oldest NSS functions to suit our changed conventions.  Note that use
of the types "int" and "long" is easier with printf than use of the types
int32 and int64, as the former correspond to %d and %ld on all platforms,
while the latter do not correspond to any fixed platform-independent format 
strings.  

So I vote to export DER_GetUInteger as is, leave the type as (u)long, and 
continue to allow the function to return any value that fits in the local
machine's definition of (u)long.  

If you feel strongly that we need a function that returns a value whose
size is the same on all platforms, then create new functions, DER_GetInteger32
and DER_GetInteger64 and use them instead.  But I think this bug can be fixed without switching to GetInteger functions of fixed length.

That's my $.02 advice. :)

Comment 17

12 years ago
Nelson: if you use PR_*printf, PRInt32 always corresponds
to %ld and PRInt64 always corresponds to %lld.

I am fine with Nelson's suggestion of not changing
DER_GetInteger and DER_GetUInteger.

Comment 18

12 years ago
DER_GetUInteger is not exported, and only has one caller in all of NSS - it's lib/certdb/crl.c . And that code casts the result to an int, so the 64-bit ability is completely lost on the caller.

Given this, I think we should expose DER_GetUInteger as Alexei wrote it, except that its return type should be changed from unsigned long to PRUInt32 .

DER_GetInteger is already a public function, and it doesn't necessarily have to change for this bug fix. Obviously its return type can't change. Limiting its return value to the lower 32 bits is good, but is not required .

Comment 19

12 years ago
Comment on attachment 201888 [details] [diff] [review]
fix

r+, except for the following :

please change the return type of DER_GetUInteger to PRUint32 . There is only one caller of this internal function in all of NSS, and it casts the return value to int, so we know nobody depends on a 64-bit return value.
Attachment #201888 - Flags: review?(julien.pierre.bugs) → review+

Comment 20

12 years ago
Comment on attachment 201888 [details] [diff] [review]
fix

DER_GetUInteger and DER_GetInteger should return
unsigned and signed versions of the same integer
type.  It would be strange for one to return PRUint32
and the other to return long.

Comment 21

12 years ago
Wan-Teh,

It is strange, but the mistake of DER_GetInteger returning long can't be undone without breaking binary compatibility. We can deal with the confusion by not having a DER_GetUInteger at all, and renaming the DER_GetUInteger function from Alexei's patch to DER_GetUInteger32 . I don't know if having all the variants (64, 32, signed, unsigned) is desirable, but their presence isn't necessary to resolve this bug.
(Assignee)

Comment 22

12 years ago
Created attachment 208920 [details] [diff] [review]
modifying the fix according to last comments
Attachment #201888 - Attachment is obsolete: true
Attachment #208920 - Flags: review?(julien.pierre.bugs)

Updated

12 years ago
Attachment #208920 - Flags: review?(julien.pierre.bugs) → review+
(Reporter)

Comment 23

12 years ago
Comment on attachment 208920 [details] [diff] [review]
modifying the fix according to last comments


> long
> DER_GetInteger(SECItem *it)
> {
>-    long ival = 0;
>+    PRUint32 ival = 0;

Wait, so we're leaving the type of DER_GetInteger as long, but we're
changing the function so that it never returns 64-bit values?

>     unsigned len = it->len;
>     unsigned char *cp = it->data;
>-    unsigned long overflow = 0x1ffUL << (((sizeof(ival) - 1) * 8) - 1);
>-    unsigned long ofloinit;
>+    PRUint32 overflow = 0x1ffUL << (((sizeof(ival) - 1) * 8) - 1);

The UL should just be U.  UL was correct when the type was unsigned long,
but it not correct for a 32-bit type.

>-unsigned long
>-DER_GetUInteger(SECItem *it)
>+PRUint32
>+DER_GetUInteger32(SECItem *it)
> {
>-    unsigned long ival = 0;
>+    PRUint32 ival = 0;
>     unsigned len = it->len;
>     unsigned char *cp = it->data;
>-    unsigned long overflow = 0xffUL << ((sizeof(ival) - 1) * 8);
>+    PRUint32 overflow = 0xffUL << ((sizeof(ival) - 1) * 8);

same issue with UL vs. U here.

Also,  use BPB instead of 8.
(Reporter)

Updated

12 years ago
Attachment #208920 - Flags: review-

Comment 24

12 years ago
I just found that SEC_ASN1DecodeInteger has the same
problem.  It stores its output in an 'unsigned long *'
output argument.  Related functions SEC_ASN1EncodeInteger
and SEC_ASN1EncodeUnsignedInteger take a 'long' or
'unsigned long' input argument.

This "elastic" integer type (long/unsigned long) caused
a confusion in the investigation of bug 325366.  In
bug 325366 comment 2, Kai said that he couldn't reproduce
that bug.  It turned out that it's because the bug reporter
has a 32-bit machine and Kai has a 64-bit machine.  So,
Julien's worry is not theoretical.

While I find such confusions unfortunate, the discovery
of these three SEC_ASN1 functions that have the same
problem makes me think we shouldn't change return
value of DER_GetInteger and DER_GetUInteger; it's better
for these functions to stay consistent with each other.
(Reporter)

Updated

12 years ago
QA Contact: jason.m.reid → tools
(Assignee)

Updated

11 years ago
Priority: -- → P3
Target Milestone: --- → 3.12
(Assignee)

Comment 25

11 years ago
Created attachment 250895 [details] [diff] [review]
alternative way of fixing

append zero(s) to the beginning of data if printing an unsigned integer.
Attachment #208920 - Attachment is obsolete: true
Attachment #250895 - Flags: review?(nelson)
(Reporter)

Comment 26

11 years ago
Comment on attachment 250895 [details] [diff] [review]
alternative way of fixing

r=nelson

>+            SECItem tmpI;
>+            char data[] = {0, 0, 0, 0, 0};
>+
>+            PORT_Memcpy(data + (5 - i->len), i->data, i->len);
>+            tmpI.len = 5;

I would have coded that as:
        PORT_Memcpy(data + 1, i->data, i->len);
        tmpI.len = i->len + 1;

But your way works too.
Attachment #250895 - Flags: review?(nelson) → review+
(Assignee)

Comment 27

11 years ago
Created attachment 250918 [details] [diff] [review]
Integrated patch

/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.75; previous revision: 1.74
(Assignee)

Comment 28

11 years ago
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.75; previous revision: 1.74
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.