1190749 - change pgp encryption from CAST-5 to AES

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[:glob ✱]

https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/SecureMail/Extension.pm#L600

# We use the CAST5 cipher because the Rijndael (AES) module doesn't
# like us for some reason I don't have time to debug fully.
# ("key must be an untainted string scalar")

Comment 1

[Gervase Markham [:gerv]]

Yep, that was me. Fair enough :-)

gerv

Comment 2

[Alex Johnson (:alex_johnson)]

What about IDEA encryption, will it work if you do:

my $encrypted = $pgp->encrypt(
        Compat     => 'PGP2',
        Data       => $text,
        Recipients => "@",
        Armour     => 0
    );

Comment 3

[Alex Johnson (:alex_johnson)]

(In reply to Alex Johnson(:alex_johnson) from comment #2)
> What about IDEA encryption, will it work if you do:
> 
> my $encrypted = $pgp->encrypt(
>         Compat     => 'PGP2',
>         Data       => $text,
>         Recipients => "@",
>         Armour     => 0
>     );

my $encrypted = $pgp->encrypt(
         Compat     => 'PGP2',
         Data       => $text,
         Recipients => "@",
         Armour     => 0
     );

Never mind, it's less secure than CAST5.  I would be interested in figuring out why we can't use AES though.

Comment 4

[Alex Johnson (:alex_johnson)]

Maybe using GnuPG::Interface might help.

Comment 5

[Alex Johnson (:alex_johnson)]

Attachment: Tried switching to GnuPG::Interface.

I tried switching to GnuPG::Interface, but I've been having trouble base64 encoding the result even if I used the same Crypt::OpenPGP::Armour->armor() funciton.

Comment 6

[Dylan Hardison [:dylan] (he/him)]

(In reply to Alex Johnson(:alex_johnson) from comment #5)
> Created attachment 8687697 [details] [diff] [review]
> Tried switching to GnuPG::Interface.
> 
> I tried switching to GnuPG::Interface, but I've been having trouble base64
> encoding the result even if I used the same Crypt::OpenPGP::Armour->armor()
> funciton.

I'm a bit confused by this comment -- is the patch for review in a working state?

Comment 7

[Alex Johnson (:alex_johnson)]

(In reply to Dylan William Hardison [:dylan] from comment #6)
> (In reply to Alex Johnson(:alex_johnson) from comment #5)
> > Created attachment 8687697 [details] [diff] [review]
> > Tried switching to GnuPG::Interface.
> > 
> > I tried switching to GnuPG::Interface, but I've been having trouble base64
> > encoding the result even if I used the same Crypt::OpenPGP::Armour->armor()
> > funciton.
> 
> I'm a bit confused by this comment -- is the patch for review in a working
> state?

It actually isn't.  I'll keep working on it.  Please ignore the review request.

Comment 8

[Alex Johnson (:alex_johnson)]

Attachment: Part 1: Switched to GnuPG::Interface and MIME::Entity

Sorry about the previous review request.  I think this one works though.  Please take a look.

Comment 9

[Alex Johnson (:alex_johnson)]

Attachment: Part 2: Added AES256 cipher-algo

Comment 10

[Alex Johnson (:alex_johnson)]

Attachment: Part 3: Use stringify-body to prevent the mime entity header from showing in message.

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8688807 [details] [diff] [review]
Part 1: Switched to GnuPG::Interface and MIME::Entity

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

This is really great, but I'd like the file-scoped lexicals to be removed. 
Also there are lots of extra white space at the ends of lines that it would be nice to clean up.

::: Bugzilla/GPGMail.pm
@@ +28,5 @@
> +my $homedir = File::Temp->newdir();
> +my $keyring;
> +my $imported;
> +my $comment;
> +

I have concerns for these package-scoped lexicals. I'd rather see them as instance members of $self.
$self->{_bz_homedir}, $self->{_bz_keyring} etc should be enough to avoid any collisions with the parent class.

Do we need to worry about a shared homedir between multiple calls to gpg?

::: extensions/SecureMail/Config.pm
@@ +28,5 @@
> +	{
> +        package => 'Mail::GPG',
> +        module  => 'Mail::GPG',
> +        version => 0,
> +    },

I think we need to explicitly depend on Try::Tiny here. I know it's pulled in anyway, but I believe that's our general guideline.

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8688807 [details] [diff] [review]
Part 1: Switched to GnuPG::Interface and MIME::Entity

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

there were quite a few "style issues" as well -- those can be taken care of with perltidy (for the most part). If you have issues with that get with me on IRC.
Also the GPGMail.pm file will need to go into extensions/SecureMail/lib and its package name will change to Bugzilla::Extension::SecureMail::GPGMail (also already mentioned on irc).

Comment 13

[Alex Johnson (:alex_johnson)]

Attachment: Moved GPGMail.pm to SecureMail/lib, added Try::Tiny to Config and perltidied stuff up.

Comment 14

[Alex Johnson (:alex_johnson)]

Attachment: All the commits folded into one with pertidy run on all the changed files.

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 8695020 [details] [diff] [review]
All the commits folded into one with pertidy run on all the changed files.

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

Please do not run perltidy on existing code and instead adhere to whatever coding style is around the areas you are editing. This makes the reviewer's job much more difficult and time consuming. Adding entirely new files or cleaning just the block of code you are adding, perltidy is not a bad idea but we would still like the new code to also use similar style to the rest of the Bugzilla code. More info (good reference but slightly out of date) can be found at https://www.bugzilla.org/docs/developer.html

dkl

Comment 16

[Alex Johnson (:alex_johnson)]

Attachment: Switch to GnuPG with fixed PerlTidy.

Sorry about the bad perltidy from the previous patch.  I have fixed that here.

Comment 17

[Alex Johnson (:alex_johnson)]

Attachment: Switched to GnuPG with fixed PerlTidy.

I just realized that I'm not using Try::Tiny anywhere.

Comment 18

[Dylan Hardison [:dylan] (he/him)]

I will get to this but it might not be until Saturday. It's quite a lot to read and test.

Comment 19

[Alex Johnson (:alex_johnson)]

Attachment: Fixup for multipart HTML messages

Comment 20

[:glob ✱]

Comment on attachment 8695500 [details] [diff] [review]
Switched to GnuPG with fixed PerlTidy.

this change doesn't need two reviews.

Comment 21

[:glob ✱]

(In reply to Alex Johnson(:alex_johnson) from comment #19)
> Created attachment 8695547 [details] [diff] [review]
> Fixup for multipart HTML messages

when fixing up, please attach a new patch which encompasses all the changes including the fixes instead of a patch that just carries the fix.

Comment 22

[Alex Johnson (:alex_johnson)]

Attachment: Switched to Mail::GPG and MIME::Entity.

Comment 23

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8695671 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity.

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

When using perltidy, it's important to use the perltidyrc file that BMO uses, make sure to copy the .perltidyrc from the BMO repo into your $HOME,
or otherwise instruct your editor to use it. The GPGMail.pm file had tabs (instead of spaces) in it, which shouldn't happen.

::: extensions/SecureMail/Extension.pm
@@ +31,3 @@
>  use Bugzilla::Util qw(correct_urlbase trim trick_taint is_7bit_clean);
>  use Bugzilla::Error;
>  use Bugzilla::Mailer;

Because of the weird way extensions are loaded, it would be better to turn this into "require Bugzilla::Extension::SecureMail::GPGMail" and move it into _pgp_encrypt()

::: extensions/SecureMail/lib/GPGMail.pm
@@ +16,5 @@
> +# Portions created by Mozilla are Copyright (C) 2008 Mozilla Foundation.
> +# All Rights Reserved.
> +#
> +# Contributor(s): Alex Johnson <[e-mail]>
> +

This is the old copyright header, don't use this one. It should look like:
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
#
# This Source Code Form is "Incompatible With Secondary Licenses", as
# defined by the Mozilla Public License, v. 2.0.

Comment 24

[Alex Johnson (:alex_johnson)]

Attachment: Switched to Mail::GPG and MIME::Entity

Moved .perltidyrc to $HOME and ran perltidy again from vim, changed the use to a require and moved it to _pgp_encrypt() and changed the copyright.

Comment 25

[Alex Johnson (:alex_johnson)]

There has been no momentum on this in over 4 months.  Are we still planning on doing this?

Comment 26

[Dylan Hardison [:dylan] (he/him)]

I have not had the time to test it in an environment that would closely match production. We are currently undergoing three very large infrastructure changes (four if you include the AWS migration) as well.

One of these changes will make specifying new CPAN modules far easier, so I suspect that this will be a great help.

Comment 27

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8705398 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity

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

I still do not have the time to *test* this but the code looks good. So it gets f+.
This also uncovered some errors in how extensions work and I'm very happy about that.

::: extensions/SecureMail/lib/GPGMail.pm
@@ +7,5 @@
> +# This Source Code Form is "Incompatible With Secondary Licenses", as
> +# defined by the Mozilla Public License, v. 2.0.
> +#
> +# Contributor(s): Alex Johnson <[e-mail]>
> +

We use MPL 2 style copyright headers, so this would need to be changed. Minor nit.

Comment 28

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8705398 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity

You have a patch that is doing something like this, right? That you've been able to test? At the time this work was done I had no easy way of testing it and it may have bit-rotten, but getting it into SecureMail would be awesome.

Comment 29

[Jeff Fearn 🐞]

Yeah I have it working on RH Beta Bugzilla, you can test it yourself if you have an account on there, it's a dump from production from about February or March IIRC.

I changed the code in Extension.pm, I didn't add any extra files.

If you have a PGP key with sub keys and your encryption key isn't the first sub key, say your first sub key is a signing key and the second is an encryption key, then you can test it properly. Otherwise you are just testing what it can already do. It has been tested with some pretty complex key structures.

Mail out is disabled, but I added a hacky preference to bypass that for crazy people, like us. The check box with the big red danger message next to it.

https://beta-bugzilla.redhat.com/userprefs.cgi?tab=securemail

Just add/change your PGP key and click the check box, then submit, and it will send you the test mail.

You might want to disable that preference shortly after!

If you tell me the repo and branch you'd like a patch against I can diff it for you.

FWIW I also have the corresponding changes for email_in.pl, it makes PGP signing (not encryption) mandatory for incoming mail, you can also test this on the beta as mail-in is enabled. It will silently swallow mail that doesn't map to a real account or isn't signed, or where the from address and PGP don't match, or if the PGP key isn't already in Bugzilla, etc, etc.

Comment 30

[Jeff Fearn 🐞]

Comment on attachment 8705398 [details] [diff] [review]
Switched to Mail::GPG and MIME::Entity

Nacking this because it isn't similar to what I've given to Dylan and I'm not in a position to validate this version fully.

Couple of notes, the hard coded values (paths, exe, etc) should be parameters (e.g. I prefer gpg2), using key servers should either be disabled or made optional. There is no authentication on key servers, anyone can add a key, limiting keys to those entered in to Bugzilla limits exposure to some degree.