The default bug view has changed. See this FAQ.

Certificate Viewer shows no details for certs in chain

RESOLVED FIXED

Status

()

Core
Security: PSM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: kaie)

Tracking

({fixed1.8.1.1})

1.8 Branch
x86
All
fixed1.8.1.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(11 attachments, 1 obsolete attachment)

11.21 KB, text/plain
Details
9.60 KB, image/gif
Details
14.22 KB, image/gif
Details
14.16 KB, image/gif
Details
17.84 KB, image/gif
Details
17.41 KB, image/gif
Details
1.85 KB, text/plain
Details
4.31 KB, text/plain
Details
1.05 KB, patch
kaie
: review+
Details | Diff | Splinter Review
1.45 KB, patch
kaie
: review+
Details | Diff | Splinter Review
38.45 KB, image/png
Details
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.
(Reporter)

Comment 1

11 years ago
Created attachment 241246 [details]
signed email message that demonstrates problems
(Reporter)

Comment 2

11 years ago
Created attachment 241248 [details]
message security dialog

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

Comment 3

11 years ago
Created attachment 241249 [details]
Cert viewer dialog with blank Cert Details pane for EE cert
(Reporter)

Comment 4

11 years ago
Created attachment 241250 [details]
Cert viewer dialog with blank Cert Details pane for CA cert
(Reporter)

Comment 5

11 years ago
Created attachment 241252 [details]
Correct Cert viewer CA display, after reopening cert viewer
(Reporter)

Comment 6

11 years ago
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

11 years ago
Same problem in Thunderbird 2.0a1 on Solaris x86, FWIW.
(Assignee)

Comment 8

11 years ago
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.

(Assignee)

Comment 9

11 years ago
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.
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Comment 11

11 years ago
Created attachment 241507 [details]
Certificate pretty print dump
(Assignee)

Comment 12

11 years ago
I confirm I get this on 1.8 branch, too, all platforms.
OS: Windows XP → All
Version: Trunk → 1.8 Branch
(Reporter)

Comment 13

11 years ago
(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.   
(Assignee)

Comment 14

11 years ago
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.
Attachment #241508 - Flags: review?(rrelyea)

Comment 15

11 years ago
Is there actually anything invalid about the cert? If so, can someone explain precisely what it is?
(Assignee)

Comment 16

11 years ago
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)
(Reporter)

Comment 17

11 years ago
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.
(Reporter)

Comment 18

11 years ago
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.
(Reporter)

Comment 19

11 years ago
Created attachment 241512 [details] [diff] [review]
Fix template patch

I think this should do the trick.
Attachment #241512 - Flags: review?(kengert)
(Assignee)

Comment 20

11 years ago
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?
Attachment #241512 - Attachment description: untested template patch → Fix template patch
Attachment #241512 - Flags: review?(kengert) → review+
(Reporter)

Comment 21

11 years ago
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

11 years ago
(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

11 years ago
(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.
(Reporter)

Comment 24

11 years ago
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

11 years ago
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

11 years ago
Created attachment 243170 [details]
MacDonnell cert display if "robustness patch" is applied (but w/o template fix)
(Reporter)

Comment 27

11 years ago
(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.
(Assignee)

Comment 28

11 years ago
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
Attachment #241512 - Flags: approval1.8.1.1?
(Assignee)

Updated

11 years ago
Attachment #241508 - Attachment is obsolete: true
Attachment #241508 - Flags: review?(rrelyea)
(Assignee)

Comment 29

11 years ago
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
Attachment #243169 - Attachment description: patch to make extension display in cert viewer more robust (against trunk) → Patch v2 - make extension display in cert viewer more robust (against trunk)
Attachment #243169 - Flags: review+
(Assignee)

Comment 30

11 years ago
Patch v2 checked in to trunk.
Marking bug fixed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 241512 [details] [diff] [review]
Fix template patch

approved for 1.8 branch, a=dveditz for drivers
Attachment #241512 - Flags: approval1.8.1.1? → approval1.8.1.1+
(Assignee)

Comment 32

11 years ago
"Fix template patch" checked in to 1.8 branch
Keywords: fixed1.8.1.1
You need to log in before you can comment on or make changes to this bug.