231709 - iframes pointing to signed XUL crashes browser

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Nick Mitchell]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

When I go to the given URL, the page loads correctly, but if I then reload or
close Mozilla it crashes with the message "mozilla.exe has caused errors and
will be terminated..."

In the testcase (an XUL page)there are two frames, one points to a normal XUL
page, the other points to a signed XUL page. The signing is done using signtool
and using a certificate we created ourselves. This problem seems to have come up
with version 1.6


Reproducible: Always

Steps to Reproduce:
1. load up testcase (page loads fine)
2. click reload
Actual Results:  
Mozilla crashes - error message is displayed

Expected Results:  
displayed the page again

Comment 1

[Peter 'vt100' Schwindt]

crashes here also - after having reloaded the URL a few times...

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040120

But there seems to be no talkback agent - wonder why

Comment 2

[Boris Zbarsky [:bzbarsky]]

There's no talkback because talkback is not set up yet.

caillon, this is yours -- the code in nsNSSComponent::VerifySignature needs to
addref aPrincipal after assigning it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Regression from bug 83536

Comment 4

[Ali Ebrahim]

This crashes Firebird as well.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040121 Firebird/0.8.0+

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Fix the crash; and another one.

There was another (not my fault) potential crash which I plugged, as well as
did some minor cleanup.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Better

Sigh.  So they cast an enum containing 3 things to a PRBool (which is typedefed
to a PRIntn) so we really can't in good faith just check if (!bool).  This
should do the trick.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

+  // SEC_PKCS7VerifyDetachedSignature returns a SECStatus cast to PRBool,
+  // so we have to explicitly check |if (verified != PR_TRUE)| here, rather
+  // than check |if (!verified)|
+  if (verified != PR_TRUE) {

so um...
according to
http://lxr.mozilla.org/seamonkey/source/security/nss/lib/util/seccomon.h#99 a
SECStatus is never PR_TRUE (which is defined as 1). so this comparison seems to
be always true?

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 139711 [details] [diff] [review]
Better

+  PRBool verified = SEC_PKCS7VerifyDetachedSignature(p7_info,
+						      certUsageObjectSigner,
+						      &digest, HASH_AlgSHA1,
+						      PR_TRUE);
+  // SEC_PKCS7VerifyDetachedSignature returns a SECStatus cast to PRBool,
+  // so we have to explicitly check |if (verified != PR_TRUE)| here, rather
+  // than check |if (!verified)|

Oh, how nice! Is there a bug on file on that?

r+sr=jst

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: What was checked in

I decided to undo the changes to the SECStatus/PRBool thing.  I don't want that
cvs blame.  :-)

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in at 01/26/2004 21:01 PST.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 229845 has been marked as a duplicate of this bug. ***

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 234622 has been marked as a duplicate of this bug. ***