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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Security: PSM
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Kaspar Brand, Assigned: Kaspar Brand)

Tracking

(6 keywords)

Trunk
mozilla1.9.2a1
crash, fixed1.8.1.21, regression, topcrash, verified1.9.0.9, verified1.9.1
Points:
---
Bug Flags:
blocking1.9 -
wanted1.9.0.x +
wanted1.8.1.x ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.1b3], crash signature)

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
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.
Attachment #285869 - Flags: review?(kengert)
(Assignee)

Comment 1

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

Comment 2

10 years ago
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).
Attachment #285869 - Attachment is obsolete: true
Attachment #286465 - Flags: review?(kengert)
Attachment #285869 - Flags: review?(kengert)
(Assignee)

Comment 3

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

Comment 4

10 years ago
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).
Flags: blocking1.9?
Summary: Cert Viewer crashes when encountering an improperly encoded AIA extension → Cert Viewer crashes when encountering improperly encoded GeneralNames (in AIA or CDP extensions)

Comment 5

10 years ago
Would definitely take a patch, not going to block
Flags: blocking1.9? → blocking1.9-

Comment 6

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

Updated

10 years ago
Blocks: 408547
(Assignee)

Comment 7

10 years ago
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).
Assignee: kengert → mozbugzilla
Attachment #286465 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #293373 - Flags: review?(kengert)
Attachment #286465 - Flags: review?(kengert)

Updated

9 years ago
Keywords: topcrash
Summary: Cert Viewer crashes when encountering improperly encoded GeneralNames (in AIA or CDP extensions) → Cert Viewer crashes when encountering improperly encoded GeneralNames (in AIA or CDP extensions) [@ ProcessGeneralName]

Updated

9 years ago
Duplicate of this bug: 440553
(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

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

r=kaie

Thanks Kaspar
Attachment #293373 - Flags: review?(kaie) → review+

Updated

9 years ago
Keywords: checkin-needed
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
Attachment #293373 - Attachment description: Proposed fix v3 (minimal crash fix) → Proposed fix v3 (minimal crash fix) [Checkin: Comment 11]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1

Updated

8 years ago
Attachment #293373 - Flags: approval1.9.1?

Comment 12

8 years ago
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 on attachment 293373 [details] [diff] [review]
Proposed fix v3 (minimal crash fix)
[Checkin: Comment 11 & 19]

a191=beltzner
Attachment #293373 - Flags: approval1.9.1? → approval1.9.1+

Updated

8 years ago
Keywords: checkin-needed
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;

?
(In reply to comment #14)

Even s/NS_ENSURE_ARG/NS_ENSURE_ARG_POINTER/
Whiteboard: [c-n: baking for 1.9.1, but wait for answer to comment 14+15]
Kaspar, Kaie, what about comment 14 and comment 15 ?
(Assignee)

Comment 17

8 years ago
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.
Assignee: mozbugzilla → nobody
Created attachment 360715 [details] [diff] [review]
(Cv1) Use NS_ENSURE_ARG_POINTER()
[Checkin: Comment 33 & 34]

Per previous comments.
(untested but trivial)
Attachment #360715 - Flags: review?(kaie)
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0aea9504e474
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [c-n: baking for 1.9.1, but wait for answer to comment 14+15]
Attachment #293373 - Attachment description: Proposed fix v3 (minimal crash fix) [Checkin: Comment 11] → Proposed fix v3 (minimal crash fix) [Checkin: Comment 11 & 19]
Duplicate of this bug: 472464
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
Flags: wanted1.9.0.x?
Flags: wanted1.8.1.x?
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.
Which version of NSS causes this? Also, can you work up a 1.8.1.x patch?
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
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.
Attachment #361279 - Flags: approval1.8.1.next?
Flags: wanted1.9.0.x? → wanted1.9.0.x+
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.
Attachment #361279 - Flags: approval1.8.1.next? → approval1.8.1.next+
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.
Keywords: fixed1.8.1.21
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.
Attachment #361279 - Flags: approval1.9.0.7?

Comment 29

8 years ago
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 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.
Attachment #361279 - Flags: approval1.9.0.7? → approval1.9.0.8?

Comment 31

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

r=kaie
Attachment #360715 - Flags: review?(kaie) → review+
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
Attachment #361279 - Flags: approval1.9.0.8? → approval1.9.0.8+
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
Attachment #360715 - Attachment description: (Cv1) Use NS_ENSURE_ARG_POINTER() → (Cv1) Use NS_ENSURE_ARG_POINTER() [Checkin: Comment 33]
Assignee: nobody → mozbugzilla
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
Attachment #360715 - Attachment description: (Cv1) Use NS_ENSURE_ARG_POINTER() [Checkin: Comment 33] → (Cv1) Use NS_ENSURE_ARG_POINTER() [Checkin: Comment 33 & 34]
Whiteboard: [fixed1.9.1b3]
Checked patch into the 190 branch.
Keywords: fixed1.9.0.8
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).
Keywords: fixed1.9.0.8 → verified1.9.0.8
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Crash Signature: [@ ProcessGeneralName]
You need to log in before you can comment on or make changes to this bug.