279085 - NSS tools display public exponent as negative number

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.)

Comment 1

[Alexei Volkov]

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

[Julien Pierre]

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 ?

Comment 3

[Alexei Volkov]

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

[Julien Pierre]

Yes, we can export DER_GetUInteger .

Comment 5

[Alexei Volkov]

Attachment: fix

Comment 6

[Julien Pierre]

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.

Comment 7

[Alexei Volkov]

Attachment: fix: added requested changes

diff generated with -U10 and changes in position of DER_GetUInteger in nss.def

Comment 8

[Julien Pierre]

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

[Wan-Teh Chang]

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

[Julien Pierre]

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.

Comment 11

[Alexei Volkov]

Attachment: fix

makes DER_GetInteger and DER_GetUInteger to return values that can be represented in 32 bits.

Comment 12

[Wan-Teh Chang]

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

[Robert Relyea]

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

Comment 14

[Alexei Volkov]

Attachment: fix

added: use int 32 type instead of long in dersubr.c

Comment 15

[Wan-Teh Chang]

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.)

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

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

[Wan-Teh Chang]

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

[Julien Pierre]

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

[Julien Pierre]

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.

Comment 20

[Wan-Teh Chang]

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

[Julien Pierre]

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.

Comment 22

[Alexei Volkov]

Attachment: modifying the fix according to last comments

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 24

[Wan-Teh Chang]

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.

Comment 25

[Alexei Volkov]

Attachment: alternative way of fixing

append zero(s) to the beginning of data if printing an unsigned integer.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 27

[Alexei Volkov]

Attachment: Integrated patch

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

Comment 28

[Alexei Volkov]

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