355447 - Certificate Viewer shows no details for certs in chain

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

This problem is seen with PSM in the SeaMonkey trunk builds.

I received a signed S/MIME email message.  It appears to be correctly signed.

When I click on the pen icon, PSM's "Message Security" dialog appears, with a 
button labelled "View Signature Certificate".  When I click that button, 
PSM's Certificate Viewer dialog appears.  The "details" tab shows me 3 panes, Certificate Hierarchy, Certificate Details, and Field Value.  

The complete chain seems to be shown correctlyin the Certificate Hierarchy pane.  
However, the "Certificate Fields" pane is blank, regardless of which certificate
I select in the Hierarchy.  

If I close the "Certificate Viewer" dialog window, and then bonk the "View Signature Certificate" button in PSM's "Message Security" dialog a second time, 
PSM's Certificate Viewer dialog appears a second time, and THIS time, the contents of the Certificate Details pane are not blank.  
However, they are only correct for the CA certs, not for the EE (leaf) cert.  
I cannot display the Certificate Details or Field Valuess for the EE cert.

I will attach an email message that reproduces this, as well as some screen
captures showing the problem appearance.  

100% reproducible with all signed emails from this same source.

CC'ing Kaspar, in case he's interested.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: signed email message that demonstrates problems

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: message security dialog

Dialog shows that PSM is able to get fields from the EE cert and display 
them individually.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Cert viewer dialog with blank Cert Details pane for EE cert

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Cert viewer dialog with blank Cert Details pane for CA cert

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Correct Cert viewer CA display, after reopening cert viewer

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: incorrect EE Cert Viewer display, after reopening cert viewer

Here, after viewing the correct CA certs, I click on the EE cert in the
certificate hierarchy pane.  The Certificate Details pane continues to 
show the details for the previously chosen CA cert.  These are NOT the
correct details for the EE cert, but are old CA cert details.  The 
Cerificate Details pane was not redrawn.

Comment 7

[Iain MacDonnell]

Same problem in Thunderbird 2.0a1 on Solaris x86, FWIW.

Comment 8

[Kai Engert (:KaiE:)]

After a bit of debugging, I found the cause of this issue to be in NSS.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp&rev=1.25&mark=1293#1293

At this line PSM calls SEC_QuickDERDecodeItem, which fails with -8183, 
SEC_ERROR_BAD_DER, Security library: improperly formatted DER-encoded message.

Comment 9

[Kai Engert (:KaiE:)]

cc'ing Martin FYI. He added the call and the DER template with bug 259031. Not sure whether the error is in the NSS parser or in the template.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Certificate in PEM format

I extracted the cert from the message, here it is in PEM format.
I will also attach a pretty-print dump.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Certificate pretty print dump

Comment 12

[Kai Engert (:KaiE:)]

I confirm I get this on 1.8 branch, too, all platforms.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #9)
> Not sure whether the error is in the NSS parser or in the template.

Or it could be that the cert itself is incorrectly encoded.

But regardless of which of those possibilities is the answer,
an inability to decode a tiny subset of a single extension (one of many)
in a cert should not prevent us from displaying the parts of the cert that
can be (and have successfully been) decoded.   

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch v1

(In reply to comment #13)
> an inability to decode a tiny subset of a single extension (one of many)
> in a cert should not prevent us from displaying the parts of the cert that
> can be (and have successfully been) decoded.   

Good point. This patch changes failure on DER decoding for displaying cert extensions to no longer be fatal, but be ignored - without further processing of the item.

With this patch cert viewer displays most data of the cert.

Comment 15

[Iain MacDonnell]

Is there actually anything invalid about the cert? If so, can someone explain precisely what it is?

Comment 16

[Kai Engert (:KaiE:)]

In case it wasn't clear from the code link I sent, the decoding failure happens in ProcessUserNotice.

(I can not say who is the culprit)

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

The immediate cause of the failure of SEC_QuickDERDecodeItem at line 1293
is that the template (at line 1229) is a choice of 3 string types:
- VisibleString
- UTF8String
- BMPString

but the data in the cert is a string of type IA5String, which is not among
the choices in the template.  

So the next question to be answered is: does the official definition of this
extension permit IA5String, or not?  If yes, then the template is bad.  
If no, then the cert is improperly encoded.  
I'll go have a look at some RFCs.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

RFC 3280 says (page 32)

   DisplayText ::= CHOICE {
        ia5String        IA5String      (SIZE (1..200)),
        visibleString    VisibleString  (SIZE (1..200)),
        bmpString        BMPString      (SIZE (1..200)),
        utf8String       UTF8String     (SIZE (1..200)) }

So, there you have it.  The template DisplayTextTemplate at 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp&rev=1.25&mark=1229,1293#1229
is bad.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Fix template patch

I think this should do the trick.

Comment 20

[Kai Engert (:KaiE:)]

Comment on attachment 241512 [details] [diff] [review]
Fix template patch

Thanks Nelson, this patch works for me! Cert viewer now displays the User Notice info in field Certificate Policies.

I'm renaming it to "Fix template patch".

r=kengert

Should we take the other patch anyway?

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Yes, I'd recommend taking both patches (assuming they both get r+).

I'm disturbed by the fact that one small problem with a leaf cert 
prevented the correct display of ANY of the certs in the chain.
I think we need to ensure that the display of each cert is as 
independent of the display of the other certs as possible.  

Comment 22

[Martin v. Löwis]

(In reply to comment #18)
> So, there you have it.  The template DisplayTextTemplate at 
> is bad.

I'd like to point out that RFC 2459 defines it as

DisplayText ::= CHOICE {
        visibleString    VisibleString  (SIZE (1..200)),
        bmpString        BMPString      (SIZE (1..200)),
        utf8String       UTF8String     (SIZE (1..200)) }


So it's an incompatible change in the protocol that caused this bug.

OTOH, according to the thread at

http://www.imc.org/ietf-pkix/old-archive-99/msg02652.html

it appears that the PKIX drafts originally had IA5String instead of VisibleString, and only changed to to VisibleString in the published RFC 2459, so it seems that this was "meant" to support IA5String all along.

Comment 23

[Martin v. Löwis]

(In reply to comment #21)
> I'm disturbed by the fact that one small problem with a leaf cert 
> prevented the correct display of ANY of the certs in the chain.
> I think we need to ensure that the display of each cert is as 
> independent of the display of the other certs as possible.  

ISTM that this is very difficult to achieve. Surely, there are failures
for which processing should not continue (e.g. out-of-memory). Now,
most code tests with NS_FAILED, so it doesn't distinguish between
more fatal and less fatal errors.

I'm uncertain though where in the XUL/js/C++ the logic is for 
stopping to browse after an error, though.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

RFC 3280 obsoleted RFC 2459 in April 2002, over 4 years ago.
No-one should be citing RFC 2459 now, 4+ years later.

Comment 25

[Kaspar]

Attachment: Patch v2 - make extension display in cert viewer more robust (against trunk)

(In reply to comment #21)
> I'm disturbed by the fact that one small problem with a leaf cert 
> prevented the correct display of ANY of the certs in the chain.
> I think we need to ensure that the display of each cert is as 
> independent of the display of the other certs as possible.

(In reply to comment #23)
> I'm uncertain though where in the XUL/js/C++ the logic is for 
> stopping to browse after an error, though.

I propose the attached patch to make cert viewer more robust when displaying extension data - i.e. fall back to displaying raw data if decoding of the extension fails (for whatever reason). I'll add another attachment with a screen shot showing Iain MacDonnell's cert if this patch is applied.

This makes patch v1 unnecessary, IMO (otherwise, quite a lot of additional NSERROR_FAILURE cases would have to be taken care of, too). The template fix should be committed, of course.

Comment 26

[Kaspar]

Attachment: MacDonnell cert display if "robustness patch" is applied (but w/o template fix)

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #20)
> (From update of attachment 241512 [details] [diff] [review] [edit])
> Thanks Nelson, this patch works for me! Cert viewer now displays the User
> Notice info in field Certificate Policies.

Kai, do you want this patch?  If so, please commit it.  Thanks.

Comment 28

[Kai Engert (:KaiE:)]

Comment on attachment 241512 [details] [diff] [review]
Fix template patch

I checked this patch in to the trunk of Mozilla.
Proposing this correctness fix for FF 2.0.x

Comment 29

[Kai Engert (:KaiE:)]

Comment on attachment 243169 [details] [diff] [review]
Patch v2 - make extension display in cert viewer more robust (against trunk)

I like this patch, thanks Kaspar!
r=kengert

Comment 30

[Kai Engert (:KaiE:)]

Patch v2 checked in to trunk.
Marking bug fixed.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 241512 [details] [diff] [review]
Fix template patch

approved for 1.8 branch, a=dveditz for drivers

Comment 32

[Kai Engert (:KaiE:)]

"Fix template patch" checked in to 1.8 branch