132543 - CRL-Download let mozilla die

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Wolfgang Rosenauer [:wolfiR]]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.9+) Gecko/20020320
BuildID:    

mozilla dies completely with this CRL

built from Source-Snapshot: 20020320
but it was the same with all my moz versions before



Reproducible: Always
Steps to Reproduce:
1.try to get the CRL downloaded


Actual Results:  mozilla dies

Expected Results:  the CRL should be downloaded and displayed under Validation

Comment 1

[Sebastian Biallas]

BuildID 2002031608, Win 98, TB4299414Q

Comment 2

[Russell Odom]

Confirmed on W2K 2002031104. Will include talkback ID as soon as it sends
successfully - talkback server doesn't want to talk to me at the moment :-(

Comment 3

[Russell Odom]

OK, here we go: talkback ID TB4303328G

Comment 4

[Marc Boullet]

cc'ing Stephen for talkback retrieval.

Comment 5

[Boris Zbarsky [:bzbarsky]]

#0  0x42648272 in nsNSSCertificate::nsNSSCertificate (this=0x86d7660, 
    certDER=0x86d6e30 "0\202\001�0\201�0\r\006\t*\206H\206�\r\001\001\004\005", 
    derLen=470) at nsNSSCertificate.cpp:643
#1  0x426556ed in nsNSSCertificateDB::ImportCertificates (this=0x86a04e8, 
    data=0x86eb510 "0\202\001�0\201�0\r\006\t*\206H\206�\r\001\001\004\005",
length=470, 
    type=1, ctx=0x8700458) at nsNSSCertificate.cpp:3013
#2  0x4263aeb2 in PSMContentDownloader::OnStopRequest (this=0x86eb3a0, 
    request=0x86a8fb0, context=0x0, aStatus=0) at nsNSSComponent.cpp:1714
#3  0x40751bf8 in nsDocumentOpenInfo::OnStopRequest (this=0x86a94d8,
request=0x86a8fb0, 
    aCtxt=0x0, aStatus=0) at nsURILoader.cpp:253
#4  0x408f4c20 in nsStreamListenerTee::OnStopRequest (this=0x86fd348,
request=0x86a8fb0, 
    context=0x0, status=0) at nsStreamListenerTee.cpp:24
#5  0x4093b9ba in nsHttpChannel::OnStopRequest (this=0x86a8fb0, request=0x86bb9ec, 
    ctxt=0x0, status=0) at nsHttpChannel.cpp:2581
#6  0x4096908b in nsOnStopRequestEvent::HandleEvent (this=0x85fc0e8)
    at nsRequestObserverProxy.cpp:212
#7  0x408dc949 in nsARequestObserverEvent::HandlePLEvent (plev=0x85fc0e8)
    at nsRequestObserverProxy.cpp:115
#8  0x4024334b in PL_HandleEvent (self=0x85fc0e8) at plevent.c:590
#9  0x4024315c in PL_ProcessPendingEvents (self=0x80fdc70) at plevent.c:520
#10 0x40245370 in nsEventQueueImpl::ProcessPendingEvents (this=0x80fdb90)
    at nsEventQueue.cpp:388
#11 0x41605cb4 in event_processor_callback (data=0x80fdb90, source=8, 
    condition=GDK_INPUT_READ) at nsAppShell.cpp:184
#12 0x4160588f in our_gdk_io_invoke (source=0x8310c50, condition=G_IO_IN,
data=0x824dc08)
    at nsAppShell.cpp:77
#13 0x404d4aca in g_io_unix_dispatch () from /usr/lib/libglib-1.2.so.0
#14 0x404d6186 in g_main_dispatch () from /usr/lib/libglib-1.2.so.0
#15 0x404d6751 in g_main_iterate () from /usr/lib/libglib-1.2.so.0
#16 0x404d68f1 in g_main_run () from /usr/lib/libglib-1.2.so.0
#17 0x403fac69 in gtk_main () from /usr/lib/libgtk-1.2.so.0
#18 0x4160635a in nsAppShell::Run (this=0x8174b78) at nsAppShell.cpp:364
#19 0x4159dd24 in nsAppShellService::Run (this=0x81740f8) at
nsAppShellService.cpp:308
#20 0x0805bfb5 in main1 (argc=2, argv=0xbffff5f4, nativeApp=0x0) at
nsAppRunner.cpp:1350
#21 0x0805cccc in main (argc=2, argv=0xbffff5f4) at nsAppRunner.cpp:1698
#22 0x406009cb in __libc_start_main (main=0x805cacc <main>, argc=2,
argv=0xbffff5f4, 
    init=0x8055ae0 <_init>, fini=0x8068a9c <_fini>, rtld_fini=0x4000ae60
<_dl_fini>, 
    stack_end=0xbffff5ec) at ../sysdeps/generic/libc-start.c:92


(gdb) frame 0
#0  0x42648272 in nsNSSCertificate::nsNSSCertificate (this=0x86d7660, 
    certDER=0x86d6e30 "0\202\001&#65533;0\201&#65533;0\r\006\t*\206H\206&#65533;\r\001\001\004\005", 
    derLen=470) at nsNSSCertificate.cpp:643
643       if(mCert->dbhandle == nsnull)
(gdb) p mCert
$2 = (CERTCertificate *) 0x0

Comment 6

[Stephane Saux]

Confirming.

The crash occurs in the constructor to nsNSSCertificate because the DER passed
as an argument fails to parse, and CERT_DecodeCertFromPackage() returns null,
but the constructor doesn't check for null.

Even if it did (avoiding the crash), it's unclear how the code using this
constructor would know about this.  I've found two uses of this contructor. The
one in this crash (nsNSSCertificateDB::ImportCertificates) and one usage in
nsCMSSecureMessage.cpp.  In neither case is the mCert variable checked.

Maybe we should get rid of this constructor and explicitely call
CERT_DecodeCertFromPackage before calling the other nsNSSCertificate constructor
which takes a CERTCertificate.

This would allow us to check on the return value, and not allocate a new object.
 This makes for more defensive code. We don't know where we've gotten the DER,
in fact in this case, we get it from the wire, and we should not trust it.  In
the other case, we get it from an email (i.e., from the wire).

We can do this regardeless of whether CERT_DecodeCertFromPackage correctly
failed to parse the DER in this crash.  This must be investigated as well.

Assigning to kai.

Welcoming comments.

Comment 7

[Kai Engert [:KaiE:]]

Your suggestion makes sense, I even think both constructors should be enhanced,
the other one could lead to a random mCert, if the passed in cert is NULL.

I have a suggestion, which would avoid duplicating the calls to NSS to outside
of class nsNSSCertificate.

First, remove both constructors, and only use a default constructor without
parameters, that initializes all members to nice defaults, and that should
include mCert.

Next, add two static methods to the class, so called "factory methods". Each
will take the argument list of the current constructors. They will have the code
from the constructors. If that succeeds, they will create a new nsNSSCertificate
object, and set the mCert member, and return the new object. If there's
something wrong, and it does not make sense to create a nsNSSCertificate object,
those factory methods just return NULL.

Something like:

class nsNSSCertificate
{
public:
  nsNSSCertificate(); // inits members, set mCert to nsnull

  static nsNSSCertificate* Construct(char *certDER, int derLEN);
  static nsNSSCertificate* Construct(CERTCertificate *cert);
};

create objects with

nsNSSCertificate *c = nsNSSCertificate::Construct(der, len);
if (!c)
...

Comment 8

[Kai Engert [:KaiE:]]

I take part of my suggestion back, too many places call the constructor that
takes a CERTCertificate as an argument.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Suggested fix

This patch
- makes one constructor failsafe.
- replaces the other constructor with a static failsafe factory method
- replaces usages of the replaced constructor
- fixes the crash for me

Comment 10

[Kai Engert [:KaiE:]]

Attachment: Updated fix

Found a minor error while proofreading my patch, updated version.

Comment 11

[Kai Engert [:KaiE:]]

Javi, can you please review?

Comment 12

[Javier Delgadillo]

Comment on attachment 78290 [details] [diff] [review]
Updated fix

>-  nsCOMPtr<nsIX509Cert> cert =  new nsNSSCertificate((char *)data, length);
>+  nsCOMPtr<nsIX509Cert> cert =  nsNSSCertificate::ConstructFromDER((char *)data, length);
> 
>-  *_retval = cert;
>-  NS_IF_ADDREF(*_retval);
>+  if (cert) {
>+    *_retval = cert;
>+    NS_IF_ADDREF(*_retval);
Since we already know *_retval is non-NULL (since cert is non-NULL) could
probably save a few cycles by just making this NS_ADDREF(*_retval)

Everything else looks fine.  r=javi

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Fixed fix

Small change as suggested.

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 78376 [details] [diff] [review]
Fixed fix

has r=javi

Comment 15

[Kai Engert [:KaiE:]]

Alec, can you please review this crash fix?

Comment 16

[Alec Flett]

Comment on attachment 78376 [details] [diff] [review]
Fixed fix

sr=alecf

Comment 17

[Kai Engert [:KaiE:]]

adt1, because it is a crash. Will send mail to drivers.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 78376 [details] [diff] [review]
Fixed fix

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 19

[Jaime Rodriguez, Jr.]

Will this change affect SSL in Mail, or any other components.

Removing adt1.0.0. Pls land this on the trunk, and let it bake for a couple of
days. If there are no regressions, and QA approves, pls renominate for ADT approval.

Comment 20

[Jaime Rodriguez, Jr.]

My apologies Kaie. We meant to remove the adt1.0.0, not plus it. thanks for
bringing it to our attention. We hope to catch Stephane tomorrow to discuss a
few PSM bugs nominated to go into the 1.0 branch, but pls land this on the trunk
and get some testing for a couple of days.

Comment 21

[Kai Engert [:KaiE:]]

Checked in to the trunk. Bug stays open until we land it on the branch.

Comment 22

[John Unruh]

Visiting the above URL with the 4/12 Win2000 trunk build does not crash the 
browser, but it does not import the CRL either.

Comment 23

[Kai Engert [:KaiE:]]

> Visiting the above URL with the 4/12 Win2000 trunk build does not crash the 
> browser, but it does not import the CRL either.

As Stephane noted when he debugged this, the CRL which is downloadable at that
CRL seems to be in a format, which NSS does not understand.

Therefore, importing does not work.

This bug is about the crash, and you confirmed that this CRL is no longer
causing Mozilla to crash. Thanks for testing, John!

Therefore, I'm renomating this bug for adt.

I suggest we file another bug, in order to find out why NSS is unable to parse
and import this CRL.

Comment 24

[Kai Engert [:KaiE:]]

Sidenote.

> As Stephane noted when he debugged this, the CRL which is downloadable at that
> CRL seems to be in a format, which NSS does not understand.
> I suggest we file another bug, in order to find out why NSS is unable to parse
> and import this CRL.

I looked at the CRL and saw that it is "empty", i.e. it does list any revoked
certificates.
John told me that other software he tried doesn't import that CRL either.

Comment 25

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for checkin into 1.0. Pls check it into 1.0 branch
today.

John - Pls file another bug for the CRL import issue.

Comment 26

[John Unruh]

Filed bug 137149

Comment 27

[Kai Engert [:KaiE:]]

Checked in to the branch, fixed

Comment 28

[John Unruh]

Verified on the 4/15 Win2000 branch build.