Last Comment Bug 355447 - Certificate Viewer shows no details for certs in chain
: Certificate Viewer shows no details for certs in chain
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: 1.8 Branch
: x86 All
: -- normal (vote)
: ---
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-04 16:17 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2006-11-30 00:46 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
signed email message that demonstrates problems (11.21 KB, text/plain)
2006-10-04 16:20 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
message security dialog (9.60 KB, image/gif)
2006-10-04 16:29 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
Cert viewer dialog with blank Cert Details pane for EE cert (14.22 KB, image/gif)
2006-10-04 16:30 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
Cert viewer dialog with blank Cert Details pane for CA cert (14.16 KB, image/gif)
2006-10-04 16:31 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
Correct Cert viewer CA display, after reopening cert viewer (17.84 KB, image/gif)
2006-10-04 16:32 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
incorrect EE Cert Viewer display, after reopening cert viewer (17.41 KB, image/gif)
2006-10-04 16:34 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
Certificate in PEM format (1.85 KB, text/plain)
2006-10-06 17:02 PDT, Kai Engert (:kaie)
no flags Details
Certificate pretty print dump (4.31 KB, text/plain)
2006-10-06 17:03 PDT, Kai Engert (:kaie)
no flags Details
Patch v1 (999 bytes, patch)
2006-10-06 17:26 PDT, Kai Engert (:kaie)
no flags Details | Diff | Review
Fix template patch (1.05 KB, patch)
2006-10-06 18:12 PDT, Nelson Bolyard (seldom reads bugmail)
kaie: review+
dveditz: approval1.8.1.1+
Details | Diff | Review
Patch v2 - make extension display in cert viewer more robust (against trunk) (1.45 KB, patch)
2006-10-23 03:03 PDT, Kaspar Brand
kaie: review+
Details | Diff | Review
MacDonnell cert display if "robustness patch" is applied (but w/o template fix) (38.45 KB, image/png)
2006-10-23 03:03 PDT, Kaspar Brand
no flags Details

Description Nelson Bolyard (seldom reads bugmail) 2006-10-04 16:17:36 PDT
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) 2006-10-04 16:20:23 PDT
Created attachment 241246 [details]
signed email message that demonstrates problems
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-10-04 16:29:00 PDT
Created attachment 241248 [details]
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) 2006-10-04 16:30:13 PDT
Created attachment 241249 [details]
Cert viewer dialog with blank Cert Details pane for EE cert
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-10-04 16:31:09 PDT
Created attachment 241250 [details]
Cert viewer dialog with blank Cert Details pane for CA cert
Comment 5 Nelson Bolyard (seldom reads bugmail) 2006-10-04 16:32:32 PDT
Created attachment 241252 [details]
Correct Cert viewer CA display, after reopening cert viewer
Comment 6 Nelson Bolyard (seldom reads bugmail) 2006-10-04 16:34:58 PDT
Created attachment 241254 [details]
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 2006-10-04 17:09:31 PDT
Same problem in Thunderbird 2.0a1 on Solaris x86, FWIW.
Comment 8 Kai Engert (:kaie) 2006-10-06 16:51:29 PDT
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) 2006-10-06 16:55:51 PDT
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) 2006-10-06 17:02:26 PDT
Created attachment 241506 [details]
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) 2006-10-06 17:03:06 PDT
Created attachment 241507 [details]
Certificate pretty print dump
Comment 12 Kai Engert (:kaie) 2006-10-06 17:03:35 PDT
I confirm I get this on 1.8 branch, too, all platforms.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2006-10-06 17:08:22 PDT
(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) 2006-10-06 17:26:27 PDT
Created attachment 241508 [details] [diff] [review]
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 2006-10-06 17:29:49 PDT
Is there actually anything invalid about the cert? If so, can someone explain precisely what it is?
Comment 16 Kai Engert (:kaie) 2006-10-06 17:35:32 PDT
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) 2006-10-06 18:01:53 PDT
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) 2006-10-06 18:06:01 PDT
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) 2006-10-06 18:12:25 PDT
Created attachment 241512 [details] [diff] [review]
Fix template patch

I think this should do the trick.
Comment 20 Kai Engert (:kaie) 2006-10-06 19:21:21 PDT
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) 2006-10-06 22:34:35 PDT
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 2006-10-09 01:47:54 PDT
(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 2006-10-09 02:10:25 PDT
(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) 2006-10-09 08:16:11 PDT
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 Brand 2006-10-23 03:03:01 PDT
Created attachment 243169 [details] [diff] [review]
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 Brand 2006-10-23 03:03:48 PDT
Created attachment 243170 [details]
MacDonnell cert display if "robustness patch" is applied (but w/o template fix)
Comment 27 Nelson Bolyard (seldom reads bugmail) 2006-10-25 11:29:50 PDT
(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) 2006-11-02 13:51:04 PST
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) 2006-11-02 14:38:06 PST
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) 2006-11-02 14:48:59 PST
Patch v2 checked in to trunk.
Marking bug fixed.
Comment 31 Daniel Veditz [:dveditz] 2006-11-29 11:38:16 PST
Comment on attachment 241512 [details] [diff] [review]
Fix template patch

approved for 1.8 branch, a=dveditz for drivers
Comment 32 Kai Engert (:kaie) 2006-11-30 00:46:52 PST
"Fix template patch" checked in to 1.8 branch

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