1759293 - Cannot add an expiry date to an OpenPGP key that doesn't have one

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[SnipFoo]

Steps to reproduce:

1. Create a new OpenPGP key pair, and set it to never expire (by selecting option "Key does not expire" in the creation dialog).

2. In the OpenPGP Key Manager, open the properties of the corresponding key pair (by double-clicking on it for instance), then click on "Change Expiration Date."

3. Select "Key will expire in", and select a positive number of months (e.g., 24).

4. Click "OK" to confirm, then "OK" again to close the "Key Properties" dialog.

Actual results:

After following these steps, the expiration date for this key pair remains unset.

Expected results:

The expiration date for this key pair should have been set to 24 months in the future.

Comment 1

[SnipFoo]

This is due to the fact that the EnigmailKeyObj::iSimpleOneSubkeySameExpiry() method doesn't set the fingerprints in its result parameter when the key doesn't expire (that is, when its expiryTime property is 0), as per modules/keyObj.jsm, line 601:

    if (!this.expiryTime && !subKey.expiryTime) {
      return true;
    }

Consequently, the fprInfo.fingerprints array computed on line 46 of ui/changeExpiryDlg.js is empty, and the call to RNP.changeExpirationDate() on line 142 of the same file is called with no fingerprints to update.

Changing the EnigmailKeyObj::iSimpleOneSubkeySameExpiry() method to set the fingerprints even when the key doesn't expire seems to fix the issue.

The diff for this simple patch follows:

--- a/mail/extensions/openpgp/content/modules/keyObj.jsm
+++ b/mail/extensions/openpgp/content/modules/keyObj.jsm
@@ -599,6 +599,10 @@ class EnigmailKeyObj {
     let subKey = this.subKeys[0];
 
     if (!this.expiryTime && !subKey.expiryTime) {
+      if (result) {
+        result.fingerprints.push(this.fpr);
+        result.fingerprints.push(subKey.fpr);
+      }
       return true;
     }
 

Thanks!

SnipFoo.

Comment 2

[SnipFoo]

Attachment: keyObj.patch

Comment 3

[Magnus Melin [:mkmelin]]

Thanks for tracking it down.
I think we should stop having an in-out parameter for iSimpleOneSubkeySameExpiry (and just have that return true/false).

Then do gFingerprints = [keyObj.fpr, keyObj.subKeys[0].fpr] at
https://searchfox.org/comm-central/rev/839a5c425e68f252eea073c15c62faa06ead0517/mail/extensions/openpgp/content/ui/changeExpiryDlg.js#46

Would you be able to update the patch?
(Also see moz-phab, https://developer.thunderbird.net/thunderbird-development/fixing-a-bug#submitting-a-patch)

Comment 4

[SnipFoo]

Attachment: Bug 1759293 - Remove in-out parameter for iSimpleOneSubkeySameExpiry. r=mkmelin

Comment 5

[SnipFoo]

Great, thank you for the feedback!

I've tried to submit a patch along the lines you suggested. I also had to remove a part of the test in test_secretKeys.js (since it also expected the fingerprints to be in the output parameter of iSimpleOneSubkeySameExpiry()). From what I could find with hg grep, these were the only two calls to iSimpleOneSubkeySameExpiry() which used its result parameter.

Also, this is my first time using moz-phab, so I hope I haven't messed things up too much :)

Thanks, in any case!

SnipFoo.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to SnipFoo from comment #5)

Also, this is my first time using moz-phab, so I hope I haven't messed things up too much :)

Everything went well! :)

I'll add the checkin-needed-tb keyword now, so the patch will get checked in soon.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a1609c797ef7
Remove in-out parameter for iSimpleOneSubkeySameExpiry. r=mkmelin

Comment 8

[SnipFoo]

That's perfect, thank you very much for your precious help in seeing this patch through! :)

SnipFoo.