Last Comment Bug 644012 - crash with an empty issuer name in SSL certificate, +leak fix [@ strcmp | AuthCertificateCallback(void*, PRFileDesc*, int, int)]
: crash with an empty issuer name in SSL certificate, +leak fix [@ strcmp | Aut...
Status: VERIFIED FIXED
[sg:dos]
: crash, mlk, regression
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla5
Assigned To: Kai Engert (:kaie)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
: 643694 644038 644354 645108 649243 (view as bug list)
Depends on:
Blocks: 532972 642395
  Show dependency treegraph
 
Reported: 2011-03-22 18:31 PDT by Christophe
Modified: 2011-10-10 16:55 PDT (History)
20 users (show)
djcater+bugzilla: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
Macaw+
.1-fixed
.17+
.17-fixed
.19+
.19-fixed
4.0.1+


Attachments
Dump of empty issuer/subject cert (2.63 KB, text/plain)
2011-03-23 05:31 PDT, Kai Engert (:kaie)
no flags Details
Patch v2 (1.91 KB, patch)
2011-03-23 05:51 PDT, Kai Engert (:kaie)
brian: review+
Details | Diff | Splinter Review
wrong file (1.91 KB, patch)
2011-03-23 10:02 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
Patch v3 (1.88 KB, patch)
2011-03-23 10:04 PDT, Kai Engert (:kaie)
kaie: review+
dveditz: approval2.0+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description Christophe 2011-03-22 18:31:34 PDT
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0) Gecko/20110321 Firefox/4.0
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0) Gecko/20110321 Firefox/4.0

Firefox crashes when trying to access a HTTPS website with a certificate that does not contain the fields issuerName.

Reproducible: Always

Steps to Reproduce:
1. Generate an SSL certificate where issuerName is null.
2. Use this certificate on a website.
3. Try to access this website with Firefox 4.0.
Actual Results:  
Firefox crashes.

Expected Results:  
Firefox displays the page.

% openssl x509 -in goodcert.pem -noout -issuer
issuer= /C=US/O=Equifax/OU=Equifax Secure Certificate Authority
% openssl x509 -in badcert.pem -noout issuer
issuer= 

backtrace:

(gdb) bt
#0  0x00007ffff73cd2d6 in strcmp () from /lib/libc.so.6
#1  0x00007ffff55391c4 in ?? () from /usr/lib/xulrunner-2.0/libxul.so
#2  0x00007ffff3efc4bc in ssl3_HandleHandshakeMessage () from /usr/lib/libssl3.so
#3  0x00007ffff3efdd7c in ssl3_HandleRecord () from /usr/lib/libssl3.so
#4  0x00007ffff3efee36 in ssl3_GatherCompleteHandshake () from /usr/lib/libssl3.so
#5  0x00007ffff3f006a1 in ssl_GatherRecord1stHandshake () from /usr/lib/libssl3.so
#6  0x00007ffff3f071f5 in ssl_Do1stHandshake () from /usr/lib/libssl3.so
#7  0x00007ffff3f088a8 in ssl_SecureSend () from /usr/lib/libssl3.so
#8  0x00007ffff3f0c842 in ssl_Write () from /usr/lib/libssl3.so
#9  0x00007ffff5535fa7 in ?? () from /usr/lib/xulrunner-2.0/libxul.so
#10 0x00007ffff63632a3 in ?? () from /usr/lib/libnspr4.so
#11 0x00007ffff7bc8cb0 in start_thread () from /lib/libpthread.so.0
#12 0x00007ffff742695d in clone () from /lib/libc.so.6
#13 0x0000000000000000 in ?? ()

The bug is caused by a call to the strcmp() function with a null argument, in function AuthCertificateCallback(), file security/manager/ssl/src/nsNSSCallbacks.cpp:

SECStatus PR_CALLBACK AuthCertificateCallback(void* client_data, PRFileDesc* fd,
                                              PRBool checksig, PRBool isServer) {
  nsNSSShutDownPreventionLock locker;

  CERTCertificate *serverCert = SSL_PeerCertificate(fd);
  if (serverCert && 
      serverCert->serialNumber.data &&
      !strcmp(serverCert->issuerName, 
        "CN=UTN-USERFirst-Hardware,OU=http://www.usertrust.com,O=The USERTRUST Network,L=Salt Lake City,ST=UT,C=US")) {

This is not exploitable and can not lead to a code execution.
Comment 1 Mats Palmgren (:mats) 2011-03-22 21:54:17 PDT
Bug 644038 is a dupe.  There's ~300 crash incidents reported so far.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-03-22 21:55:54 PDT
*** Bug 644038 has been marked as a duplicate of this bug. ***
Comment 3 Kai Engert (:kaie) 2011-03-22 22:33:57 PDT
Sigh. Didn't expect this to happen.
Comment 4 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-03-23 00:21:37 PDT
I suspect this will rarely (if ever) occur for certs issued by public CAs we trust, but lots of users run into certs that aren't issued by them. The additional check for NULL is necessary & sufficient, and won't cause any regressions. We should fix this for the first Firefox 4 update and for the next 3.x releases.
Comment 5 Ludovic Hirlimann [:Usul] 2011-03-23 00:45:05 PDT
(In reply to comment #0)
> This is not exploitable and can not lead to a code execution.

Any reasons to keep the bug closed ?
Comment 6 Kai Engert (:kaie) 2011-03-23 05:31:09 PDT
Bug 644038 has more info.

Happening with Cisco Wifi routers.
Some people cannot connect to the web at all, from within their corporate network, because of a mandatory redirect through a landing/login page.

Sample URL:
https://81.12.49.108/IBSng/user

I'm attaching the exported cert and its dump (just FYI).
Comment 7 Kai Engert (:kaie) 2011-03-23 05:31:54 PDT
Created attachment 521155 [details]
Dump of empty issuer/subject cert
Comment 8 Kai Engert (:kaie) 2011-03-23 05:47:56 PDT
Can we use this bug to fix both the crash and the leak?

Can we mark bug 643694 as a dupe of this one, 
and transfer approvals to this one?
Comment 9 Kai Engert (:kaie) 2011-03-23 05:51:09 PDT
Created attachment 521156 [details] [diff] [review]
Patch v2

For the leak fix, addressing Brian's comment from bug 642395 comment 70.

For the crash fix, checking issuerName for non-null, and also, just in case, adding a check to make sure serial numbers have a non-zero length.
Comment 10 Kai Engert (:kaie) 2011-03-23 05:52:16 PDT
*** Bug 643694 has been marked as a duplicate of this bug. ***
Comment 11 Kai Engert (:kaie) 2011-03-23 05:54:33 PDT
I've tested the patch builds and no longer crashes with the test site from comment 6.
Comment 12 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-03-23 09:25:30 PDT
Comment on attachment 521156 [details] [diff] [review]
Patch v2

Please remove the new check that serverCert->serialNumber.len is not zero. The current code will work fine, using the same code path as the case where the cert serial number is all zero bytes. 

r+ with that change.
Comment 13 Kai Engert (:kaie) 2011-03-23 10:02:45 PDT
Created attachment 521219 [details] [diff] [review]
wrong file

Updated patch as requested.
Comment 14 Kai Engert (:kaie) 2011-03-23 10:04:36 PDT
Created attachment 521220 [details] [diff] [review]
Patch v3
Comment 15 Kai Engert (:kaie) 2011-03-23 10:05:18 PDT
diff v3 vs v2 shows the line that Brian didn't like has been removed
Comment 16 Daniel Veditz [:dveditz] 2011-03-23 10:40:16 PDT
This is just a null check, is there really a security problem here? I'd like to un-hide the bug unless I'm completely misunderstanding the problem.
Comment 17 Daniel Veditz [:dveditz] 2011-03-23 10:43:00 PDT
Comment on attachment 521220 [details] [diff] [review]
Patch v3

Approved for 1.9.2.17 and 1.9.1.19, a=dveditz for release-drivers

I know it's the end of your day already, but if you have time to land it today that would be great because we're supposed code-freeze. Early your morning would be OK too.
Comment 18 Kai Engert (:kaie) 2011-03-23 10:46:55 PDT
(In reply to comment #16)
> This is just a null check, is there really a security problem here? I'd like to
> un-hide the bug unless I'm completely misunderstanding the problem.

I think the bug was originally hidden because the patch / stack trace contained the name of the CA.

Since the issue is now public, I think it should be OK to open this bug.
Comment 19 Kai Engert (:kaie) 2011-03-23 10:49:01 PDT
(In reply to comment #17)
> 
> I know it's the end of your day already, but if you have time to land it today
> that would be great because we're supposed code-freeze. Early your morning
> would be OK too.


I can land on mozilla-1.9.1 and mozilla-1.9.2 soon (today)
Comment 20 Kai Engert (:kaie) 2011-03-23 12:36:46 PDT
fixed on branches

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fe344625dee8
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cc1618c6d434


not yet on mozilla-central
Comment 21 Kai Engert (:kaie) 2011-03-23 12:37:55 PDT
Do I need to request approval for Firefox 4 (mozilla-2.0) and Fennec 4 (mozilla-2.1) separately?

Can't find patch approval flags
Comment 23 Matthias Versen [:Matti] 2011-03-23 15:57:49 PDT
*** Bug 644354 has been marked as a duplicate of this bug. ***
Comment 25 Daniel Cater 2011-04-05 11:00:31 PDT
Does this still need to land on mozilla-central, 2.1 and 2.0? I thought patches were meant to land in that order anyway, and then 1.9.2, 1.9.1.
Comment 26 Kai Engert (:kaie) 2011-04-05 11:45:11 PDT
(In reply to comment #25)
> Does this still need to land on mozilla-central, 2.1 and 2.0?

Yes


> I thought patches
> were meant to land in that order anyway, and then 1.9.2, 1.9.1.

yes, usually
Comment 27 Daniel Veditz [:dveditz] 2011-04-05 14:13:52 PDT
Comment on attachment 521220 [details] [diff] [review]
Patch v3

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Comment 29 Kai Engert (:kaie) 2011-04-06 14:24:13 PDT
cedar?
what about mozilla-central?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2011-04-06 15:28:03 PDT
Cedar is merging to mozilla-central about once a day, so it'll land there soon.
Comment 32 timeless 2011-04-11 23:49:12 PDT
*** Bug 649243 has been marked as a duplicate of this bug. ***
Comment 33 Kevin Brosnan [:kbrosnan] 2011-04-19 17:26:51 PDT
Verified

Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110419 Firefox/6.0a1
Fennec/6.0a1 ID:20110419042214

Mozilla/5.0 (Android; Linux armv7l; rv:2.1.1) Gecko/20110415 Firefox/4.0.2pre
Fennec/4.0.1 ID:20110415172201

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110419 Firefox/6.0a1
Comment 34 Marcia Knous [:marcia - use ni] 2011-10-10 16:55:14 PDT
*** Bug 645108 has been marked as a duplicate of this bug. ***

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