122261 - Assertion failed for certificate without e-mailAddr data

Status: VERIFIED FIXED

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

Description

[Erland Lewin]

I am experimenting with signing E-mails with using a certificate from my
Internet bank.

I sent a signed E-mail to myself. When I open it in Mozilla mail, I get an
assertion:

Assertion failure: emailAddr, at pcertdb.c:6860

Using gdb, I looked at the certificate, and it looks OK. However its' emailAddr
field is NULL. I think this is correct, that is, the certificate does not
contain my e-mail address.

Whatever Mozilla's behaviour should be, it shouldn't crash!

Comment 1

[Erland Lewin]

Attachment: gdb backtrace of assertion

Comment 2

[Stephane Saux]

kai

Comment 3

[Kai Engert [:KaiE:]]

Bob, Julien,

CERT_SaveSMimeProfile has PORT_Assert(emailAddr), which causes this crash.

What do you prefer, should we remove that assertion (and just return if there is
no address), or should the application assure the availability of an email
adress prior to calling NSS_SMIMESignerInfo_SaveSMIMEProfile?

Comment 4

[Kai Engert [:KaiE:]]

Should I fix that assertion failure inside NSS by adding checks to PSM?

Comment 5

[Kai Engert [:KaiE:]]

In the routine that crashed, I don't see an assertion anymore. It seems that
code has been removed. Not sure what the new behaviour is, i.e. whether it still
crashes: Erland, FYI: We landed a new version of the internal crypto libraries a
few days ago. Could you please help us, grab a Mozilla nightly build, and see
whether you still crash?

As the assertion has been removed, I'm marking this invalid. Erland, please
reopen the bug if you still crash. Thanks.

Comment 6

[Erland Lewin]

I now get the following assertion:

Assertion failure: emailAddr, at stanpcertdb.c:688

I believe this line has moved to #698 in stanpcertdb.c

I hope it's ok to keep this bug instead of creating a new one.

Comment 7

[Stephane Saux]

NSS?

Comment 8

[Wan-Teh Chang]

Kai,

I think the assertion PORT_Assert(emailAddr) in
CERT_SaveSMimeProfile() should be removed.  Bob,
do you agree?

Although PSM checks the return value of
NSS_SMIMESignerInfo_SaveSMIMEProfile(), it merely
logs a debug message and still returns NS_OK if
that function fails.  Is that the right thing?

Comment 9

[Wan-Teh Chang]

Attachment: Remove the unnecessary assertion in CERT_SaveSMimeProfile.

Comment 10

[Robert Relyea]

Yes we should remove the assert. we shouldn't crash on a malformed S/MIME
message, and we particularly should crash on a message that's well formed, but
missing the email address. We, of course, should not verify the signature (at
least without some other way to bind the email address to the certificate).

I think it's OK for PSM to quietly fail to save the S/MIME profile. A noisy
failure could be more annoying, especially since this operation happens as a
side effect of reading the email.

bob

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 71824 [details] [diff] [review]
Remove the unnecessary assertion in CERT_SaveSMimeProfile.

I agree to all previous comments.
r=kaie

Comment 12

[Wan-Teh Chang]

Comment on attachment 71824 [details] [diff] [review]
Remove the unnecessary assertion in CERT_SaveSMimeProfile.

I checked this patch into the tip of NSS.
I am going to get drivers's approval to get this
fix into the NSS_CLIENT_TAG.

Comment 13

[Wan-Teh Chang]

Comment on attachment 71824 [details] [diff] [review]
Remove the unnecessary assertion in CERT_SaveSMimeProfile.

This patch has been checked into the NSS_CLIENT_TAG of NSS.
I think you can mark this bug fixed now.

Comment 14

[Kai Engert [:KaiE:]]

Marking fixed.

Erland, tomorrow's build should work. 
If it does, can you please give feedback? Thanks!

Comment 15

[Erland Lewin]

Super, the crashes are gone. Thanks for your efforts.

A question though: How should I be informed that the message was signed?
  In the normal mail view (message under list of message in the mailbox), with a
collapsed 'headers' pane, I see no indication that the message is signed at all.
  If I expand the headers pane, I do se a "<signed>" at the far right opposite
the "Received:" header. I also see that the Contents-Type: says:

Content-Type: multipart/signed; protocol="application/x-pkcs7-signature";
micalg=sha1; boundary="------------ms020804000300050303040507"

(However, the signature is not presented as an attachment).

I can also select "View Message Security Info", and I properly see that the
message is signed.

So, what I'm wondering is if I shouldn't be seeing some kind of icon to indicate
that the message was signed (and the status of signature verification) in the
collapsed-headers view.

Comment 16

[Kai Engert [:KaiE:]]

Erland, thanks for pointing this out, we'll include some status on the status bar.