CRM-21085 Replace mcrypt functions where possible with openssl versio…

[seamuslee001]

…ns and add in upgrade step to convert

Overview

mcrypt functions are deprecated as of PHP7.1 and need to be replaced and there seems to be an obvious openSSL version we can use

Before

mcrypt was used to encrypt passwords

After

openssl is used for encryption

Technical Details

as per description changing encryption styles

Comments

@totten @xurizaemon @eileenmcnaughton i think this on the face of it should cover things. However we will need to alert extension authors to this and down stream folks. This is one area that makes it difficult manage but its an issue we need to address

[eileenmcnaughton]

I don't feel very qualified to comment on this one but I see jenkins does :-)

[seamuslee001]

@eileenmcnaughton ha ha :-) I have tested this a few times on upgrade and it seems to be correctly coverting the encrypted passwords from mycrpt to openssl

[seamuslee001]

Jenkins re test this please

[davejenx]

@seamuslee001 Well done getting this under way! A few thoughts...

1. Crypto library

In a Mattermost discussion of Mcrypt, @totten suggested using phpseclib:
"fwiw, Civi includes a copy of phpseclib, which is wrapper that falls back to the best available implementation (eg openssl, mcrypt, pure-php). Rewriting CRM_Utils_Crypt with that (while preserving the crypto params) would also fix the compatibility issue."

2. Compatibility

The compatibility issue mentioned above was described at https://civicrm.stackexchange.com/questions/19914/why-does-outbound-email-stop-working-after-enabling-the-php-mcrypt-extension

Re compatibility, I responded to Tim's above Mattermost post:
"IIUC this would fix the compatibility issue going forwards, in that once we switch to phpseclib, future enablings of PHP extensions shouldn't break stored values. But it would break backwards compatibility in the case where the server doesn't currently have PHP Mcrypt, so values stored via CRM_Utils_Crypt are just base64_encoded whereas the new CRM_Utils_Crypt::decrypt code would expect them to be encrypted."

I think your convertMcryptToOpenSSL() method would handle the backwards compatibility (mcrypt vs base64) issue at upgrade time, as the decrypt method with $useMcrypt = TRUE would use mcrypt if installed, base64 otherwise. But only for the smtpPassword field, which AFAIK is the only field that core uses CRM_Utils_Crypt for. See below re extensions / external code.

3. Crypto method

Tim posted re this on the security channel, 11th August, 20:34 BST = 19:34 UTC. That may be the best place to take up discussion of which method to use. One possibility may be to keep the existing method but using the phpseclib wrapper, removing the dependency on mcrypt while reducing the backwards compatibility issue (would just need to handle the mcrypt vs base64 issue for existing values).

4. Extensions / external code

As you mention, there's a need to alert extension authors. I know com.cividesk.email.sparkpost uses CRM_Utils_Crypt. This is important I think, as e.g. the Sparkpost extension would stop working after the core upgrade unless something was done to migrate the stored API key. Extension authors would need to implement upgrade functions and we'd get into the issue of the current version of the extension effectively only being compatible with core <= 4.7.24 say.

If we decided to keep the same crypto method but using phpseclib (assuming that's feasible), that would I think reduce this problem. On servers that have mcrypt installed, the existing stored values would have been encrypted and should decrypt OK. On servers without mcrypt, we'd still have the problem.

[totten]

I've been a little bit confused about the scoping of the PR. The title/description suggests that it's a formulaic change for compatibility (replacing one library with another work-a-like), but in reality it's also a policy change which uses different crypto params. (AES is a variant of Rijndael, but AES-256 is different from mcrypt's Rijndael-256. ) This is (part of) why the on-disk format changes and why the PR requires re-encrypting the data.

Generally, I feel there are two plays that make the most sense. Either:

• (a) Drop-in replacement: Strictly preserve the on-disk format and the crypto params, but use a different implementation (eg phpseclib/openssl).
• (b) Design cleanup: Deprecate CRM_Utils_Crypt. Keep it around (so we can still read/write the old format), but design a different API and on-disk format. Anything that calls CRM_Utils_Crypt should probably call something else.
  • Why? Because CRM_Utils_Crypt has several issues, like (a) deriving the storage-crypto key from a rest-authentication key and (b) using ECB mode and (c) protecting one arbitrarily chosen field and (d) allowing the stored password to become corrupt based on runtime changes (enabling/disabling a PHP module). The only argument I can make for the status-quo is that... the flaws of CRM_Utils_Crypt don't matter because its use-case is narrow and obscure.
  • Tackling this is a bigger scope-of-work, though. You need to step back and ask different questions.

I've been trying to figure out how we'd write a drop-in replacement. Haven't figured out how to do it with Crypt_Rijndael (the obvious params didn't work), but in case it helps... here's my attempt to figure it out: https://gist.github.com/totten/2a269885e86f4515e17aad962cda1160

[seamuslee001]

I was aiming as a drop in replacement and i believed that this was the closest to the mcrypt which i thought was the case, i would prefer a drop in replacement and see this as being part of it, I think b is good but going to be crazy to solve

[totten]

Well, I guess it depends on how urgent this really is:

• If the status-quo is that all PHP 7.1 sites have a hard break, then one can argue that it's urgent, and we might accept design-compromises to deliver quickly.
• If the status-quo is that PHP 7.1 sites get a deprecation notice (depending on their reporting/settings/thresholds), then it's not really urgent. We can take our time and only push a change to the data-structure when we think it's Really Better.

[adixon]

See https://issues.civicrm.org/jira/browse/CRM-21120 for a related issue that isn't addressed by this one, though I'd have to rewrite my PR to cover this change. Short version - the problem of changing platforms that do/don't include the crypt functions cause silent failure in decrypting.

[eileenmcnaughton]

I'm just trying to figure out the status on this one - ie is there a reasonable path forwards for it to be reviewed or is is stalled pending time to try a new approach (in which case we should close & re-open when ready)

I note mcrypt is only deprecated in 7.1 & I think we recently added a check to require it.

Note that I think the PR queue should be full of things that are predominantly in the reviewer's court or actively being worked on. Nothing is lost (IMHO) by closing stalled PRs & re-opening them when unblocked

[eileenmcnaughton]

This one is stale so I'm going to close for now - it looks like it might be worth agreeing the scope in gitlab before re-basing & re-opening as this was stalled on 'discussion'

[jusfreeman]

Discussion in progress here, https://lab.civicrm.org/dev/core/issues/236#note_6419