Closed
Bug 644012
Opened 13 years ago
Closed 13 years ago
crash with an empty issuer name in SSL certificate, +leak fix [@ strcmp | AuthCertificateCallback(void*, PRFileDesc*, int, int)]
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
VERIFIED
FIXED
mozilla5
People
(Reporter: c.devaux, Assigned: KaiE)
References
Details
(Keywords: crash, memory-leak, regression, Whiteboard: [sg:dos])
Crash Data
Attachments
(2 files, 2 obsolete files)
2.63 KB,
text/plain
|
Details | |
1.88 KB,
patch
|
KaiE
:
review+
dveditz
:
approval2.0+
dveditz
:
approval1.9.2.17+
dveditz
:
approval1.9.1.19+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Comment 1•13 years ago
|
||
Bug 644038 is a dupe. There's ~300 crash incidents reported so far.
Assignee | ||
Comment 3•13 years ago
|
||
Sigh. Didn't expect this to happen.
Comment 4•13 years ago
|
||
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
Comment 5•13 years ago
|
||
(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•13 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•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → kaie
Assignee | ||
Comment 8•13 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 | ||
Comment 9•13 years ago
|
||
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•13 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•13 years ago
|
||
I've tested the patch builds and no longer crashes with the test site from comment 6.
Comment 12•13 years ago
|
||
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•13 years ago
|
||
Updated patch as requested.
Attachment #521156 -
Attachment is obsolete: true
Attachment #521219 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Attachment #521219 -
Attachment description: Patch v3 → wrong file
Attachment #521219 -
Attachment is obsolete: true
Attachment #521219 -
Flags: review+
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #521220 -
Flags: review+
Assignee | ||
Comment 15•13 years ago
|
||
diff v3 vs v2 shows the line that Brian didn't like has been removed
Assignee | ||
Updated•13 years ago
|
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Updated•13 years ago
|
tracking-fennec: --- → ?
Comment 16•13 years ago
|
||
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
Comment 17•13 years ago
|
||
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•13 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•13 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•13 years ago
|
Group: core-security
Assignee | ||
Comment 20•13 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
Assignee | ||
Comment 21•13 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•13 years ago
|
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)]
Updated•13 years ago
|
Whiteboard: [sg:dos]
Updated•13 years ago
|
tracking-fennec: ? → 4.0.1+
Comment 25•13 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•13 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
Updated•13 years ago
|
blocking2.0: .x+ → Macaw
Comment 27•13 years ago
|
||
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+
Comment 28•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-2.0/rev/f747a1a341a0 http://hg.mozilla.org/projects/cedar/rev/e03e24a4f7fa
Assignee | ||
Comment 29•13 years ago
|
||
cedar? what about mozilla-central?
Comment 30•13 years ago
|
||
Cedar is merging to mozilla-central about once a day, so it'll land there soon.
Updated•13 years ago
|
Whiteboard: [sg:dos][fixed-in-cedar] → [sg:dos]
Target Milestone: --- → mozilla2.2
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 33•13 years ago
|
||
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
Updated•13 years ago
|
Crash Signature: [@ strcmp | AuthCertificateCallback(void*, PRFileDesc*, int, int)]
You need to log in
before you can comment on or make changes to this bug.
Description
•