search

gosa-project / gosa-core

GOsa core
GNU General Public License v2.0
16 stars 14 forks source link

PHP 7.4 chokes on a flaw in cred_encrypt() and cred_decrypt() that pr… #33

Open sunweaver opened 4 years ago

sunweaver commented 4 years ago

…evious PHP versions silently ignored.

PHP 7.4 chokes with...

  Fatal error: Uncaught Error: Length must be greater than 0 in
  /usr/share/gosa/include/functions.inc:3324 Stack trace:
  #0 /usr/share/gosa/include/functions.inc(3324): openssl_random_pseudo_bytes()
  #1 /usr/share/gosa/include/class_config.inc(310): cred_decrypt()
  #2 /usr/share/gosa/include/class_config.inc(362): config->get_credentials()
  #3 /usr/share/gosa/include/class_configRegistry.inc(408): config->get_ldap_link()
  #4 /usr/share/gosa/include/class_config.inc(453): configRegistry->reload()
  #5 /usr/share/gosa/include/class_config.inc(441): config->load_servers()
  #6 /usr/share/gosa/html/index.php(267): config->set_current()
  #7 {main}
  thrown in /usr/share/gosa/include/functions.inc on line 3324

... because the chosen method (aes-256-ecb) doesn't use an initialization vector ($iv) at all, causing its length ($ivlen) to be 0, see e.g. https://usr.ed48.com/php/ssl/?xf=7

So the encrypt/decrypt implementation seems to have been sort of wrong before (and only now with PHP 7.4 an error is thrown).

See Debian bug https://bugs.debian.org/964318

master-caster commented 3 years ago

Is there a reason to remove the cipher from the parameter list in cred_encrypt ?

Have you tried to decrypt a previously encrypted gosa.conf ?

dzatoah commented 2 years ago

I stumbled across this in the new gosa_v2.8_PHP8.1 branch. We definitely should consider merging ASAP.

dzatoah commented 2 years ago

Maybe we should use a more sophisticated approach.

We can assemble an array of ciphers which don't need an init vector. (is this too maintenance-heavy maybe?) Then we match the desired cipher with the list and decide if we use the "old" method or sunweavers approach.

I think this would ensure backwards compatibility and a fix for default aes-256-ecb.

EDIT:

Or instead we just check $ivlen, if null then use new approach, if not null then use old approach :)

dzatoah commented 2 years ago

So I've implemented my idea, please consider merging quickly.

dzatoah commented 2 years ago

So... I've tested this quite a bit now.. @master-caster can you review this again? :)