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: