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

VERIFIED FIXED in mozilla5

Status

()

Core
Security
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Christophe, Assigned: kaie)

Tracking

(Blocks: 1 bug, {crash, mlk, regression})

unspecified
mozilla5
crash, mlk, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 Macaw+, status2.0 .1-fixed, blocking1.9.2 .17+, status1.9.2 .17-fixed, blocking1.9.1 .19+, status1.9.1 .19-fixed, fennec4.0.1+)

Details

(Whiteboard: [sg:dos], crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Blocks: 642395
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
Bug 644038 is a dupe.  There's ~300 crash incidents reported so far.
Duplicate of this bug: 644038
(Assignee)

Comment 3

6 years ago
Sigh. Didn't expect this to happen.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #0)
> This is not exploitable and can not lead to a code execution.

Any reasons to keep the bug closed ?
(Assignee)

Comment 6

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

Comment 7

6 years ago
Created attachment 521155 [details]
Dump of empty issuer/subject cert
(Assignee)

Updated

6 years ago
Assignee: nobody → kaie
(Assignee)

Comment 8

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

Updated

6 years ago
Keywords: crash
(Assignee)

Comment 9

6 years ago
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.
Attachment #521156 - Flags: review?(bsmith)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 643694
(Assignee)

Updated

6 years ago
Keywords: mlk
Summary: crash with an empty issuer name in SSL certificate → crash with an empty issuer name in SSL certificate, +leak fix
(Assignee)

Comment 11

6 years ago
I've tested the patch builds and no longer crashes with the test site from comment 6.
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.
Attachment #521156 - Flags: review?(bsmith) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 521219 [details] [diff] [review]
wrong file

Updated patch as requested.
Attachment #521156 - Attachment is obsolete: true
Attachment #521219 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #521219 - Attachment description: Patch v3 → wrong file
Attachment #521219 - Attachment is obsolete: true
Attachment #521219 - Flags: review+
(Assignee)

Comment 14

6 years ago
Created attachment 521220 [details] [diff] [review]
Patch v3
Attachment #521220 - Flags: review+
(Assignee)

Comment 15

6 years ago
diff v3 vs v2 shows the line that Brian didn't like has been removed
(Assignee)

Updated

6 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
tracking-fennec: --- → ?
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.
blocking1.9.1: ? → .19+
blocking1.9.2: ? → .17+
blocking2.0: ? → .x+
status1.9.1: --- → wanted
status1.9.2: --- → wanted
status2.0: --- → wanted
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.
Attachment #521220 - Flags: approval1.9.2.17+
Attachment #521220 - Flags: approval1.9.1.19+
(Assignee)

Comment 18

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

Comment 19

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

Updated

6 years ago
Group: core-security
(Assignee)

Comment 20

6 years ago
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
status1.9.1: wanted → .19-fixed
status1.9.2: wanted → .17-fixed
(Assignee)

Comment 21

6 years ago
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

Updated

6 years ago
Blocks: 532972
Duplicate of this bug: 644354
Summary: crash with an empty issuer name in SSL certificate, +leak fix → crash with an empty issuer name in SSL certificate, +leak fix [@ strcmp | AuthCertificateCallback(void*, PRFileDesc*, int, int)]
Whiteboard: [sg:dos]

Updated

6 years ago
tracking-fennec: ? → 4.0.1+

Comment 25

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

Comment 26

6 years ago
(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
Keywords: checkin-needed
blocking2.0: .x+ → Macaw
Comment on attachment 521220 [details] [diff] [review]
Patch v3

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #521220 - Flags: approval2.0+

Updated

6 years ago
Flags: in-testsuite?
http://hg.mozilla.org/releases/mozilla-2.0/rev/f747a1a341a0
http://hg.mozilla.org/projects/cedar/rev/e03e24a4f7fa
status2.0: wanted → .1-fixed
Keywords: checkin-needed
Whiteboard: [sg:dos] → [sg:dos][fixed-in-cedar]
(Assignee)

Comment 29

6 years ago
cedar?
what about mozilla-central?
Cedar is merging to mozilla-central about once a day, so it'll land there soon.
Whiteboard: [sg:dos][fixed-in-cedar] → [sg:dos]
Target Milestone: --- → mozilla2.2
http://hg.mozilla.org/mozilla-central/rev/e03e24a4f7fa
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Duplicate of this bug: 649243
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
Status: RESOLVED → VERIFIED
Crash Signature: [@ strcmp | AuthCertificateCallback(void*, PRFileDesc*, int, int)]
Duplicate of this bug: 645108
You need to log in before you can comment on or make changes to this bug.