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)
Tracking
(Not tracked)
VERIFIED
FIXED
3.8.1
People
(Reporter: laurens, Assigned: nelson)
References
()
Details
(4 keywords, Whiteboard: [3.7.7][3.4.4])
Crash Data
Attachments
(1 file, 1 obsolete file)
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>.
Reporter | ||
Comment 1•21 years ago
|
||
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)
Reporter | ||
Comment 2•21 years ago
|
||
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.
Comment 3•21 years ago
|
||
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...).
Comment 6•21 years ago
|
||
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
Reporter | ||
Comment 7•21 years ago
|
||
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
Comment 8•21 years ago
|
||
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
Comment 9•21 years ago
|
||
Bob Relyea: This crash is in NSS. Can you comment?
Updated•21 years ago
|
Comment 10•21 years ago
|
||
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
Comment 11•21 years ago
|
||
To Kai for investigation. Even if we have seen many self signed certificates being constructed incorrectly, we should not crash.
Assignee: ssaux → kaie
Comment 12•21 years ago
|
||
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.
Assignee | ||
Comment 13•21 years ago
|
||
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
Assignee | ||
Comment 14•21 years ago
|
||
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
Assignee | ||
Comment 15•21 years ago
|
||
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.
Assignee | ||
Comment 16•21 years ago
|
||
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)
Assignee | ||
Comment 17•21 years ago
|
||
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 18•21 years ago
|
||
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 19•21 years ago
|
||
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.
Comment 20•21 years ago
|
||
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=)
Assignee | ||
Comment 21•21 years ago
|
||
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 22•21 years ago
|
||
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.
Assignee | ||
Comment 23•21 years ago
|
||
This patch is much smaller than the last and makes the code more obviously correct, IMO.
Attachment #124948 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #124964 -
Flags: review?(wtc)
Assignee | ||
Comment 24•21 years ago
|
||
In answer to comment 22, I would say that this condition is not an error. It is simply an empty list.
Comment 25•21 years ago
|
||
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+
Assignee | ||
Comment 26•21 years ago
|
||
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.
Assignee | ||
Comment 27•21 years ago
|
||
Comment on attachment 124964 [details] [diff] [review] Simpler patch drivers: remember that NSS doesn't require sr review.
Attachment #124964 -
Flags: approval1.4?
Comment 28•21 years ago
|
||
a=adt Please land this fix on the 1.4 Branch after getting drivers approval and add the fixed1.4 keyword.
Comment 29•21 years ago
|
||
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+
Comment 30•21 years ago
|
||
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).
Updated•21 years ago
|
Flags: blocking1.4?
Comment 31•21 years ago
|
||
Patch checked into NSS_3_7_BRANCH for NSS 3.7.7 Beta 1.
Whiteboard: [3.7.7]
Comment 32•21 years ago
|
||
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]
Assignee | ||
Comment 33•21 years ago
|
||
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.
Comment 34•21 years ago
|
||
Patch checked in on the NSS_3_4_BRANCH (3.4.4).
Whiteboard: [3.7.7] → [3.7.7][3.4.4]
Updated•13 years ago
|
Crash Signature: [@ cert_get_next_general_name]
You need to log in
before you can comment on or make changes to this bug.
Description
•