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

ASSIGNED
Assigned to

Status

ASSIGNED
2 years ago
10 hours ago

People

(Reporter: mkmelin+mozilla, Assigned: mkmelin+mozilla)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
+++ 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

2 years ago
(Presumably this isn't actually a blocker - else someone would have already killed this)
Severity: blocker → normal
Flags: needinfo?(mkmelin+mozilla)

Updated

a year ago
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 2

6 days ago
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 :/
(Assignee)

Comment 3

2 days ago
Created attachment 8999582 [details] [diff] [review]
bug1293378_cert_status.patch

Made the status column work again.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8999582 - Flags: review?(acelists)
(Assignee)

Comment 4

2 days ago
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

2 days ago
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? ;)
Attachment #8999582 - Flags: feedback+
(Assignee)

Comment 6

a day ago
(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...
(Assignee)

Comment 7

a day ago
Created attachment 8999700 [details] [diff] [review]
bug1293378_cert_status.patch
Attachment #8999582 - Attachment is obsolete: true
Attachment #8999582 - Flags: review?(acelists)
Attachment #8999700 - Flags: review?(acelists)

Comment 8

21 hours ago
How can I test this?
(Assignee)

Comment 9

18 hours ago
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.
Attachment #8999700 - Attachment is obsolete: true
Attachment #8999700 - Flags: review?(acelists)
(Assignee)

Comment 10

10 hours ago
Created attachment 9000036 [details] [diff] [review]
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.
Attachment #9000036 - Flags: review?(acelists)
(Assignee)

Comment 11

10 hours ago
Created attachment 9000037 [details]
richardmarti@gmailcom.crt (Experied cert for Richard)
(Assignee)

Comment 12

10 hours ago
Created attachment 9000038 [details]
mbanner@mozillacom.crt (Expired cert for mbanner@mozilla.com)
(Assignee)

Comment 13

10 hours ago
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.
You need to log in before you can comment on or make changes to this bug.