browser crashes when clicking "view certificate"



Core Graveyard
Security: UI
17 years ago
2 years ago


(Reporter: leg+mozilla, Assigned: Kai Engert)



Other Branch

Firefox Tracking Flags

(Not tracked)




(2 attachments)



17 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.3+) Gecko/20010817
BuildID:    2001081715 has a DSA certificate signed by a CMU-private DSA root
certificate.  My mozilla hasn't imported the root cert, so I correctly get a
unrecognized certificate warning.  When I click on "View certificate" to examine
the cert, the browser crashes.

Reproducible: Always
Steps to Reproduce:
1. launch browser
2. go to
3. click "view certificate"

Actual Results:  crashes

Expected Results:  standard certificate UI

Comment 3

17 years ago
no, i can view most certificates fine.

Can you try a talkback-enabled build ?
After you crashed and talkback submitted the crash, run
"install-dir/components/" and poste the Talkback ID# in this bug
Component: Security: General → Client Library
Product: Browser → PSM
Version: other → unspecified

Comment 5

17 years ago
i have three talkback incidents of this queued up.

i run components/talkback/talkback.
when i do "Incidents, Send Unsent Incident(s)", I get a "Network error" dialog
box with "Connect failed".

Comment 6

17 years ago
I also have a talback on this (Win2k this time) but the talkback server seems to
be down(they had that outage at MV this weekend which might be the cause) I'll
try to re-submit later on Monday because the talkback server should be back up
(I hope)

Reporter: please do the same and then post the Talkback Ids in this bug


Comment 7

17 years ago
OK, the talkback server is back up. Talkback ID: TB34271652Z on Win2k 20010817

Need to change OS to ALL
OS -> All.

Matti: Confirm?
OS: Linux → All
Assignee: mstoltz → ssaux

Comment 10

17 years ago
Assignee: ssaux → ddrinan
Priority: -- → P1
Target Milestone: --- → 2.1

Comment 11

17 years ago
Linux talkback ID TB34271536M on 2001081715
CC stephen for the talkback...

Comment 14

17 years ago
Confirming, I can reproduce this bug.

nsNSSCertificate was constructed with a CERTCertificate *cert, where
cert->signature->parameters is null. This crashes in ProcessSECAlgorithmID,
which assume this member is non-null.
The most important parts of the stack trace:
#0  0x4166dc05 in ProcessSECAlgorithmID (algID=0x884d9c0,
nssComponent=0x81f4f24, retSequence=0xbfff92ac) at
#1  0x4166fd0f in nsNSSCertificate::CreateTBSCertificateASN1Struct
(this=0x887f900, retSequence=0xbfff9594, nssComponent=0x81f4f24) at
#2  0x416717ac in nsNSSCertificate::CreateASN1Struct (this=0x887f900) at
#3  0x41671f17 in nsNSSCertificate::GetASN1Structure (this=0x887f900,
aASN1Structure=0xbfff975c) at
Ever confirmed: true

Comment 15

17 years ago
adding relyea to cc list for comments.
-> to kai.
Assignee: ddrinan → kai.engert

Comment 16

17 years ago
ProcessSECAlgorithmID should verify that the parameters is not NULL before

DSA keys are made up of a couple of components:
1) the key value.
2) prime (p)
3) small prime (q)
4) generator (g)

Many keys in the same administrative domain typically share the same pq and g
parameters. Users typically inherit them from their CA. You cannot verify
signatures without them.

Since the users already share these parameters with the CA, and these parameters
are quite large, DSA sites often do not include these parameters in subordinate
CA's and subordinate user certs when they are shared with the root most cert.
NSS agressively copies these parameters to various leaf certs when they first
become available, but if we do not have the root cert to copy from, we will not
be able to update the field.

So the correct behavior is check the paramter field. If it is NULL, then we will
not be able to verify any signatures for this certificate (in the case of DSA).
This also means we cannot extract a working Public Key.


Comment 17

17 years ago
Ok, with the null check it doesn't crash anymore. So the fix for the crash at
least is straightforward, I will attach a patch.

In addition I noticed: Just before the crash, GetOIDText is called, which has no
id-to-name-string mapping for the algorithm
SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST. I will open another (futured)
tracking bug to extend function GetOIDText with
the missing mappings.

Comment 18

17 years ago
Created attachment 47430 [details] [diff] [review]
Suggested fix

Comment 19

17 years ago
Adding keywords. Bob, can you please review the patch?
Keywords: crash, patch, review

Comment 20

17 years ago
OK, the patch looks good.

Comment 21

17 years ago
From the patch:

-  if (algID->[0] == nsIASN1Object::ASN1_NULL) {
+  if (!algID->parameters.len || algID->[0] ==
nsIASN1Object::ASN1_NULL) {

If you only want to look at data when len is non-zero, shouldn't this be

  if (algID->parameters.len && algID->[0]...)



Comment 22

17 years ago
No, we explicitly want it the way I wrote it. 

What we have here is similar to:

if ( !var || *var == NULLVAL ) {
  // a
else {
  // b

The code (a) is the default code. It must be executed in two cases, either if
var is null, or if the contents of var are a special app defined NULL value. The
order of the two expressions separated by || makes sure, the second expression
(*var == NULLVAL) will only be evaluated when the first one is false, i.e. var
is non-null.

Your suggested statement is not correct. If you wanted to code it using &&,
without changing the order of (a) and (b), you could rewrite the expression as:

if ( !(var && *var != NULLVAL) )

or, using the variables from the code:

if ( !(algID->parameters.len && algID->[0] !=
nsIASN1Object::ASN1_NULL) )

Comment 23

17 years ago
Ok, that'll teach to look at the patch in context, and not just by itself.
Sorry about that.


Comment 24

17 years ago
a=asa on behalf of drivers

Comment 25

17 years ago
Patch checked in
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 26

17 years ago
Verified fixed.


13 years ago
Component: Security: UI → Security: UI
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.