644012 - crash with an empty issuer name in SSL certificate, +leak fix [@ strcmp | AuthCertificateCallback(void*, PRFileDesc*, int, int)]

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Christophe]

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 (inactive)]

Bug 644038 is a dupe.  There's ~300 crash incidents reported so far.

Comment 3

[Kai Engert (:KaiE:)]

Sigh. Didn't expect this to happen.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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]]

(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:)]

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:)]

Attachment: Dump of empty issuer/subject cert

Comment 8

[Kai Engert (:KaiE:)]

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:)]

Attachment: 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 11

[Kai Engert (:KaiE:)]

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?)]

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:)]

Attachment: wrong file

Updated patch as requested.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Comment 15

[Kai Engert (:KaiE:)]

diff v3 vs v2 shows the line that Brian didn't like has been removed

Comment 16

[Daniel Veditz [:dveditz]]

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]]

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:)]

(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:)]

(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:)]

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:)]

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 25

[Daniel C]

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:)]

(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]]

Comment on attachment 521220 [details] [diff] [review]
Patch v3

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 28

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-2.0/rev/f747a1a341a0
http://hg.mozilla.org/projects/cedar/rev/e03e24a4f7fa

Comment 29

[Kai Engert (:KaiE:)]

cedar?
what about mozilla-central?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Cedar is merging to mozilla-central about once a day, so it'll land there soon.

Comment 31

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e03e24a4f7fa

Comment 33

[Kevin Brosnan [Ex-Mozilla]]

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