403691 - unable to display decoded policies extension in certs from https://www.digicert.com

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

When playing with PKIX_VerifyCert, I tested the certs used on https://www.digicert.com
I see several problems.


A)
NSS or PSM are unable to decode the certificate policies field.
Usually (for other certs from other sites) we get a nice human readable dump of the policy extension.

With server and intermediate certs all I get is a hex dump.

(Strange. I know I recently was able to view the cert on that site. But now, Firefox 2 with NSS 3.11.x is unable to display the site's cert at all! It complains about an invalid asn1. Either that cert is completely broken, or we are unable to process it.)


B)
PKIX_VerifyCert fails to verify the cert, when asking for policy 2.16.840.1.114412.2.1 (probably because it can not decode it).


C)
After having called PKIX_VerifyCert, PSM is no longer able to display the cert cert at all, it runs into an exception. This is probably a follow up error from A) or B).

Comment 1

[Kai Engert (:KaiE:)]

Attachment: cert: server

Comment 2

[Kai Engert (:KaiE:)]

Attachment: cert: intermediate

Comment 3

[Kai Engert (:KaiE:)]

Attachment: cert: root

Comment 4

[Kai Engert (:KaiE:)]

Attachment: cert: intermediate

(I meant to attach a human readable dump of the intermediate...)

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

> Firefox 2 with NSS 3.11.x is unable to display the site's cert at all!
So, is this really a trunk bug? or is it a branch bug?
Has the site's cert changed recently?

How were the attached readable cert dumps produced?  What code? What version?

Comment 6

[Kai Engert (:KaiE:)]

This is a trunk bug.

(While there might be strange behavior on branch, we should fix trunk first, IMHO.)

The PEM dumps were produced with firefox + 3.12 alpha 2 trunk, using cert viewer and the new "export" button.

The human readable dumps were produced using "openssl x509 -text", I beg your pardon, this was the quickest way to do it ;-)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Our policy is that bugs are filed against the earliest (lowest numbered)
version of NSS in which they can be reproduced.  Comment 0 seems to say
that is 3.11.x, not the trunk.  It's important for us to know if this 
problem is somehow part of PKIX (in which case it cannot be a 3.11.x bug)
or if it is seen without use of libPKIX (as seems to be the case, if it 
can be seen in FF2).

Comment 8

[Kai Engert (:KaiE:)]

(In reply to comment #7)
> Our policy is that bugs are filed against the earliest (lowest numbered)
> version of NSS in which they can be reproduced.  Comment 0 seems to say
> that is 3.11.x, not the trunk.  It's important for us to know if this 
> problem is somehow part of PKIX (in which case it cannot be a 3.11.x bug)
> or if it is seen without use of libPKIX (as seems to be the case, if it 
> can be seen in FF2).


The behavior is different between trunk and 3.11

I think both require separate analysis.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Kai, since you already have them, please also attach the binary certs to this 
bug. 

Comment 10

[Kai Engert (:KaiE:)]

I didn't export binary certs, but certs in ascii PEM format.

Not sure why you need the binary files, the NSS tools are able to process the attached files just fine, if you give them the -a flag.

For example,
  pp -i /tmp/www.digicert.com.dump -t certificate -a
or
  certutil -d . -A -a -i /tmp/www.digicert.com.dump -n digicert -t ,,
both work for me.

Comment 11

[Kai Engert (:KaiE:)]

That display text looks like a 16 bit encoding?

            Name: Certificate Policies
            Data:
                Policy Name: OID.2.16.840.1.114412.2.1
                    Policy Qualifier Name: PKIX CPS Pointer Qualifier
                    Policy Qualifier Data: "http://www.digicert.com/ssl-cps-r
                        epository.htm"
                    Policy Qualifier Name: PKIX User Notice Qualifier
                        Display Text: ".n.y. .u.s.e. .o.f. .t.h.i.s. .C.e.r.t
                            .i.f.i.c.a.t.e. .c.o.n.s.t.i.t.u.t.e.s. .a.c.c.e.
                            p.t.a.n.c.e. .o.f. .t.h.e. .D.i.g.i.C.e.r.t. .E.V
                            . .C.P.S. .a.n.d. .t.h.e. .R.e.l.y.i.n.g. .P.a.r.
                            t.y. .A.g.r.e.e.m.e.n.t. .w.h.i.c.h. .l.i.m.i.t.
                            .l.i.a.b.i.l.i.t.y. .a.n.d. .a.r.e. .i.n.c.o.r.p.
                            o.r.a.t.e.d. .h.e.r.e.i.n. .b.y. .r.e.f.e.r.e.n.c
                            .e.."

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Kai, thanks for the reminder that the attachments include the base64 encoded
certs.  I can get the DER certs from those.  I need the DER certs to examine
their encoding to see if it is correct.

The text format you see there is Unicode, UTF-16 (formerly known as UCS2) 
Big-Endian format (everything in DER encoding is Big Endian).  

Am I right in thinking that you produced the output in comment 11 using 
NSS's own tools, certutil or pp ?

Some questions to investigate:

1) is this UTF16 correctly encoded in the DER cert? 
or, is it UTF-16 data encoded as if it were UTF8 or some other encoding?
(Determination requires examination of the DER cert, which is why...)

2) Do the standards allow these "Display Text" strings to be UTF-16 strings?
(I'd guess that they do.)

3) Is there code somewhere in NSS or PSM that erroneously assumes that the 
strings are UTF8 or ASCII, and does not handle UCS2 or UCS4 ?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

After examining the DER cert and RFC 3280, it appears (not surprisingly) that
the display text is a correctly encoded DMPString (which is UTF16), and that
display text is permitted to be a BMPString.

Comment 14

[Kaspar]

Attachment: Patch for PSM's ProcessUserNotice()

This is a regression introduced by bug 324744, which fixed problems with CERT_DecodeUserNotice (see bug 324744 comment 26, in particular).

The issue now showing up with the DigiCert certificates (or any certificate including a userNotice in its policy extension, for that matter) has to be addressed in PSM, actually. What Kai reports in comment 0 is a problem of the code in nsNSSCertHelper.cpp, which is no longer compatible with the current version of CERT_DecodeUserNotice in NSS.

The attached patch cleans up the ProcessUserNotice() function in PSM - it no longer has to decode the displayText in with its own template, since NSS is already doing this.

Contrary to what Kai writes in the original report, I'm not able to reproduce this with NSS 3.11 (Firefox 2). From looking at the code, it would also be rather surprising if this problem would show up there, too.

Comment 15

[Kaspar]

Attachment: Test certificate (includes all permissible string encodings for DisplayText)

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 289325 [details] [diff] [review]
Patch for PSM's ProcessUserNotice()

I can't comment on the NSS changes and the required changes on the decoding strategy.

I hope Nelson will speak up if he sees concerns about your decoding strategy.

Your code looks good to. I tried it and it seems to work.

r=kengert

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289325 [details] [diff] [review]
Patch for PSM's ProcessUserNotice()

looks right to me, too.

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 289325 [details] [diff] [review]
Patch for PSM's ProcessUserNotice()

requesting review, this would help our EV story

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 289325 [details] [diff] [review]
Patch for PSM's ProcessUserNotice()

a=beltzner for 1.9

Comment 20

[Kai Engert (:KaiE:)]

Changing component, as the fix is in PSM.

Comment 21

[Kai Engert (:KaiE:)]

Attachment: Patch for check in

When I compile the reviewed and approved patch, I get a lot of warnings:
- complaints about unhandled enums in switch
- complaints about an unused variable.

The fixes are simple, I added a "default: break;" to 2 switch statements, and removed an unused "nsresult rv;" variable.

Comment 22

[Kai Engert (:KaiE:)]

Patch checked in.

Thanks Kaspar for the fix!

Comment 23

[Kaspar]

(In reply to comment #21)
> When I compile the reviewed and approved patch, I get a lot of warnings:
> - complaints about unhandled enums in switch
> - complaints about an unused variable.

Thanks for fixing these - I didn't see them because I was compiling with MSVC8, which is only called with -W3 when building Mozilla (in contrast to -Wall when using GCC). If I turn on -Wall for MSVC and compile nsNSSCertHelper.cpp, on the other hand, then I'm getting tons of additional warnings... MSVC can apparently be even more pedantic than GCC, if desired.