Closed Bug 644012 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Security, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla5
Tracking Status
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
fennec 4.0.1+ ---

People

(Reporter: c.devaux, Assigned: kaie)

References

(Blocks 1 open bug)

Details

(Keywords: crash, memory-leak, regression, Whiteboard: [sg:dos])

Crash Data

Attachments

(2 files, 2 obsolete files)

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.
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
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 ?
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: nobody → 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?
Keywords: crash
Attached patch Patch v2 (obsolete) — Splinter Review
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)
Duplicate of this bug: 643694
Keywords: mlk
Summary: crash with an empty issuer name in SSL certificate → crash with an empty issuer name in SSL certificate, +leak fix
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+
Attached patch wrong file (obsolete) — Splinter Review
Updated patch as requested.
Attachment #521156 - Attachment is obsolete: true
Attachment #521219 - Flags: review+
Attachment #521219 - Attachment description: Patch v3 → wrong file
Attachment #521219 - Attachment is obsolete: true
Attachment #521219 - Flags: review+
Attached patch Patch v3Splinter Review
Attachment #521220 - Flags: review+
diff v3 vs v2 shows the line that Brian didn't like has been removed
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+
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+
(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.
(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)
Group: core-security
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
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]
tracking-fennec: ? → 4.0.1+
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.
(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+
Flags: in-testsuite?
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.