Last Comment Bug 279085 - NSS tools display public exponent as negative number
: NSS tools display public exponent as negative number
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.10
: All All
: P3 normal (vote)
: 3.12
Assigned To: Alexei Volkov
:
:
Mentors:
Depends on: 314115
Blocks:
  Show dependency treegraph
 
Reported: 2005-01-20 00:12 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2007-01-08 17:03 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (896 bytes, patch)
2005-11-01 10:00 PST, Alexei Volkov
julien.pierre: review-
Details | Diff | Splinter Review
fix: added requested changes (2.51 KB, patch)
2005-11-01 14:13 PST, Alexei Volkov
no flags Details | Diff | Splinter Review
fix (3.66 KB, patch)
2005-11-02 17:40 PST, Alexei Volkov
no flags Details | Diff | Splinter Review
fix (3.51 KB, patch)
2005-11-04 15:34 PST, Alexei Volkov
julien.pierre: review+
Details | Diff | Splinter Review
modifying the fix according to last comments (5.51 KB, patch)
2006-01-18 17:23 PST, Alexei Volkov
julien.pierre: review+
nelson: review-
Details | Diff | Splinter Review
alternative way of fixing (1.21 KB, patch)
2007-01-08 14:39 PST, Alexei Volkov
nelson: review+
Details | Diff | Splinter Review
Integrated patch (1.20 KB, patch)
2007-01-08 17:02 PST, Alexei Volkov
no flags Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2005-01-20 00:12:39 PST
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 2005-10-24 15:09:02 PDT
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 2005-10-24 16:14:00 PDT
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 2005-10-26 15:41:49 PDT
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 2005-10-26 16:27:39 PDT
Yes, we can export DER_GetUInteger .
Comment 5 Alexei Volkov 2005-11-01 10:00:41 PST
Created attachment 201535 [details] [diff] [review]
fix
Comment 6 Julien Pierre 2005-11-01 13:56:17 PST
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 2005-11-01 14:13:39 PST
Created attachment 201560 [details] [diff] [review]
fix: added requested changes

diff generated with -U10 and changes in position of DER_GetUInteger in nss.def
Comment 8 Julien Pierre 2005-11-01 14:50:57 PST
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 2005-11-01 16:24:15 PST
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 2005-11-01 17:49:46 PST
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 2005-11-02 17:40:34 PST
Created attachment 201708 [details] [diff] [review]
fix

makes DER_GetInteger and DER_GetUInteger to return values that can be represented in 32 bits.
Comment 12 Wan-Teh Chang 2005-11-02 18:37:53 PST
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 2005-11-03 13:09:22 PST
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 2005-11-04 15:34:55 PST
Created attachment 201888 [details] [diff] [review]
fix

added: use int 32 type instead of long in dersubr.c
Comment 15 Wan-Teh Chang 2005-11-04 17:33:57 PST
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) 2005-11-05 12:38:55 PST
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 2005-11-07 15:04:12 PST
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 2005-11-07 20:40:00 PST
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 2005-12-07 15:56:12 PST
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 2005-12-07 19:14:52 PST
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 2005-12-07 19:25:50 PST
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 2006-01-18 17:23:12 PST
Created attachment 208920 [details] [diff] [review]
modifying the fix according to last comments
Comment 23 Nelson Bolyard (seldom reads bugmail) 2006-01-18 17:55:54 PST
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 2006-02-20 17:27:51 PST
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 2007-01-08 14:39:18 PST
Created attachment 250895 [details] [diff] [review]
alternative way of fixing

append zero(s) to the beginning of data if printing an unsigned integer.
Comment 26 Nelson Bolyard (seldom reads bugmail) 2007-01-08 15:51:01 PST
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 2007-01-08 17:02:28 PST
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
Comment 28 Alexei Volkov 2007-01-08 17:03:25 PST
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.75; previous revision: 1.74

Note You need to log in before you can comment on or make changes to this bug.