307319 - Certificate details show incorrect public key information

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Simon Fraser [no longer active]]

It seems that PSM is showing the wrong data when I view the Public Key for a
certificate, in the Details tab of the View Certificate dialog.

For example, if I look at the "Equifax Secure CA" certificate, we display the
Subject's Public Key as: 

30 81 89 02 81 81 00 c1 5d b1 58 67 08 62 ee a0 
9a 2d 1f 08 6d 91 14 68 98 0a 1e fe da 04 6f 13 
84 62 21 c3 d1 7c ce 9f 05 e0 b8 01 f0 4e 34 ec 
e2 8a 95 04 64 ac f1 6b 53 5f 05 b3 cb 67 80 bf 
42 02 8e fe dd 01 09 ec e1 00 14 4f fc fb f0 0c 
dd 43 ba 5b 2b e1 1f 80 70 99 15 57 93 16 f1 0f 
97 6a b7 c2 68 23 1c cc 4d 59 30 ac 51 1e 3b af 
2b d6 ee 63 45 7b c5 d9 5f 50 d2 e3 50 0f 3a 88 
e7 bf 14 fd e0 c7 b9 02 03 01 00 01 

which is a weird number of bytes (140).

Looking at the same cert in the Keychain app in Mac OS X shows:

C1 5D B1 58 67 08 62 EE A0 9A 2D 1F 08 6D 91 14 68 98 0A 1E FE DA 04 6F 13 84 62
21 C3 D1 7C CE 9F 05 E0 B8 01 F0 4E 34 EC E2 8A 95 04 64 AC F1 6B 53 5F 05 B3 CB
67 80 BF 42 02 8E FE DD 01 09 EC E1 00 14 4F FC FB F0 0C DD 43 BA 5B 2B E1 1F 80
70 99 15 57 93 16 F1 0F 97 6A B7 C2 68 23 1C CC 4D 59 30 AC 51 1E 3B AF 2B D6 EE
63 45 7B C5 D9 5F 50 D2 E3 50 0F 3A 88 E7 BF 14 FD E0 C7 B9

which is 128 bytes.

Something is wrong here.

Comment 1

[Simon Fraser [no longer active]]

It looks like we dump an extra 7 bytes at the front, and 5 bytes at the back.
Where do those come from?

Comment 2

[Robert Relyea]

PSM is showing the full DER encoded public key. What it should show is the
modulus and the exponent (though for DSA it should show public value and pqg if
it exists).

BTW, does the Mac specify the key as modulus and display a separate exponent?



The full encoding is:
30 Sequence {
81 89 = 109Hex (138) bytes length
  02 Integer
  81 81 = 101Hex (129) bytes length
    0 + Modulus (128)
  02 Integer
  03 = 3 bytes length
    0x010001 (exponent).
}

Comment 3

[Robert Relyea]

Oops in my previous post I says

81 89 -> 109H 138 bytes: every thing is right except the 109H part, that should
be 89H.


Microsoft appears to present the same fully der encoded key in it's UI (this key
is 256 bytes (2048 bit)).

30 82 01 0a 02 82 01 01 00 9c ee f1 27 ef db d2 80 1c d7 1e f6 96 74 54 e1 83 cb
b3 38 52 8f 21 b5 3e c3 83 50 57 01 c3 52 9d 79 99 5d 02 c3 2f ce 9f c9 9e a0 2d
79 37 38 9d f1 c9 05 2c 97 b7 4f 5c bc 11 8a 0a 99 5c 3d a7 16 1c fb 62 21 df 80
05 b2 c3 72 5f a9 1a e0 43 24 63 ca aa a6 11 fe d4 dd 61 3d cf db a6 d8 86 5d 58
ac 3f 38 d0 a7 ae df a6 e9 0b 4e 86 e8 9b b9 f1 2a 77 26 22 47 d1 8f 44 19 28 c7
ab 4e e1 2a ee 0a 2e 01 8e ae 9b 3b db 8b 96 19 70 1c 50 33 81 fc 61 ab ec 73 63
d3 11 2b a0 d2 69 0a 89 20 e0 6e ec 4f b7 61 ba fd 9d 4c 61 15 e0 a1 07 88 60 de
1c 75 cf 2a 68 8c e2 15 7b ec 60 f2 ff 66 83 cc 7f 77 33 65 42 b1 65 b5 fb 2a 1b
97 58 fb 78 c7 3c 66 d7 2e e9 5c e8 64 b6 89 ca 11 ca 4c 66 78 db 78 ce a3 32 5d
5a 4f 3f 86 5e b0 cd b4 2d c6 2d 61 09 ce a6 4c 55 3c ea 44 ce 5d 02 03 01 00 01

Comment 4

[Simon Fraser [no longer active]]

The Mac OS UI doesn't appear to show the exponent anywhere.

So what is best from a user perspective? Would the user want to be able to see
the modulus and exponent separately?

What I would like to be able to do is to show the key size (e.g. "1024 bits"),
for which there is no API call now. It looks like I can deduce this from the
DER-encoded key, but is there a better way?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

This bug seems closely related to, or overlapping with, bug 259031 
but I don't think I'd call them duplicates.

This bug observes that PSM is dumping the Subject Public Key Info (SPKI) 
as raw undecoded DER in hex.  Bug 259031 obsreves that PSM also does that 
for a number of cert extensions, such as:
 
- Subject Key ID
- Authority Key ID
- CRL Distribution Points
- Authority Information Access
- Extended Key Usage
- Policies

Bug 259031 has a large patch attached to it that fixes many issues with 
badly displayed cert components.  

Martin, would you be willing to take a stab at this bug, too?

Comment 6

[Robert Relyea]

This field has very little utility to the average user, other than comparing two
certs to see if they have the same key.

To someone trying to diagnose a problem, not having the exponent could be an
issue. (I certainly would like to know if a cert had an exponent of 1 or 3, or
instance).

My feeling is that it would be easier to read and use if 1) everyone displayed
the key consistantly, and 2) the key was separated into it's components.
Obviously microsoft and apple disagree on how the key should be displayed, so 1
isn't possible (broadly speaking).

If we print the separate components, I think a reasonable person will see that
apple just neglected the exponent and be able to compare keys relatively easily,
and for someone who just read applied crypto, the key components would have more
meaning than the spki blob we are printing now.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #6)

> If we print the separate components, I think a reasonable person will see 
> that apple just neglected the exponent and be able to compare keys 
> relatively easily, and for someone who just read applied crypto, the key 
> components would have more meaning than the spki blob we are printing now.

I agree emphatically.  Simon, are you willing to tackle that?

Comment 8

[Martin v. Löwis]

I think there is also overlap with Bug 154641, although this is probably the wider-reaching request.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Here's a good reason to print the exponent:  It might be the value 1!
In that case, RSA doesn't do any encryption at all, and signatures are
trivially forged.  NSS now rejects RSA keys with that exponent.
It would be useful for the user to be able to clearly see the exponent
to understand the vulnerability.  

Comment 10

[Martin v. Löwis]

Attachment: Patch to display RSA keys

This patch breaks down RSA keys, and provides a framework for rendering other types of keys. RSA keys are displayed in the form

Modulus (%S Bits):
hex bytes of modulus

Exponent (%S Bits):
hex bytes of exponent

I started doing other keys (DH), but then found that
a) I have no way of testing them,
b) they are (probably) not supported in other parts of Mozilla, and
c) they might not be in wide use, at all.

If an unsupported key type is encountered, it falls back to the current
behaviour (presenting the raw bytes of the key).

For the record, the algorithms missing in the patch, but supported
by SECKEY_ExtractPublicKey are dhKey, dsaKey, fortezzaKey, keaKey,
and ecKey. There might be more key types defined elsewhere, but NSS cannot
break them down, so they should definitely be out of scope.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

What do you mean "NSS cannot break them down" ?
NSS uses them all, ergo, it understands their components.

Comment 12

[Martin v. Löwis]

All of the ones I listed, yes. However, there might be object identifiers which aren't recognized in NSS currently (e.g. some not defined in RFC 3279). For those, NSS will report nullKey (AFAICT); while PSM could try to analyse them on its own, there would be no point in doing so.

For example, I believe RSASSA-PSS (1.2.840.113549.1.10), defined in RFC 4055, is currently not supported in NSS. As always in ASN.1, object identifiers can take nearly arbitrary values, so there might be others for which there isn't even public documentation.

Comment 13

[Robert Relyea]

Adding Kai to bug.

Comment 14

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Nelson, Bob, Who should review Martin's patch?

This is Martin's patch merged to current trunk.
I tested it with an RSA cert and an ECC cert.


For an RSA cert I get:
  Modulus (1024 bits):
  Size: 128 Bytes / 1024 Bits
  9d 0f ae 70 5e 74 5e b2 3b fa 06 b4 a9 38 c1 03 
  76 17 da 43 93 c6 36 8c cd 1b d6 93 84 17 4a 57 
  81 5b 9e f4 72 40 29 7a 56 71 6b ea a3 7a 71 2b 
  27 7d df 38 d3 de e2 58 64 92 c4 7b 52 10 3b 70 
  e7 ac 69 2c 09 cf 24 32 86 d4 bc fe d6 60 46 a9 
  21 d7 39 75 1a fe 2e 73 d2 91 b3 dd 9d 89 cc 4f 
  30 46 09 da 98 ba fa 13 ff c8 b1 d4 26 7b 5f 17 
  21 65 5b c1 e6 4c f0 a1 6c 64 40 e5 b5 c6 36 79 
  
  Exponent (24 bits):
  Size: 3 Bytes / 24 Bits
  01 00 01 


For an ECC cert on I get:
  Size: 65 Bytes / 520 Bits
  04 e9 85 4f 45 a2 7f c2 27 ca 4b 65 f3 79 7d 8d 
  b2 5c cd 98 f1 28 90 c0 50 26 fb 0b 2b 5e 97 1d 
  ce a4 24 04 e9 04 57 ab 0e 0c 4e 1e f3 ee 27 c9 
  68 f1 ee 43 12 91 cc ac 55 09 34 54 7e 97 0e 18 
  9f 
(tested using https://ecc.fedora.redhat.com:8443/TLS_ECDH_ECDSA_WITH_RC4_128_SHA/ )

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

I would suggest that when the RSA public exponent fits in 32 bits or less 
(as it usually will) then it should be printed in decimal, not as a hex
byte string, e.g. 65537, not 01 00 01.  Be sure to use ntohl before printing
the value.  It's always in network byte order in the cert.

As for who should review it, AFAIK, I'm not an approved PSM reviewer. (?)
You and Bob will certainly suffice.

Comment 16

[Robert Relyea]

Comment on attachment 240397 [details] [diff] [review]
Patch v2

r=relyea.

some things that would be nice to fix:

1) you already put the size out in both bytes and bits in the ProcessRaw function, putting them out again in the header is a little redundant.

2) Nelson is right, I prefer the hex output, but exponents of size < 32 bits are traditionally printed in decimal. We should probably to the same.

There is no need to parse anything more for dh, and dsa keys. The key value is the same as the 'value'. parameters are typically stored in the parameter part of the key, which won't be displayed here. If they are in the cert, the will already be displayed correctly (AFAIK). It might be worth parsing the ECC certificates. ECC keys consist of 2 points X and Y with a tag saying wether or not the value is 'compressed'. 04 means uncompressed. The length of X and Y are equal to the total keylength/2 -1 (in bytes).

The main reason for doing this is to get a reasonable key length value (256 bit means the 'X' and 'Y' are 256 bits long, the key would be 520 bits)

Fixing this is not as important as fixing rsa, though.

bob

Comment 17

[Kai Engert [:KaiE:]]

Attachment: Patch v2 b

In this patch I omitted the duplicate size headers for the RSA key.
This was simple to do.
I'll check that in now.

As for the other comments, I propose we do that in a follow up patch, so the next patch will be smaller and easier to review.

carrying forward r=rrelyea

Comment 18

[Kai Engert [:KaiE:]]

Checked in to trunk.

Leaving bug open to address comment 16 in a follow up patch.

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Incremental Patch v3

Addressing review comments.

I found DER_GetInteger.
We have a common function for hex dumps in cert viewer.
In this patch I changed that common function to display all raw fields with a size <= 4 as a single integer. Not just public fields, but everything.
Is this a good idea?

This patch also contains an attempt to print details of an EC public key.
I found two functions in NSS, although not yet exported. Should they get exported?
I printed these out with labels:
  Key size (SECKEY_ECParamsToKeySize)
  Base point order length (SECKEY_ECParamsToBasePointOrderLen)
Do these labels make sense?
I did not find a function to decode the public key value into a point x/y.
I found how one of NSS' cmd utils prints it out, and mirrored that:
  Public value
(followed by a hex dump)

Comments welcome.
I coded this up quickly, feel free to pick this patch up and improve it (I don't think I'll spend a lot of time on this bug)

Comment 20

[Kai Engert [:KaiE:]]

I also think, we should consider to break up the string in the properties file into separate items, to make it easier for localizers. Currently it's a challenge to translate it and still get all our line breaks and % variables right.

Comment 21

[Kai Engert [:KaiE:]]

Attachment: NSS change [checked in]

I forgot to include this snippet that exports the functions I use in patch v3.

Does that make sense?

Comment 22

[Martin v. Löwis]

(In reply to comment #19)

I don't think it is good to decode all byte strings as integer. The value might be an unsigned integer, or it might be some kind of character or bit string. I think it sometimes also holds DER data (TLV). Converting them to integer only adds confusion. Instead, we should restrict display-as-integer to cases where we know it's a signed integer.

Comment 23

[Robert Relyea]

Comment on attachment 240961 [details] [diff] [review]
NSS change [checked in]

r+ for NSS trunk (Change NSS_3.11.4 to NSS_3.12)

Comment 24

[Robert Relyea]

Comment on attachment 240558 [details] [diff] [review]
Incremental Patch v3

r+.

OK to slipt this patch and land the ecc switch statement after the appropriate NSS patch has landed.

Comment 25

[Kai Engert [:KaiE:]]

Comment on attachment 240961 [details] [diff] [review]
NSS change [checked in]

this patch, well, the equivalent of this patch for NSS 3.12, has been landed on the trunk of NSS.

Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.166; previous revision: 1.165
done

Comment 27

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central (attachement 240558):
http://hg.mozilla.org/mozilla-central/index.cgi/rev/661cce8d7fd0