1293378 - deal with nsIX509Cert constants VERIFIED_OK, NOT_VERIFIED_UNKNOWN (after bug 1284946)

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1287825 +++

Bug 1287825 comment 2:

Sorry - I forgot to check for instances of what bug 1284946 was going to remove in comm-central. Along those lines, it looks like the nsIX509Cert constants VERIFIED_OK, NOT_VERIFIED_UNKNOWN, etc. will have to be dealt with here: https://dxr.mozilla.org/comm-central/rev/752a2122c521c5115049f4fbb4229484a05b2c2c/mailnews/extensions/smime/content/msgCompSecurityInfo.js#24
As far as I can tell, that function is only being used from line 204 of that file. The values passed to it come from the call to nsISMimeJSHelper.getRecipientCertsInfo on line 89. Looking at the implementation, those values appear to all just be 0: https://dxr.mozilla.org/comm-central/source/mailnews/extensions/smime/src/nsSMimeJSHelper.cpp#97 so this implementation can be simplified somewhat.

Comment 1

[Wayne Mery (:wsmwk)]

(Presumably this isn't actually a blocker - else someone would have already killed this)

Comment 2

[Magnus Melin [:mkmelin]]

There appears to be a bunch of dead code here. 
The statuses in https://searchfox.org/comm-central/rev/206167865d0412b06b864631cee8a9cb7f86ff63/mailnews/extensions/smime/content/msgCompSecurityInfo.js#24 do not exist (anymore?)

The code is only about the Security dialog in the compose window, where all the cert info one will see at the moment is issuing date and expiry date of certs. But in this context I don't think those are even that useful as it doesn't really matter that the cert expired. I guess in this case you could send unencrypted to be sure, but in general the recipient would be able to read it fine unless he lost the old certificate. What would be interesting in that dialog is the cert status, but that column is always blank now since the verification is all dead code. Hmm :/

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1293378_cert_status.patch

Made the status column work again.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8999582 [details] [diff] [review]
bug1293378_cert_status.patch

Review of attachment 8999582 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
@@ +233,5 @@
>      gListBox.appendChild(listitem);
>    }
>  }
>  
> +function findOutStatus(cert) {

ah, forgot to remove this

Comment 5

[:aceman]

Comment on attachment 8999582 [details] [diff] [review]
bug1293378_cert_status.patch

Review of attachment 8999582 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
@@ +166,2 @@
>      listitem.appendChild(createCell(gEmailAddresses.value[i]));
> +    let id = "recipient-nbr-" + i

Missing semicolon? But is the 'id' variable even needed?

@@ +191,5 @@
> +        const SEC_ERROR_UNKNOWN_ISSUER                    = SEC_ERROR_BASE + 13;
> +        const SEC_ERROR_UNTRUSTED_ISSUER                  = SEC_ERROR_BASE + 20;
> +        const SEC_ERROR_UNTRUSTED_CERT                    = SEC_ERROR_BASE + 21;
> +        const SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE        = SEC_ERROR_BASE + 30;
> +        const SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED = SEC_ERROR_BASE + 176;

What are these constants (like 176)? Can't we fetch them by names from anywhere?

@@ +216,5 @@
> +           let errorPresent = results.some(result =>
> +             result.errorCode == errorRanking.error
> +           );
> +           if (errorPresent) {
> +             bs = errorRanking.bundleString;

I'm not sure I understnad this. We have multiple results? Why? And then we compare the multiple results to error codes and only use the first hit for output? Why?

@@ +222,5 @@
> +           }
> +         }
> +
> +         document.getElementById(id).firstChild.nextSibling // second column
> +           .setAttribute("label", gBundle.getString(bs));

Don't we align under the dot in these cases?

@@ +261,5 @@
> +      certdb.asyncVerifyCertAtTime(cert, usage, 0, null, now,
> +        (aPRErrorCode, aVerifiedChain, aHasEVPolicy) => {
> +          resolve({ usageString,
> +                    errorCode: aPRErrorCode,
> +                    chain: aVerifiedChain });

Does m-c have competitions in writing the most obfuscated code? ;)

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #5)
> Comment on attachment 8999582 [details] [diff] [review]
> bug1293378_cert_status.patch
> 
> Review of attachment 8999582 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
> @@ +166,2 @@
> >      listitem.appendChild(createCell(gEmailAddresses.value[i]));
> > +    let id = "recipient-nbr-" + i
> 
> Missing semicolon? But is the 'id' variable even needed?

Ah you're right. This was leftover from an earlier iteration.

> 
> @@ +191,5 @@
> > +        const SEC_ERROR_UNKNOWN_ISSUER                    = SEC_ERROR_BASE + 13;
> > +        const SEC_ERROR_UNTRUSTED_ISSUER                  = SEC_ERROR_BASE + 20;
> > +        const SEC_ERROR_UNTRUSTED_CERT                    = SEC_ERROR_BASE + 21;
> > +        const SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE        = SEC_ERROR_BASE + 30;
> > +        const SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED = SEC_ERROR_BASE + 176;
> 
> What are these constants (like 176)? Can't we fetch them by names from
> anywhere?

Unfortunately we can't. I'm not sure why security things like these don't define accessible named constants. I think it has something to do with that part is nss code and part is integration code.

> 
> @@ +216,5 @@
> > +           let errorPresent = results.some(result =>
> > +             result.errorCode == errorRanking.error
> > +           );
> > +           if (errorPresent) {
> > +             bs = errorRanking.bundleString;
> 
> I'm not sure I understadd this. We have multiple results? Why? And then we
> compare the multiple results to error codes and only use the first hit for
> output? Why?

Certificates are issued for different usages. In theory (at least) you could have a certificate that is allowed to sign but not encrypt. Therefore there are multiple results from the checks.

We only show the first one, mainly because it gets crowded to display two. Doesn't really matter so much which use case that has the a problem either - as a problem is a problem, and this is status "at a glance". For all practical use case I do think both sign and encrypt are supported for s/mime certificates.

> 
> @@ +222,5 @@
> > +           }
> > +         }
> > +
> > +         document.getElementById(id).firstChild.nextSibling // second column
> > +           .setAttribute("label", gBundle.getString(bs));
> 
> Don't we align under the dot in these cases?

Maybe, but it's a bit confusing in cases like this. I've changed it now. 

> Does m-c have competitions in writing the most obfuscated code? ;)

Heh, yeah I was happy to just borrow it. Certificate handling is quite involved...

Comment 7

[Magnus Melin [:mkmelin]]

Attachment: bug1293378_cert_status.patch

Comment 8

[:aceman]

How can I test this?

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8999700 [details] [diff] [review]
bug1293378_cert_status.patch

Discovered this still needs some more work to work as intended for all cases.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: bug1293378_cert_status.patch

It turned out the non-ok certificates were already filtered out.

That prompts the question though, should invalid certificates be allowed to use? CA:s would probably say no, but for practical purposes, you might want to do that especially if it's only that the cert expired. If you want to encrypt, you don't have a choice as you can't obtain the recipient cert on the spot - so your only option is to send in clear text.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: [e-mail] (Experied cert for Richard)

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: [e-mail] (Expired cert for [e-mail])

Comment 13

[Magnus Melin [:mkmelin]]

To test, you can go to the settings: Advanced | Certificates. Manage certificates | People. Then import.

After you have the cert imported, write a new mail and put their email in the To field. Then click open Security (in the toolbar). See the results.

Comment 14

[Magnus Melin [:mkmelin]]

Attachment: bug1293378_cert_status.patch

Fixed a bug.

Comment 15

[:aceman]

Comment on attachment 9001522 [details] [diff] [review]
bug1293378_cert_status.patch

Review of attachment 9001522 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I have seen the difference on the sample certificates. The dialog now displays them and marks Expired instead of "Not found".

::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
@@ +180,5 @@
>      }
>      else
>      {
>        let status = document.createElement("label");
> +      status.setAttribute("value", "…");

What does this say? Temporary value until the status string is fetched? May it need to be localized?

@@ +201,5 @@
> +        let someError = results.some(result =>
> +          result.errorCode !== PRErrorCodeSuccess
> +        );
> +        if (!someError) {
> +          listitem.firstChild.nextSibling // second column

Don't you have the 'status' variable for this element?

@@ +244,5 @@
> +             break;
> +           }
> +         }
> +
> +         listitem.firstChild.nextSibling // second column

Don't you have the 'status' variable for this element?

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +1238,3 @@
>    }
>  
> +  // node now contains the first valid (if aRequireValidCert true)

Node

::: mailnews/extensions/smime/src/nsSMimeJSHelper.cpp
@@ +114,5 @@
>          ToLowerCase(email, email_lowercase);
>  
>          nsCOMPtr<nsIX509Cert> cert;
>          if (NS_SUCCEEDED(nsMsgComposeSecure::FindCertByEmailAddress(
> +                           email_lowercase, false, getter_AddRefs(cert))))

Align below nsMsgComposeSecure?

@@ +221,5 @@
>        ToLowerCase(mailboxes[i], email_lowercase);
>  
>        nsCOMPtr<nsIX509Cert> cert;
>        if (NS_SUCCEEDED(nsMsgComposeSecure::FindCertByEmailAddress(
> +                         email_lowercase, true, getter_AddRefs(cert))))

Align below nsMsgComposeSecure?

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #15)
> > +      status.setAttribute("value", "…");
> 
> What does this say? Temporary value until the status string is fetched? May
> it need to be localized?

Yes it's a placeholder. Changed it to "?" now. I wouldn't bother localizing, the value is practically never shown since the validation happens in a split second.

Thanks for the review!

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: bug1293378_cert_status.patch

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3d5c8c87d645
security info dialog doesn't show cert status anymore. r=aceman

Comment 19

[Magnus Melin [:mkmelin]]

It would be nice to uplift, but for that the patch needs some adjustments (due to the listbox removal).

Comment 20

[Jorg K (CEST = GMT+2)]

hg clone https://hg.mozilla.org/releases/comm-esr60 and Bob's your uncle ;-)

Comment 21

[Magnus Melin [:mkmelin]]

That and mozilla-esr60 yeah, I already have those.

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #21)
> That and mozilla-esr60 yeah, ...
Why? Does it need M-esr60 changes?

Comment 23

[Magnus Melin [:mkmelin]]

No, comm-esr60 isn't going to build with mc-trunk

Comment 24

[Jorg K (CEST = GMT+2)]

Right, so you do a try push from comm-esr60 to make sure the adapted patch works. Or is that not feasible?

Comment 25

[alta88]

(In reply to Magnus Melin from comment #6)
> (In reply to :aceman from comment #5)
> > Comment on attachment 8999582 [details] [diff] [review]
> > bug1293378_cert_status.patch
> > 
> > Review of attachment 8999582 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
> > @@ +191,5 @@
> > > +        const SEC_ERROR_UNKNOWN_ISSUER                    = SEC_ERROR_BASE + 13;
> > > +        const SEC_ERROR_UNTRUSTED_ISSUER                  = SEC_ERROR_BASE + 20;
> > > +        const SEC_ERROR_UNTRUSTED_CERT                    = SEC_ERROR_BASE + 21;
> > > +        const SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE        = SEC_ERROR_BASE + 30;
> > > +        const SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED = SEC_ERROR_BASE + 176;
> > 
> > What are these constants (like 176)? Can't we fetch them by names from
> > anywhere?
> 
> Unfortunately we can't. I'm not sure why security things like these don't
> define accessible named constants. I think it has something to do with that
> part is nss code and part is integration code.
> 

I don't think we should do another copy/paste for cert errors.  I've reluctantly had to do it for feeds https://bugzilla.mozilla.org/show_bug.cgi?id=497488#c8 but it would be better to adapt/reuse this in a general utils jsm if there will be multiple callers.

Comment 26

[Magnus Melin [:mkmelin]]

Well nsINSSErrorsService would appear to be the correct place to me. Maybe I should file a bug and see if we can get it into there?

Comment 27

[Magnus Melin [:mkmelin]]

Filed bug 1484464.

Comment 28

[Magnus Melin [:mkmelin]]

Attachment: bug1293378_cert_status.esr60.patch

For 60. Had to include bug 1453778 first

Comment 29

[Jorg K (CEST = GMT+2)]

So you're saying bug 1453778 needs uplift, too?

Comment 30

[Magnus Melin [:mkmelin]]

No i included and adjusted it, included in the esr patch already.

Comment 31

[Jorg K (CEST = GMT+2)]

Attachment: 1293378-esr60.patch

Actually, wouldn't it be cleaner to uplift bug 1453778 first? Then the original patch almost applies with one tweak.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003732 [details] [diff] [review]
bug1293378_cert_status.esr60.patch

Let's not mix two bugs here.

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003764 [details] [diff] [review]
1293378-esr60.patch

This esr60 patch folded on top of the patch from bug 1453778 gives your esr60 patch, so it must be right.

Comment 34

[Magnus Melin [:mkmelin]]

You could look at it either way, this is all just adjusting to current reality. Seems odd to uplift a bug to port a bug which didn't land on esr :/

You have the same end result. I wouldn't spend time fiddling with it.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003764 [details] [diff] [review]
1293378-esr60.patch

Seems odd to mix up two bugs. I'll uplift bug 1453778. I do this sort of thing all the time.

Comment 36

[Jorg K (CEST = GMT+2)]

TB 60 beta 11:
https://hg.mozilla.org/releases/comm-beta/rev/2bc3eb38e9674628f6b15043f4dbb6d2d0770b72
Accidentally I'm the patch author since I refactored Magnus' ESR 60 patch.

Note that the listitem/label processing is quite different:
C-C 63: https://hg.mozilla.org/comm-central/rev/3d5c8c87d645#l1.104
C-B 60: https://hg.mozilla.org/releases/comm-beta/rev/2bc3eb38e9674628f6b15043f4dbb6d2d0770b72#l1.109
That's due to the listitem removal in mozilla63.

Magnus, can you test that this works as desired.

Comment 37

[Jorg K (CEST = GMT+2)]

Working :-)

[e-mail] - Expired - 16 April 2013 - 17 April 2014.

Comment 38

[Jorg K (CEST = GMT+2)]

TB 60.1 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/2af4d611dc8a6f42c106f5444d44de69a65feb7f

Comment 39

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #36)
> Magnus, can you test that this works as desired.

Yes. Forgot to reply earlier (comment 24), but that's why I wanted a local build so I could check it out in action.