Last Comment Bug 207740 - Browser crashes on HTTPS urls - Trunk M140RC1 [@ cert_get_next_general_name]
: Browser crashes on HTTPS urls - Trunk M140RC1 [@ cert_get_next_general_name]
Status: VERIFIED FIXED
[3.7.7][3.4.4]
: crash, fixed1.4, regression, topcrash
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.8
: All All
: P1 critical with 1 vote (vote)
: 3.8.1
Assigned To: Nelson Bolyard (seldom reads bugmail)
: bmartin
Mentors:
https://webmail.upv.es
Depends on: 174885
Blocks:
  Show dependency treegraph
 
Reported: 2003-05-31 02:21 PDT by Laurens Blankers
Modified: 2003-06-20 17:04 PDT (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
test pointers for NULL before using them (1010 bytes, patch)
2003-06-04 16:16 PDT, Nelson Bolyard (seldom reads bugmail)
wtc: review-
Details | Diff | Review
Simpler patch (573 bytes, patch)
2003-06-04 19:59 PDT, Nelson Bolyard (seldom reads bugmail)
wtc: review+
asa: approval1.4+
Details | Diff | Review

Description Laurens Blankers 2003-05-31 02:21:19 PDT
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>.
Comment 1 Laurens Blankers 2003-05-31 04:34:47 PDT
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)
Comment 2 Laurens Blankers 2003-05-31 05:26:57 PDT
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 Matthias Versen [:Matti] 2003-05-31 09:32:19 PDT
Reporter: 
Have you installed Mozilla in a empty directory (uninstalled first, deleted the
Mozilla application directory) and have you tried a new profile ?
Comment 4 Mijail 2003-05-31 09:45:36 PDT
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
Comment 5 Mijail 2003-05-31 09:59:20 PDT
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 Matthias Versen [:Matti] 2003-05-31 10:09:26 PDT
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
Comment 7 Laurens Blankers 2003-05-31 10:11:31 PDT
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.
Comment 8 Matthias Versen [:Matti] 2003-05-31 10:17:41 PDT
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)
Comment 9 Stephane Saux 2003-06-02 13:23:14 PDT
Bob Relyea: This crash is in NSS. Can you comment?
Comment 10 Daniel Bratell 2003-06-03 02:12:08 PDT
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).
Comment 11 Stephane Saux 2003-06-04 11:32:38 PDT
To Kai for investigation. Even if we have seen many self signed certificates
being constructed incorrectly, we should not crash.
Comment 12 Andreas Lange 2003-06-04 14:23:53 PDT
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.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2003-06-04 15:32:41 PDT
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.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2003-06-04 15:50:37 PDT
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.  
Comment 15 Nelson Bolyard (seldom reads bugmail) 2003-06-04 16:16:36 PDT
Created attachment 124948 [details] [diff] [review]
test pointers for NULL before using them

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 16 Nelson Bolyard (seldom reads bugmail) 2003-06-04 16:17:47 PDT
Comment on attachment 124948 [details] [diff] [review]
test pointers for NULL before using them

Wan-Teh, if you approve, please request approval1.4. Thanks.
Comment 17 Nelson Bolyard (seldom reads bugmail) 2003-06-04 16:19:25 PDT
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.
Comment 18 Wan-Teh Chang 2003-06-04 17:06:55 PDT
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.
Comment 19 Wan-Teh Chang 2003-06-04 17:34:43 PDT
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 Carl Adebring 2003-06-04 18:45:53 PDT
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=)
Comment 21 Nelson Bolyard (seldom reads bugmail) 2003-06-04 19:30:41 PDT
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 Wan-Teh Chang 2003-06-04 19:31:19 PDT
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.
Comment 23 Nelson Bolyard (seldom reads bugmail) 2003-06-04 19:59:25 PDT
Created attachment 124964 [details] [diff] [review]
Simpler patch

This patch is much smaller than the last and makes the code more obviously
correct, IMO.
Comment 24 Nelson Bolyard (seldom reads bugmail) 2003-06-04 20:04:23 PDT
In answer to comment 22, I would say that this condition is not an error. 
It is simply an empty list.  
Comment 25 Wan-Teh Chang 2003-06-04 20:32:09 PDT
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;
Comment 26 Nelson Bolyard (seldom reads bugmail) 2003-06-04 20:37:27 PDT
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 27 Nelson Bolyard (seldom reads bugmail) 2003-06-04 20:39:09 PDT
Comment on attachment 124964 [details] [diff] [review]
Simpler patch

drivers: remember that NSS doesn't require sr review.
Comment 28 Paul Wyskoczka 2003-06-05 08:43:43 PDT
a=adt Please land this fix on the 1.4 Branch after getting drivers approval and
add the fixed1.4 keyword.
Comment 29 Asa Dotzler [:asa] 2003-06-05 10:12:00 PDT
Comment on attachment 124964 [details] [diff] [review]
Simpler patch

a=asa (on behalf of drivers) for checkin to the 1.4 branch.
Comment 30 Wan-Teh Chang 2003-06-05 10:50:50 PDT
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).
Comment 31 Wan-Teh Chang 2003-06-05 15:51:14 PDT
Patch checked into NSS_3_7_BRANCH for NSS 3.7.7 Beta 1.
Comment 32 Jay Patel [:jay] 2003-06-10 15:00:04 PDT
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.
Comment 33 Nelson Bolyard (seldom reads bugmail) 2003-06-20 16:38:55 PDT
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 Wan-Teh Chang 2003-06-20 17:04:07 PDT
Patch checked in on the NSS_3_4_BRANCH (3.4.4).

Note You need to log in before you can comment on or make changes to this bug.