Last Comment Bug 400822 - Cert Viewer crashes when encountering improperly encoded GeneralNames (in AIA or CDP extensions) [@ ProcessGeneralName]
: Cert Viewer crashes when encountering improperly encoded GeneralNames (in AIA...
Status: VERIFIED FIXED
[fixed1.9.1b3]
: crash, fixed1.8.1.21, regression, topcrash, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9.2a1
Assigned To: Kaspar Brand
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
: 440553 472464 (view as bug list)
Depends on:
Blocks: 408547
  Show dependency treegraph
 
Reported: 2007-10-23 06:00 PDT by Kaspar Brand
Modified: 2011-06-09 14:58 PDT (History)
19 users (show)
sayrer: blocking1.9-
dveditz: wanted1.9.0.x+
mozilla: wanted1.8.1.x?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (11.09 KB, patch)
2007-10-23 06:00 PDT, Kaspar Brand
no flags Details | Diff | Splinter Review
Test certificate (1.02 KB, application/pkix-cert)
2007-10-23 06:02 PDT, Kaspar Brand
no flags Details
Proposed fix v2 (12.29 KB, patch)
2007-10-28 03:18 PDT, Kaspar Brand
no flags Details | Diff | Splinter Review
Another test certificate, with a broken CRL distribution point entry (833 bytes, application/pkix-cert)
2007-10-28 03:21 PDT, Kaspar Brand
no flags Details
Proposed fix v3 (minimal crash fix) [Checkin: Comment 11 & 19] (1012 bytes, patch)
2007-12-16 02:56 PST, Kaspar Brand
kaie: review+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review
(Cv1) Use NS_ENSURE_ARG_POINTER() [Checkin: Comment 33 & 34] (1.16 KB, patch)
2009-02-05 06:46 PST, Serge Gautherie (:sgautherie)
kaie: review+
Details | Diff | Splinter Review
patch for 1.8branch (using NS_ENSURE_ARG_POINTER macro) (803 bytes, patch)
2009-02-09 07:40 PST, Wolfgang Rosenauer [:wolfiR]
dveditz: approval1.9.0.9+
samuel.sidler+old: approval1.8.1.next+
Details | Diff | Splinter Review

Description Kaspar Brand 2007-10-23 06:00:40 PDT
Created attachment 285869 [details] [diff] [review]
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 Brand 2007-10-23 06:02:15 PDT
Created attachment 285871 [details]
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: foobar@example.net
--------------- 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: foobar@example.net
--------------- with patch ---------------
Comment 2 Kaspar Brand 2007-10-28 03:18:12 PDT
Created attachment 286465 [details] [diff] [review]
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 Brand 2007-10-28 03:21:19 PDT
Created attachment 286466 [details]
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 Brand 2007-10-28 03:23:51 PDT
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 2007-11-02 07:55:47 PDT
Would definitely take a patch, not going to block
Comment 6 Kai Engert (:kaie) (on vacation) 2007-11-07 05:33:46 PST
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 Brand 2007-12-16 02:56:17 PST
Created attachment 293373 [details] [diff] [review]
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 8 Dave Garrett 2008-06-19 15:24:36 PDT
*** Bug 440553 has been marked as a duplicate of this bug. ***
Comment 9 Wayne Mery (:wsmwk, NI for questions) 2008-12-13 20:20:27 PST
(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) (on vacation) 2008-12-15 10:05:35 PST
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) 2008-12-21 09:30:41 PST
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 2008-12-30 10:58:36 PST
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] 2008-12-30 23:28:49 PST
Comment on attachment 293373 [details] [diff] [review]
Proposed fix v3 (minimal crash fix)
[Checkin: Comment 11 & 19]

a191=beltzner
Comment 14 Serge Gautherie (:sgautherie) 2009-01-01 05:44:56 PST
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) 2009-01-01 21:46:26 PST
(In reply to comment #14)

Even s/NS_ENSURE_ARG/NS_ENSURE_ARG_POINTER/
Comment 16 Serge Gautherie (:sgautherie) 2009-02-04 10:24:52 PST
Kaspar, Kaie, what about comment 14 and comment 15 ?
Comment 17 Kaspar Brand 2009-02-04 22:24:27 PST
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) 2009-02-05 06:46:00 PST
Created attachment 360715 [details] [diff] [review]
(Cv1) Use NS_ENSURE_ARG_POINTER()
[Checkin: Comment 33 & 34]

Per previous comments.
(untested but trivial)
Comment 20 Wolfgang Rosenauer [:wolfiR] 2009-02-09 04:50:05 PST
*** Bug 472464 has been marked as a duplicate of this bug. ***
Comment 21 Wolfgang Rosenauer [:wolfiR] 2009-02-09 04:54:24 PST
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] 2009-02-09 05:55:43 PST
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) 2009-02-09 07:26:15 PST
Which version of NSS causes this? Also, can you work up a 1.8.1.x patch?
Comment 24 Wolfgang Rosenauer [:wolfiR] 2009-02-09 07:34:39 PST
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] 2009-02-09 07:40:54 PST
Created attachment 361279 [details] [diff] [review]
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) 2009-02-09 11:30:16 PST
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) 2009-02-09 11:51:45 PST
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] 2009-02-10 00:11:44 PST
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 2009-02-10 00:30:19 PST
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) 2009-02-10 07:48:28 PST
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) (on vacation) 2009-02-19 09:50:35 PST
Comment on attachment 360715 [details] [diff] [review]
(Cv1) Use NS_ENSURE_ARG_POINTER()
[Checkin: Comment 33 & 34]

r=kaie
Comment 32 Daniel Veditz [:dveditz] 2009-02-20 12:09:55 PST
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) 2009-02-20 18:18:35 PST
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) 2009-02-22 10:46:04 PST
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] 2009-03-17 23:34:00 PDT
Checked patch into the 190 branch.
Comment 36 Al Billings [:abillings] 2009-03-23 18:43:19 PDT
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] 2009-04-07 09:30:32 PDT
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

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