Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to AES-256-GCM for openssl_seal/open #25551

Closed
wants to merge 2 commits into from

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Feb 9, 2021

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

@rullzer rullzer added the 2. developing Work in progress label Feb 9, 2021
@rullzer rullzer added this to the Nextcloud 22 milestone Feb 9, 2021
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@faily-bot

This comment was marked as outdated.

$prev = null;

// We need to be able to extract the IV
if (strlen($encKeyFile) > 12) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean we'd always go into this block or do I miss something? Is there a case in which this is =< 12 for older files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure might be for older files yes...

@J0WI
Copy link
Contributor

J0WI commented Feb 17, 2021

#2182 suggests to move to openssl_public_encrypt

This was referenced May 20, 2021
@blizzz blizzz mentioned this pull request Jun 2, 2021
57 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 22, Nextcloud 23 Jun 2, 2021
@skjnldsv skjnldsv force-pushed the enh/encryption/move_to_better_cipher branch from 0b21799 to bc0d48c Compare June 14, 2021 09:31
@rotdrop
Copy link
Contributor

rotdrop commented Jul 18, 2021

Hi, of course, this is not a PHP lesson, so my apologies if I am doing something obviously wrong. Out of curiosity:

I am not able to use openssl_seal()/openssl_open() with the aes-256-gcm cipher. I suspect that PHP does not take the authentication tag into account which is needed to validate the encrypted data. Specifically, the example code works for aes-256-ctr but not for the gcm cipher. I hope the example is fairly minimal.

For demonstration purposes I have also added a direct call to openssl_decrypt(), which of course cannot work for the gcm-cipher. However, neither does openssl_open().

So I wonder why your code is able to decrypt data sealed with the gcm cipher?

<?php

const NUM_KEYS = 4;

for ($i = 0; $i < NUM_KEYS; $i++) {
  $keys[] = $key = openssl_pkey_new();
  $details = openssl_pkey_get_details($key);
  $pubKeys[] = $details['key'];
  $privKeys[] = openssl_pkey_get_private($key);
}

$data = 'Hello World! Blub';

$ciphers = [
  'aes-256-ctr',
  'aes-256-gcm',
];

foreach ($ciphers as $cipherAlgo) {

  echo "*** TESTING $cipherAlgo ***" . PHP_EOL . PHP_EOL;
  $iv = \random_bytes(openssl_cipher_iv_length($cipherAlgo));
  $result = openssl_seal($data, $sealedData, $sealedKeys, $pubKeys, $cipherAlgo, $iv);

  echo 'DATA  : ' . strlen($data) . ' ' . $data . PHP_EOL;
  echo "IV-LEN: " . openssl_cipher_iv_length($cipherAlgo) . PHP_EOL;
  echo "IV    : " . bin2hex($iv) . PHP_EOL;
  echo 'RESULT: ' . strlen($sealedData) . ' ' . bin2hex($sealedData) . PHP_EOL;
  echo PHP_EOL;

  // Try decrypt
  foreach ($keys as $i => $key) {
    $decrypted = null; // ;)
    openssl_open($sealedData, $decrypted, $sealedKeys[$i], $key, $cipherAlgo, $iv);
    echo "OPEN:    " . $decrypted . PHP_EOL;

    openssl_private_decrypt($sealedKeys[$i], $unsealedKey, $key);
    echo "UNSEAL:  " . bin2hex($unsealedKey) . PHP_EOL;
    echo "DECRYPT: " . openssl_decrypt($sealedData, $cipherAlgo, $unsealedKey, OPENSSL_RAW_DATA, $iv) . PHP_EOL;
  }

  echo PHP_EOL;
}

Example output:

*** TESTING aes-256-ctr ***
DATA  : 17 Hello World! Blub
IV-LEN: 16
IV    : 1bf00d7a57cc5d2feb6b6a0b2f60d713
RESULT: 17 302092028ed588efad61223a77419a232a

OPEN:    Hello World! Blub
UNSEAL:  f8c3d61298efd7614f75ebfd1bbcfc3902ac2a5839efdf425e4c64faba67a23a
DECRYPT: Hello World! Blub
OPEN:    Hello World! Blub
UNSEAL:  f8c3d61298efd7614f75ebfd1bbcfc3902ac2a5839efdf425e4c64faba67a23a
DECRYPT: Hello World! Blub
OPEN:    Hello World! Blub
UNSEAL:  f8c3d61298efd7614f75ebfd1bbcfc3902ac2a5839efdf425e4c64faba67a23a
DECRYPT: Hello World! Blub
OPEN:    Hello World! Blub
UNSEAL:  f8c3d61298efd7614f75ebfd1bbcfc3902ac2a5839efdf425e4c64faba67a23a
DECRYPT: Hello World! Blub

*** TESTING aes-256-gcm ***
DATA  : 17 Hello World! Blub
IV-LEN: 12
IV    : 0a37ec3c2884982ae8b43823
RESULT: 17 81048c4414a15177237e85bbc6111f8f57

OPEN:    
UNSEAL:  f87544db170c38c84d67628665d82b4d6248f22e15d4d0ae1b464b6c39954557
DECRYPT: 
OPEN:    
UNSEAL:  f87544db170c38c84d67628665d82b4d6248f22e15d4d0ae1b464b6c39954557
DECRYPT: 
OPEN:    
UNSEAL:  f87544db170c38c84d67628665d82b4d6248f22e15d4d0ae1b464b6c39954557
DECRYPT: 
OPEN:    
UNSEAL:  f87544db170c38c84d67628665d82b4d6248f22e15d4d0ae1b464b6c39954557
DECRYPT: 

@solracsf
Copy link
Member

Any move here? This is becoming a problem with recent OS (Ubuntu 22.04, Fedora 36...) shipping OpenSSL v3 which removes RC4 cipher by default. See #32003

@solracsf
Copy link
Member

/rebase

@rotdrop
Copy link
Contributor

rotdrop commented Jun 29, 2022

Any move here? This is becoming a problem with recent OS (Ubuntu 22.04, Fedora 36...) shipping OpenSSL v3 which removes RC4 cipher by default. See #32003

As I have already stated in my previous posts you just cannot simply move the openssl_seal() stuff to AES-256-GCM. Please see my previous posts for example code and explanations and in particular have a look at php/php-src#7737 and the related report php/doc-en#1210

The problem is that still PHP does not return the authentication tag, so you can happily use the GCM cipher to encrypt data, but you will not be able to decrypt it again because the auth tag is not return by openssl_seal(). If one want to stick to openssl_seal() one has either to somehow push the PHP people to implement authenticated ciphers with openssl_seal() or code this kind of sealing by oneself.

RC4 is not required for openssl_seal(), but no authenticated cipher text will ATM work with openssl_seal() because it does not return the auth tag back to the caller.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@nextcloud-command nextcloud-command force-pushed the enh/encryption/move_to_better_cipher branch from bc0d48c to 905e6d8 Compare June 29, 2022 15:41
@yahesh
Copy link
Member

yahesh commented Jul 14, 2022

A potential solution could be to reimplement the openssl_seal() functionality which is nothing more than encrypting $data with a random $key and then encrypting $key with the provided public keys. This would open up a lot of possibilities:

1.) A new scheme that uses AES-256-GCM could be introduced.
2.) One could finally switch from PKCS 1.5 padding to OAEP padding.
3.) Updating the share list could be sped up as newly shared files wouldn't have to be re-encrypted anymore (but the intermediate would just have to be encrypted for the added users).
4.) A future path would be opened up to get rid of the intermediate key altogether, which IMHO doesn't serve any real purpose.
5.) A fallback-mechanism could be introduced that uses a PHP implementation of RC4 in case OpenSSL v3 is installed.

@come-nc
Copy link
Contributor

come-nc commented Dec 8, 2022

@rullzer @rotdrop @yahesh
Is there any particular reason to use AES-256-GCM? We could switch to any cipher supported by openssl_seal but not obsolete yet, no? What is the added value of ciphers which needs an auth tag?

@yahesh
Copy link
Member

yahesh commented Dec 9, 2022

@come-nc IMHO cryptographic processing should fail early. Currently, the encryption of the filekeys is not integrity-protected, meaning that an attacker can replace the filekey by knowing the recipients' public keys and Nextcloud will use it until finally the HMAC check of the individually encrypted file blocks fails. Using GCM mode would already fail upfront.

I'd be even more thrilled to get rid of openssl_seal and openssl_open entirely as it introduces additional intermediate keys that don't help to gain any security benefits. But that's a totally different story.

@rotdrop
Copy link
Contributor

rotdrop commented Dec 9, 2022

A potential solution could be to reimplement the openssl_seal() functionality which is nothing more than encrypting $data with a random $key and then encrypting $key with the provided public keys. This would open up a lot of possibilities:

1.) A new scheme that uses AES-256-GCM could be introduced. 2.) One could finally switch from PKCS 1.5 padding to OAEP padding. 3.) Updating the share list could be sped up as newly shared files wouldn't have to be re-encrypted anymore (but the intermediate would just have to be encrypted for the added users). 4.) A future path would be opened up to get rid of the intermediate key altogether, which IMHO doesn't serve any real purpose. 5.) A fallback-mechanism could be introduced that uses a PHP implementation of RC4 in case OpenSSL v3 is installed.

... it would even be possible to not use openssl at all and choose something else like Halite. But yes: in order to use autenticated cipher text one has to replace open_ssl() by something similar. If done properly one could then make the use of openssl an option.

@yahesh
Copy link
Member

yahesh commented Dec 28, 2022

... it would even be possible to not use openssl at all and choose something else like Halite. [...]

Which would make it more difficult to install Nextcloud as the php-sodium package would be required. OpenSSL support is typically compiled into PHP and is therefore widely available.

@yahesh
Copy link
Member

yahesh commented Dec 30, 2022

With #35916 I introduced wrapped_openssl_seal() and wrapped_openssl_open() which contains a reimplementation of openssl_seal() and openssl_open(). These functions therefore contain everything that's needed to replace the standard functions with something individual which could also properly handle AEAD tags.

@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz removed this from the Nextcloud 26 milestone Mar 7, 2023
@blizzz
Copy link
Member

blizzz commented Mar 7, 2023

Seems obsoloted by #36173

@blizzz blizzz closed this Mar 7, 2023
@skjnldsv skjnldsv deleted the enh/encryption/move_to_better_cipher branch August 22, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants