400822 - Cert Viewer crashes when encountering improperly encoded GeneralNames (in AIA or CDP extensions) [@ ProcessGeneralName]

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Kaspar]

Attachment: Proposed fix

Current trunk builds crash when trying to view a certificate with an improperly encoded OCSP responder URI. To reproduce the problem, open the "Authorities" tab in Cert Manager, scroll down to the "VeriSign Time Stamping Authority CA" cert (part of the builtin roots), and try to double click / open with the "View..." button.

The crash is due to an incorrectly encoded OCSP responder URI, which is explicitly tagged (instead of implicitly). pp shows it as

            Name: Authority Information Access
            Method: PKIX Online Certificate Status Protocol
            Location: [6]: {
                "http://ocsp.verisign.com/ocsp/status"
            }

It *should* look like this, however (with implicit tagging):

            Name: Authority Information Access
            Method: PKIX Online Certificate Status Protocol
            Location:
                URI: "http://ocsp.verisign.com/ocsp/status"

The crash occurs in ProcessGeneralName() in nsNSSCertHelper.cpp, when trying to dereference a null pointer ("current" is NULL in this case):

 996                   switch (current->type) {
 997                   case certOtherName: {

I'm proposing the attached patch, which checks for a null pointer at the beginning of ProcessGeneralName(). While we could also fix this specific issue by a check in ProcessAuthInfoAccess() (before calling ProcessGeneralName() with a null desc->location pointer), I believe it's the better (more robust) fix if we add a check at the beginning of ProcessGeneralName().

While we're at it, I also suggest to add some formatting improvements for the extension dumps (especially when displaying general names):

- indent general names when their content spans multiple lines

- don't add needless blank lines after a certDirectoryName

I'm going to attach another cert which can be used for testing (will also crash unpatched trunk builds) and for illustrating the formatting improvements.

Note that current branch versions (Fx/Tb 2.x, Sm 1.x) don't exhibit this problem - apparently it's the change to QuickDER (bug 178894) which triggers this regression. SEC_ASN1DecodeItem seems to be more tolerant when decoding general names. I'm therefore Ccing Nelson and Julien, in case they might be able to shed more light on this.

Comment 1

[Kaspar]

Attachment: Test certificate

This certificate includes an AIA extension with two OCSP responder URIs - one is implicitly tagged, while the other is explicitly tagged (will lead to Cert Viewer crashes on trunk). pp shows it as

            Name: Authority Information Access
            Method: PKIX Online Certificate Status Protocol
            Location:
                URI: "http://implicit.and.correct.example.net"
            Method: PKIX Online Certificate Status Protocol
            Location: [6]: {
                "http://explicit.and.wrong.example.net"
            }

Additionally, it includes a subjectAltName extension which illustrates the formatting changes the proposed patch would add. (There are two otherName entries - id-on-xmppAddr and id-on-dnsSRV - to show what happens with subjectAltName entries of types not yet known to PSM.)

--------------- without patch ---------------
Not Critical
X.500 Name: DC = foobar
DC = example
DC = net

1 3 6 1 5 5 7 8 5: Size: 20 Bytes / 160 Bits
0c 12 66 6f 6f 62 61 72 2e 65 78 61 6d 70 6c 65 
2e 6e 65 74 
DNS Name: foobar.example.net
1 3 6 1 5 5 7 8 7: Size: 19 Bytes / 152 Bits
16 11 5f 73 6d 74 70 2e 65 78 61 6d 70 6c 65 2e 
6e 65 74 
E-Mail Address: [e-mail]
--------------- without patch ---------------


--------------- with patch ---------------
Not Critical
X.500 Name: 
  DC = foobar
  DC = example
  DC = net
1.3.6.1.5.5.7.8.5: 
  Size: 20 Bytes / 160 Bits
  0c 12 66 6f 6f 62 61 72 2e 65 78 61 6d 70 6c 65 
  2e 6e 65 74 
DNS Name: foobar.example.net
1.3.6.1.5.5.7.8.7: 
  Size: 19 Bytes / 152 Bits
  16 11 5f 73 6d 74 70 2e 65 78 61 6d 70 6c 65 2e 
  6e 65 74 
E-Mail Address: [e-mail]
--------------- with patch ---------------

Comment 2

[Kaspar]

Attachment: Proposed fix v2

Earlier I wrote:

> While we could also fix this specific issue by a check in
> ProcessAuthInfoAccess() (before calling ProcessGeneralName() with a null
> desc->location pointer),

In the meantime, after another look at the ProcessGeneralName callers, I think that's actually the more appropriate way to fix this issue (instead of using the "CertDumpNULL" string in ProcessGeneralName itself).

More specifically: there are three callers of ProcessGeneralName (all in nsNSSCertHelper.cpp), and for two of them (in ProcessCrlDistPoints and ProcessAuthInfoAccess) we need to make sure that they don't call ProcessGeneralName with a null pointer (ProcessGeneralNames [note the plural "s"] doesn't exhibit this problem - so here's what v2 of the proposed fix does: if CERT_DecodeCRLDistributionPoints() or CERT_DecodeAuthInfoAccessExtension() don't return a parsed GeneralName, then we dump the raw DER data. The AIA extension in the "Verisign Time Stamping Authority CA" certificate is then shown as

----- snip -----
Not Critical
OCSP: 
  Size: 40 Bytes / 320 Bits
  a6 26 16 24 68 74 74 70 3a 2f 2f 6f 63 73 70 2e 
  76 65 72 69 73 69 67 6e 2e 63 6f 6d 2f 6f 63 73 
  70 2f 73 74 61 74 75 73 
----- snip -----

(The check at the beginning of ProcessGeneralName is still there, but now it simply returns with NS_ERROR_FAILURE, instead of inserting any display text).

Comment 3

[Kaspar]

Attachment: Another test certificate, with a broken CRL distribution point entry

This certificate illustrates the other case where an improperly encoded GeneralName is causing a crash in Cert Viewer. The second CDP entry (http://cdp2.example.net) is tagged explicitly instead of implicitly.

Comment 4

[Kaspar]

This should be blocking 1.9 - the crash happens when trying to view one of the builtin roots, and additionally it's also pretty easy to trigger this by installing a broken certificate on a server (the "Add Security Exception" dialog would also suffer from this, when people click the "View..." button).

Comment 5

[Robert Sayre]

Would definitely take a patch, not going to block

Comment 6

[Kai Engert [:KaiE:]]

Kaspar, please let's do critical crash fixes and display improvements in two separate patches and steps.

It's critical to fix crashes and reviewing and approving a small crash fix is quickly doable.

It will take me more time to review your improvements.

Comment 7

[Kaspar]

Attachment: Proposed fix v3 (minimal crash fix) [Checkin: Comment 11 & 19]

Ok, so I've spun off the improvements into a separate patch: see bug 408547. Kai, can you please keep that one on your radar, too? Thanks.

What's left for this bug here, then, is a straightforward two-line fix for the crash in ProcessGeneralName(): if we're called with a null pointer, then simply return with the appropriate NS_ERROR. This will cause the whole extension to be dumped in hex (even if there are other CDP or AIA entries which we could actually display - but again, bug 408547 takes care of this).

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to comment #6)
> It's critical to fix crashes and reviewing and approving a small crash fix is
> quickly doable.

won't make 1.9.1 without review

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 293373 [details] [diff] [review]
Proposed fix v3 (minimal crash fix)
[Checkin: Comment 11 & 19]

r=kaie

Thanks Kaspar

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 293373 [details] [diff] [review]
Proposed fix v3 (minimal crash fix)
[Checkin: Comment 11 & 19]

http://hg.mozilla.org/mozilla-central/rev/58cbf5d86442

Comment 12

[Simon Bünzli]

Comment on attachment 293373 [details] [diff] [review]
Proposed fix v3 (minimal crash fix)
[Checkin: Comment 11 & 19]

Can we get this fix for Firefox 3.1 as well? Considering today, we'll suddenly have significantly more people digging through their root certs...

Comment 13

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

Comment on attachment 293373 [details] [diff] [review]
Proposed fix v3 (minimal crash fix)
[Checkin: Comment 11 & 19]

a191=beltzner

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 293373 [details] [diff] [review]
Proposed fix v3 (minimal crash fix)
[Checkin: Comment 11 & 19]

> {
>   nsAutoString key;
>   nsXPIDLString value;
>   nsresult rv = NS_OK;
> 
>+  if (!current)
>+    return NS_ERROR_NULL_POINTER;
>+

Shouldn't this rather be:

 {
+  NS_ENSURE_ARG(current);
+
   nsAutoString key;

?

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)

Even s/NS_ENSURE_ARG/NS_ENSURE_ARG_POINTER/

Comment 16

[Serge Gautherie (:sgautherie)]

Kaspar, Kaie, what about comment 14 and comment 15 ?

Comment 17

[Kaspar]

It's a matter of coding style, IMO - I have neither strong feelings about this nor do I know if there are any style guidelines which should be followed for this case.

"NS_ENSURE_ARG_POINTER(current);" would be the correct choice, which actually expands to

do { if ((!!(!(current)))) { do { } while (0); return ((nsresult) 0x80004003L); } } while (0);

[This of course has exactly the same result as saying "if (!current) return ((nsresult) 0x80004003L);".]

Feel free to submit a new patch, but for the sake of consistency, it should then also be applied to trunk.

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Use NS_ENSURE_ARG_POINTER() [Checkin: Comment 33 & 34]

Per previous comments.
(untested but trivial)

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0aea9504e474

Comment 21

[Wolfgang Rosenauer [:wolfiR]]

Same crash happens on 1.9 and 1.8.1. I guess SM and TB should get that fix and so asked for wanted-1.9.0.x and wanted-1.8.1.x

Comment 22

[Wolfgang Rosenauer [:wolfiR]]

Ok, let me clarify something. This is marked as regression and that could be basically true. It probably does not happen with stock 1.8.1 and 1.9. The reason why I still want that to be approved for 1.9 and 1.8.1 is that it's a regression caused by an NSS update. So if you build/run 1.8.1 and 1.9 based mozilla applications against newer versions of NSS you run into that issue.
And it's very easy and safe fix anyway.

Comment 23

[Samuel Sidler (old account; do not CC)]

Which version of NSS causes this? Also, can you work up a 1.8.1.x patch?

Comment 24

[Wolfgang Rosenauer [:wolfiR]]

It's mentioned in the initial comment as being related to bug 178894. I'm just repeating what I've read there and according to the bug's metadata it seems to be NSS 3.12 but actually I haven't tried. Given that it probably affects FF3 as well but I haven't tried your builds yet.
Patch for 1.8.1 upcoming

Comment 25

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch for 1.8branch (using NS_ENSURE_ARG_POINTER macro)

Not really different but created against current 1.8 tip.

Comment 26

[Samuel Sidler (old account; do not CC)]

Comment on attachment 361279 [details] [diff] [review]
patch for 1.8branch (using NS_ENSURE_ARG_POINTER macro)

Approved for 1.8.1.21. a=ss for release-drivers.

Please land this in CVS and add the fixed1.8.1.21 keyword.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 23,
Any recent version of NSS should suffice to reproduce this.  
This bug is not caused by a bug in NSS.  It is correct for NSS's decoders
to store NULL pointers into the decoded structure for incorrectly encoded
or missing components.

Comment 28

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 361279 [details] [diff] [review]
patch for 1.8branch (using NS_ENSURE_ARG_POINTER macro)

I've checked and the patch for 1.9 is the very same as for 1.8.1 so I just set the approval? on the 1.8.1 patch.
I also recommend to change the 1.9.1/trunk patch to use the macro but wondering when kaie will be able to review that. Probably someone can just give Serge's change a go since it has the same meaning as the landed one anyway.

Comment 29

[Masahiro YAMADA]

Comment on attachment 361279 [details] [diff] [review]
patch for 1.8branch (using NS_ENSURE_ARG_POINTER macro)

Fx3.0.7 is already code freezed.
Can we land this patch to 1.9.0.7?

Comment 30

[Samuel Sidler (old account; do not CC)]

Comment on attachment 361279 [details] [diff] [review]
patch for 1.8branch (using NS_ENSURE_ARG_POINTER macro)

Yeah, too late for 1.9.0.7, but we'll get this in 1.9.0.8 when the tree opens.

Comment 31

[Kai Engert [:KaiE:]]

Comment on attachment 360715 [details] [diff] [review]
(Cv1) Use NS_ENSURE_ARG_POINTER()
[Checkin: Comment 33 & 34]

r=kaie

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 361279 [details] [diff] [review]
patch for 1.8branch (using NS_ENSURE_ARG_POINTER macro)

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 360715 [details] [diff] [review]
(Cv1) Use NS_ENSURE_ARG_POINTER()
[Checkin: Comment 33 & 34]


http://hg.mozilla.org/mozilla-central/rev/4959742e5439

Comment 34

[Serge Gautherie (:sgautherie)]

Comment on attachment 360715 [details] [diff] [review]
(Cv1) Use NS_ENSURE_ARG_POINTER()
[Checkin: Comment 33 & 34]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b6798ad69695

Comment 35

[Daniel Veditz [:dveditz]]

Checked patch into the 190 branch.

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.8 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8pre) Gecko/2009031805 GranParadiso/3.0.8pre (.NET CLR 3.5.30729). It no longer crashes with the test cert (though 1.9.0.7 does).

Comment 37

[Aakash Desai [:aakashd]]

Verified FIXED on builds: 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090406 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090406045355

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090331 Shiretoko/3.5b4pre (.NET CLR 3.5.30729) ID:20090331041754