Closed Bug 207740 Opened 21 years ago Closed 21 years ago

Browser crashes on HTTPS urls - Trunk M140RC1 [@ cert_get_next_general_name]

Categories

(NSS :: Libraries, defect, P1)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: laurens, Assigned: nelson)

References

(
URL
)

Details

(4 keywords, Whiteboard: [3.7.7][3.4.4])

Crash Data

Attachments

(1 file, 1 obsolete file)

Simpler patch
573 bytes, patch
wtc
: review+
asa
: approval1.4+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030529 Mozilla 1.4 RC 1 crashes on accessing certain HTTPS sites (either entering them in the URL bar or clicking on a link.) At the moment I have only been able to reproduce this bug with my local web server. I haven't found public internet servers that trigger the same behaviour. But by accessing my local server I can reproduce the crash every time. Reproducible: Always Steps to Reproduce: 1. Enter an url in the location bar e.g. "https://www" and press enter Actual Results: Browser crashes. Expected Results: Browser opens this page using SSL. Talkback Incident IDs: TB20604478W, TB20604426Q, TB20604314K and TB20604306Z. My local web server is running Apache 1.3.27 and mod_ssl 2.8.12-1.3.27. The page requested is very simple: <HTML><BODY>-</BODY></HTML>.
Some more details: - The SSL certificate I am using is (of course) self signed and has as Common Name "10.1.1.2" - Mozilla 1.4 beta is working fine, but nightly build 2003-05-25-08-trunk does have this problem (I can't find nightly build before 05-25)
Some mirrors still have old builds, so I was able to narrow the problem down to the day it first accorded. Nightly build 2003-05-23-09-trunk is working fine. Nightly build 2003-05-24-08-trunk crashes.
Reporter: Have you installed Mozilla in a empty directory (uninstalled first, deleted the Mozilla application directory) and have you tried a new profile ?
Keywords: crash, stackwanted
Whiteboard: TB20604478W
Also happens on Mac OS X (10.2.3): crashes Mozilla 1.4rc1 (Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4) Gecko/20030529) and nightly build ID 2003053103 (Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.5a) Gecko/20030531) Does NOT happen on Mozilla 1.4b release (just re-downloaded and re-installed to confirm, but forgot to write down the ID before trashing it :P. Sorry. It's the May 7th release) Looks like some URLs cause the crash: https://webmail.upv.es ...but others don't: https://store.apple.com/Catalog/US/Images/salespolicies.html The first URL used to be problematic because of the type of the certificate (something to do with a not-recognized issuer of the certificate). May that be a cause? TalkBack IDs: release 1.4rc1: TB272328W, TB272333W nightly build 2003053103: TB273009G
As suggested in comment #3 from Matti, just tried with a new profile in the nightly build ID 2003053103. But the bad behaviour is consistent: the first URL given in comment #4 crashes Mozilla, but the 2nd doesn't. One difference: the 2nd URL warns me about entering a secure page (normal behaviour). The first URL doesn't; just crashes. I'm running this nightly build from a separate folder (though I guess that bit is not really useful for a Mac, since here the app is just [kind of] an atomic 46 Mb item...).
confirming crash with a 1 day old win2k build and the URL https://webmail.upv.es I asked for a new profile because you I use some https URLs frequently that doesn't crash and you mean that all https URLs cause a crash. cert_get_next_general_name(CERTGeneralNameStr * 0x00000000) line 215 + 4 bytes cert_DecodeGeneralNames(PLArenaPool * 0x1fcead2a, SECItemStr * * 0x07300a40) line 441 CERT_DecodeAltNameExtension(PLArenaPool * 0x07300a40, SECItemStr * 0x0175fbe0) line 180 + 7 bytes cert_GetCertificateEmailAddresses(CERTCertificateStr * 0x1fce3196) line 1109 + 11 bytes CERT_DecodeDERCertificate(SECItemStr * 0x1fcfa067, int 24509472, char * 0x00000001) line 875 nssDecodedPKIXCertificate_Create(NSSArenaStr * 0x00000000, NSSItemStr * 0x06e55bbc) line 426 + 33 bytes stan_GetCERTCertificate(NSSCertificateStr * 0x06e55b90, int 1226) line 709 + 11 bytes __CERT_NewTempCertificate(NSSTrustDomainStr * 0x1fd848c3, SECItemStr * 0x0701f660, char * 0x076344d4, int 0, int 0) line 272 + 6 bytes ssl3_HandleHandshakeMessage(sslSocketStr * 0x071c40b0, unsigned char * 0x071c4258, unsigned int 119292500) line 7993 + 8 bytes ssl3_HandleRecord(sslSocketStr * 0x1fd8ad86, SSL3Ciphertext * 0x071c40b0, sslBufferStr * 0x00000000) line 8378 + 11 bytes ssl_GatherRecord1stHandshake(sslSocketStr * 0x071c40b0) line 1261 ssl_SecureSend(sslSocketStr * 0x1fd90016, const unsigned char * 0x071c40b0, int 116027440, int 447) line 1028 + 6 bytes ssl_SecureWrite(sslSocketStr * 0x071c40b0, const unsigned char * 0x06ea7030, int 447) line 1062 + 22 bytes ssl_Write(PRFileDesc * 0x06e7c608, const void * 0x06ea7030, int 447) line 1283 nsSSLIOLayerWrite(PRFileDesc * 0x06e7c608, const void * 0x00000000, int 805319248) line 1187 + 14 bytes 07770a80() nsSSLIOLayerClose(PRFileDesc *) line 944 -> PSM
Assignee: general → ssaux
Status: UNCONFIRMED → NEW
Component: Browser-General → Client Library
Ever confirmed: true
Flags: blocking1.4?
Keywords: stackwanted → regression
Product: Browser → PSM
QA Contact: general → bmartin
Whiteboard: TB20604478W
Version: Trunk → unspecified
As suggested in comment 3 I tried installing Mozilla in a fresh directory, this did not change the behaviour. I assume comment 5 takes care of the new profile. Or do I need to try with both a fresh install and a new profile? The first url from comment 4 does indeed crash Mozilla, see Talkback ID TB20612627Y.
Component: Client Library → Browser-General
Keywords: regression → stackwanted
Product: PSM → Browser
QA Contact: bmartin → general
Whiteboard: TB20604478W
Version: unspecified → Trunk
Please don't clobber my changes. If you get a mid-air and i changed something don't use "proccedd anyway" ! YOu don't need to test now because i can reproduce it (but not with all Https URLS)
Component: Browser-General → Client Library
Keywords: stackwanted → regression
Product: Browser → PSM
QA Contact: general → bmartin
Whiteboard: TB20604478W
Version: Trunk → unspecified
Bob Relyea: This crash is in NSS. Can you comment?
I was told this happens on Mac too -> Changing hardware/OS to All. Self signed certificates seems to be the triggering factor here. I'm glad this was found before the 1.4 release (we don't want a 1.4.1 too soon).
OS: Windows 2000 → All
Hardware: PC → All
To Kai for investigation. Even if we have seen many self signed certificates being constructed incorrectly, we should not crash.
Assignee: ssaux → kaie
The certificate chain at https://www.ida.liu.se/ was investigated quite thoroughly in bug 174634, and was found to be valid then. It works as a testcase for this bug as well. Tested clean install/profile of 1.4RC1 on Linux, resulted in hanged networking in Mozilla. No crash though, but more or less just as bad.
Taking bug. Changing product to NSS. There are two separate issues here. 1. The immediate cause of the crash is a bug in cert_DecodeGeneralNames, which is an old bug, not a recent regression. The source file that contains this function had MANY such errors in it. Recently, many of those bugs were fixed, but apparently a few more remain. 2. It's unclear why cert_DecodeGeneralNames is being called with the particular input it is receiving. I will investigate.
Assignee: kaie → nelsonb
Component: Client Library → Libraries
Priority: -- → P1
Product: PSM → NSS
Target Milestone: --- → 3.8.1
Version: unspecified → 3.8
This is not a regression, per se'. It is not the case that some recenrly made change introduced a new bug. Rather, It is the case that one bug (that was recently fixed) masked another old one (which is now exposed). I recommend that this bug receive the same attention and priority as bug 204555 . Patch forthcoming.
Status: NEW → ASSIGNED
This patch may be overly cautious. The new nextName variable should never be NULL, so the test for it should be reduntdant. But better safe than sorry.
Comment on attachment 124948 [details] [diff] [review] test pointers for NULL before using them Wan-Teh, if you approve, please request approval1.4. Thanks.
Attachment #124948 - Flags: review?(wtc)
Since this is YET ANOTHER null pointer dereference bug in genname.c, I'm marking this bug as depending on bug 204555, which acts as a dependency for all the other bugs of this ilk.
Depends on: 204555
Comment on attachment 124948 [details] [diff] [review] test pointers for NULL before using them cert_get_prev_general_name() and cert_get_next_general_name() cannot return a NULL pointer. It's an artifact of their implementation; they could return an invalid pointer, but not NULL. So the only change we need is the "if (currentName)" test.
Attachment #124948 - Flags: review?(wtc) → review-
Comment on attachment 124948 [details] [diff] [review] test pointers for NULL before using them Nelson, I didn't read comment 16, so I didn't realize that you already knew those two functions cannot return NULL. I also found that in the cert_DecodeGeneralNames function those two functions will return a valid pointer if currentName is a valid pointer (because currentName is in a circular linked list). So I think the "if (nextName)" tests are not necessary there.
Hi! This is the bug I've had tons of problems with on RC1. My testcase is not only the one from liu.se (actually webmai.island.liu.se but it is the same certificate) but in my case its 2part. 1. I get a crash to windows with no chance of "recovery" when using the webmail https link. 2. When connecting to the mailserver IMAP.island.liu.se (that I think uses the same certificate) I get the Crashmessage but I'm still able to use both the browser and the email client if I ignore the message (No actuall connection to the mailserver is made though, so I cannot receive or send any mails) Absolutely a blocker/critical bug, at least for me=)
Regarding Comment 20: Note that the certs in question do not comply with RFC 3280, AFAIK. They contain empty Subject Alternative Name extensions. They contain extensions that say "here is the list of alternative names for this subject", and the list is empty. An empty list is not allowed by the ASN.1 definition of GeneralNames (see page 35 of RFC 3280). If the list is empty, the extension should not be present. I'm trying to get this patch ready to go in ASAP, so I coded it conservatively. Now, I'll have to recode it. :-(. I hope there's still time for this patch to make it into moz 1.4.
Comment on attachment 124948 [details] [diff] [review] test pointers for NULL before using them >+ if (currentName) { >+ nextName = cert_get_next_general_name(currentName); >+ if (nextName) >+ nextName->l.prev = tail; >+ return nextName; >+ } > loser: > return NULL; > } Should we set the error code right before the loser label? Alternatively, we can fix the caller (CERT_DecodeAltNameExtension) to not call cert_DecodeGeneralNames under this condition.
Attached patch Simpler patch — — Splinter Review
This patch is much smaller than the last and makes the code more obviously correct, IMO.
Attachment #124948 - Attachment is obsolete: true
Attachment #124964 - Flags: review?(wtc)
In answer to comment 22, I would say that this condition is not an error. It is simply an empty list.
Comment on attachment 124964 [details] [diff] [review] Simpler patch r=wtc. Nelson, I believe this patch can be further simplified. >+ if (currentName) { >+ return cert_get_next_general_name(currentName); >+ } This can be replaced by: return head;
Attachment #124964 - Flags: review?(wtc) → review+
Regarding comment 25. "head" has the wrong type. It is the doubly-linked list structure inside the CERTGeneralName. This function needs to return the address of the CERTGeneralName that contains head.
Comment on attachment 124964 [details] [diff] [review] Simpler patch drivers: remember that NSS doesn't require sr review.
Attachment #124964 - Flags: approval1.4?
a=adt Please land this fix on the 1.4 Branch after getting drivers approval and add the fixed1.4 keyword.
Comment on attachment 124964 [details] [diff] [review] Simpler patch a=asa (on behalf of drivers) for checkin to the 1.4 branch.
Attachment #124964 - Flags: approval1.4? → approval1.4+
The patch has been checked into the NSS tip (NSS 3.9), NSS_3_8_BRANCH (NSS 3.8.1), NSS_CLIENT_TAG (Mozilla 1.5 alpha), and MOZILLA_1_4_BRANCH (Mozilla 1.4 final).
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Keywords: fixed1.4
Resolution: --- → FIXED
Flags: blocking1.4?
Patch checked into NSS_3_7_BRANCH for NSS 3.7.7 Beta 1.
Whiteboard: [3.7.7]
Adding topcrash and Trunk M140RC1 [@ cert_get_next_general_name] to summary for future reference. No crashes since 6/4 builds...marking verified based on Talkback data.
Status: RESOLVED → VERIFIED
Keywords: topcrash
Summary: Browser crashes on HTTPS urls → Browser crashes on HTTPS urls - Trunk M140RC1 [@ cert_get_next_general_name]
Changing dependency from 204555 to 174885. This bug and 204555 are independent of each other. Both were both masked by bug 174885, and were exposed once 174885 was fixed.
Depends on: 174885
No longer depends on: 204555
Patch checked in on the NSS_3_4_BRANCH (3.4.4).
Whiteboard: [3.7.7] → [3.7.7][3.4.4]
Crash Signature: [@ cert_get_next_general_name]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: