browser crashes when clicking "view certificate"

VERIFIED FIXED in psm2.1

Status

Core Graveyard
Security: UI
P1
normal
VERIFIED FIXED
17 years ago
2 years ago

People

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

Tracking

({crash})

Other Branch
psm2.1
x86
All
crash

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

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

netreg.net.cmu.edu 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 https://netreg.net.cmu.edu/
3. click "view certificate"

Actual Results:  crashes

Expected Results:  standard certificate UI
(Reporter)

Comment 3

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

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

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

Thanks

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
->PSM
Assignee: mstoltz → ssaux

Comment 10

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

Comment 11

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

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
../../../../../mozilla/security/manager/ssl/src/nsNSSCertificate.cpp:1892
#1  0x4166fd0f in nsNSSCertificate::CreateTBSCertificateASN1Struct
(this=0x887f900, retSequence=0xbfff9594, nssComponent=0x81f4f24) at
../../../../../mozilla/security/manager/ssl/src/nsNSSCertificate.cpp:2162
#2  0x416717ac in nsNSSCertificate::CreateASN1Struct (this=0x887f900) at
../../../../../mozilla/security/manager/ssl/src/nsNSSCertificate.cpp:2301
#3  0x41671f17 in nsNSSCertificate::GetASN1Structure (this=0x887f900,
aASN1Structure=0xbfff975c) at
../../../../../mozilla/security/manager/ssl/src/nsNSSCertificate.cpp:2344
Status: UNCONFIRMED → NEW
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
proceeding.

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.

bob
(Assignee)

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.
Status: NEW → ASSIGNED
(Assignee)

Comment 18

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

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.
r=relyea

Comment 21

17 years ago
From the patch:

-  if (algID->parameters.data[0] == nsIASN1Object::ASN1_NULL) {
+  if (!algID->parameters.len || algID->parameters.data[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->parameters.data[0]...)

?

(Assignee)

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->parameters.data[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.

sr=tor

Comment 24

17 years ago
a=asa on behalf of drivers
(Assignee)

Comment 25

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

Comment 26

17 years ago
Verified fixed.
Status: RESOLVED → VERIFIED

Updated

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.