Closed Bug 231709 Opened 21 years ago Closed 21 years ago

iframes pointing to signed XUL crashes browser

Categories

(Core :: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.7alpha

People

(Reporter: nramitchell, Assigned: caillon)

References

()

Details

(Keywords: crash, regression)

Attachments

(2 files, 1 obsolete file)

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
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
Keywords: crash
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.
Assignee: general → security-bugs
Status: UNCONFIRMED → NEW
Component: Browser-General → Security: General
Ever confirmed: true
Keywords: regression
OS: Windows 2000 → All
Hardware: PC → All
Regression from bug 83536
Assignee: security-bugs → caillon
This crashes Firebird as well.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040121 Firebird/0.8.0+
Attached patch Fix the crash; and another one. (obsolete) — Splinter Review
There was another (not my fault) potential crash which I plugged, as well as
did some minor cleanup.
Attached patch BetterSplinter Review
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.
Attachment #139710 - Attachment is obsolete: true
Attachment #139711 - Flags: superreview?(jst)
Attachment #139711 - Flags: review?(jst)
+  // 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 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
Attachment #139711 - Flags: superreview?(jst)
Attachment #139711 - Flags: superreview+
Attachment #139711 - Flags: review?(jst)
Attachment #139711 - Flags: review+
I decided to undo the changes to the SECStatus/PRBool thing.  I don't want that
cvs blame.  :-)
Checked in at 01/26/2004 21:01 PST.
Status: NEW → RESOLVED
Closed: 21 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.7alpha
*** Bug 229845 has been marked as a duplicate of this bug. ***
*** Bug 234622 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: