[Encryption] New component #39344

Nyholm · 2020-12-05T23:07:24Z

Q	A
Branch?	6.0
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR

This PR introduces a new Encryption component. But wait, NO. That is not what we are doing.

We are not doing encryption

This component does not do encryption. We do not invent a way to protect your data. This is an encryption abstraction component. So the only thing we do is to define two interfaces and provide a sodium implementation of them.

The interfaces

There is an KeyInterface and a EncryptionInterface. They look something like this:

interface KeyInterface
{
    public function extractPublicKey(): self;
}

interface EncryptionInterface
{
    public function generateKey(string $secret = null): KeyInterface;
    public function encrypt(string $message, KeyInterface $myKey): string;
    public function encryptFor(string $message, KeyInterface $recipientKey): string;
    public function decrypt(string $message, KeyInterface $key, KeyInterface $senderPublicKey = null): string;
}

There are some documentation on the interfaces, they can be good to read if one is not sure what the methods are doing.

Keys

The idea behind these interfaces is that you create a Key. That key can be used for both symmetric and asymmetric encryption. That key can be serialized and stored on disk or in database. You can extract the public part of your Key.

You as a user need to know what type of key to have. You cannot load a random key from disk and use it with encryptFor(). You need to know that it was Alice's private key. Ie, Key management is out of scope of the component.

Keys cannot be transfered to a different implementation. (Just like PSR-6). So you cannot use SodiumEncryption to create a Key and then use it with PhpseclibEncryption.

Encryption

The encryption interface provides an explicit way how to handle encryption. There are 3 scenarios:

encrypt(): When you don't share the encrypt/decrypt keys
encryptFor(): When you want to send a message to Bob

For someone who worked with encryption they know that this is no rocket science. But for someone who takes their first step into encrypting in PHP, they probably look at PHP's Sodium "documentation" and have no idea where to start. This EncryptionInterface will be super helpful for them.

Portability

( This section is where I accept all kinds of "you are doing it wrong" 😄 )

The output of all EncryptionInterface::encrypt*() methods is a string. That string has a specific format.
The format is implemented in the Ciphertext class. Here is how the final string look like:

{"alg": "sodium"} . encryption-output . hash

With some psudo-code:

$nonce = random_bytes(16);
$ciphertext = sodium_encrypt('my message', $nonce);
$headers = json_encode(['alg'=>'sodium_encrypt', 'ver'=>'1', 'nonce' => $nonce]);
$hash = hash('sha256', $headers.$ciphertext);

$output = sprintf('%s.%s.%s',
           base64_encode(headers),
           base64_encode($ciphertext),
           base64_encode($hash)
        );

But why do we need this format?

We do need a format. We need to store some meta data, the ciphertext and the nonce somewhere. To concatenate these together seams like the simplest solution.

The alg header is basically a routing key to tell the difference between “Algorithm not supported” and “Wrong key was used to decrypt message”. Also to make the keys somewhat portable. Ie, it is possible for a non-encryption-component-application to decrypt the key. Note that there is no algorithm negotiation. Note also that the hash here is not to make things "more secure", it is just for an integrity check.

This format will also fall under or BC policy.

I have looked at using JWE, but I was not able to follow the specification to 100% to create the Authentication Tag (Im not sure it is possible in PHP...) and I've also seen posts about not to use JWE. JWE also made things much more complex.

The name of the component

I am not a big fan of the name of this component. What I've noticed that just because it is called "Encryption", then people think that we are doing advanced security stuff. This component will never compete with Sodium, Phpseclib or Halite. It will only make a very thin abstraction on top of it. This abstraction will work for 80% of your encryption needs. For the other 20% you need more flexibility or customisation. That is where you use Sodium, Phpseclib, Halite etc directly.

Ie, this component is not a good fit for "That other team's Java application sends us encrypted data, can we decrypt it?".

src/Symfony/Component/Security/Core/Encryption/SymmetricEncryptionInterface.php

fabpot

Looks georgous and simple! Thank you for doing it. Sorry for the not so useful comments, but I've focused on the text instead of the code for this first review.

Two questions that come to mind:

Would it make sense to have a dedicated component instead of having it in security? I can see many usages for which I would not need anything else from the Security components.

We are using "encryption" in the secret feature (part of framework bundle) and in the Mime component. Any code that could be moved to this new hypothetical encryption component?

src/Symfony/Component/Security/Core/Encryption/AsymmetricEncryptionInterface.php

src/Symfony/Component/Security/Core/Encryption/SodiumEncryption.php

src/Symfony/Component/Security/Core/Encryption/SymmetricEncryptionInterface.php

src/Symfony/Component/Security/Core/Exception/EncryptionException.php

Nyholm · 2020-12-06T09:30:54Z

Thank you for the review. I've updated the PR accordingly.

Would it make sense to have a dedicated component instead of having it in security?

Yes, it will be a small component, but I do think so.

We are using "encryption" in the secret feature (part of framework bundle) and in the Mime component. Any code that could be moved to this new hypothetical encryption component?

Looking at the SodiumVault: Yes, we can use these abstractions. They authors have used the sodium keypair as "decryption key" which I fail to understand why, But we can discuss that in a separate issue.

In the MimeComponent we have a DkimSigner, SMimeEncryoter and SMimeSigner, these cannot be leveraged by the SymmetricEncryptionInterface or AsymmetricEncryptionInterface.

The current implementation is an abstractionSymmetricEncryptionInterface or AsymmetricEncryptionInterface, you are not able to chose algorithm. The output of encrypt() is following the pattern of cipher.algorithm.nonce, so it is possible for a non-php application to decrypt it, but they need to know our arbitrary values of algorithm.

fabpot · 2020-12-06T09:34:21Z

There is no such thing as a "small component" :) History showed us that most small components grew over time with new needs are discovered.
So, I'm 👍 for a new component.

wouterj · 2020-12-06T11:06:38Z

👍 for a new component (I would even prefer a component outside the Security namespace) and moving its framework integration to the FrameworkBundle.

There already are a couple features in the framework bundle that can benefit from this new component I think, e.g. encrypting cache values (refs #35019) and encrypting session data (refs #35804).

src/Symfony/Component/Security/Core/Encryption/AsymmetricEncryptionInterface.php

src/Symfony/Component/Security/Core/Encryption/PhpseclibEncryption.php

src/Symfony/Component/Encryption/Ciphertext.php

Nyholm · 2020-12-07T09:59:54Z

I need some more work to support JWE. Using JWE has the benefit that we more easily can share the encrypted strings with other applications.

JWE exists of 5 parts:

header: a json object defining what algorithms we use
CEK: The encoded key for the symmetric encryption
Initialization Vector: Nonce for symmetric encryption
Cipher text: The encrypted payload
Authentication tag: used for integrity checks

So we use symmetric encryption for the payload and asymmetric to encrypt the symmetric key.

Nyholm · 2020-12-07T23:31:39Z

So, we are compatible with the JWE format.. almost. I cannot figure out the JWE Authentication Tag.

Here is an example for RFC7516:

Perform authenticated encryption on the plaintext with the AES GCM algorithm using the CEK as the encryption key, the JWE Initialization Vector, and the Additional Authenticated Data value above, requesting a 128-bit Authentication Tag output.

So basically: "Take a bunch of data, encrypt it with AES GCM and from that encryption you get ciphertext X and Authentication Tag Y".

But when I look at ie sodium or libsecphp, I find no encryption algoritm where I can get two outputs.. Am I missing something?

However, We are following the same protocol and the ciphertext is secure, but it is not according to the spec (not so good for portability)

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

src/Symfony/Component/Encryption/Provider/PhpseclibEncryption.php

Nyholm · 2020-12-08T19:52:35Z

I liked JWE because it mean that our payload could be portable. However, JWE introduced a lot of complexity and I didn't manage to add a Authentication Tag according to the spec. So.. just complexity and no probability. I changed back to a more simple implementation.

Nyholm · 2020-12-08T21:47:18Z

Im trying out something new. Im making the API more explicit by introducing a KeyInterface. See 12804bb

sstok · 2020-12-09T08:56:28Z

Sorry for this harsh comment, but it's noting personal: DON'T EVER ROLL YOUR OWN ENCRYPTION!

I'm no crypto expert but I already see some things that are potentially dangerous.

The message and private-key are passed-in as plain text, Halite requires that the them passed as a HiddenString to prevent stack traces from exposing sensitive information.
// Check if Authentication Tag is valid $hash = hash('sha256', $headersString.$payload.$nonce);
Authenticated encryption should use hmac-based hashing.
Ciphertext::create('RSAES-PKCS1-v1_5' https://paragonie.com/blog/2018/04/protecting-rsa-based-protocols-against-adaptive-chosen-ciphertext-attacks (might be wrong on this one)

Why do you think Sodium was created? 😄 Cryptographic is a complex topic and with my limited knowledge I can already spot some things I'm not entirely sure of, but I know have caused major problems in the past. If you have sodium, use it. PHP has it bundled already, and it is the most secure standard of today. RSA and DSA should only used for legacy systems or by experts who know how the handle these complexities.

ping @paragonie-scott

jvasseur · 2020-12-09T09:31:30Z

Sorry for this harsh comment, but it's noting personal: DON'T EVER ROLL YOUR OWN ENCRYPTION!

We are not creating our own encryption here but just creating a wrapper that relies on other lower level encryption libraries (including Sodium).

….php Co-authored-by: Robin Chalas <chalasr@users.noreply.github.com>

src/Symfony/Component/Encryption/EncryptionInterface.php

jderusse · 2021-07-19T22:17:03Z

src/Symfony/Component/Encryption/Sodium/SodiumKey.php

+        return $key;
+    }
+
+    public static function fromPrivateAndPublicKeys(string $privateKey, string $publicKey): self


not used and class is internal

I believe these method will be very useful when we use the encryption component for the "symfony secrets".

I have the feeling that having some usages of this new component in core would help moving forward here.
@Nyholm Maybe could you open a PR doing so based on this PR? Or commit here directly?

jderusse · 2021-07-19T22:17:25Z

src/Symfony/Component/Encryption/Sodium/SodiumKey.php

+        return $key;
+    }
+
+    public static function fromKeypair(string $keypair): self


src/Symfony/Component/Encryption/SymfonyEncryptionToken.php

paragonie-security

The current implementation needs work before it can be a general-purpose encryption component for all Symfony users.

paragonie-security · 2021-08-03T09:21:44Z

src/Symfony/Component/Encryption/Sodium/SodiumKey.php

+            $key->secret = $secret;
+        } else {
+            // Trim the key to a good size
+            $key->secret = substr(sha1($secret), 0, \SODIUM_CRYPTO_SECRETBOX_KEYBYTES);


First, sha1() is not an appropriate function to use here.

But even worse, this is hex-encoded so you're getting roughly half of the expected entropy out of your secret from this sha1 invocation.

- $key->secret = substr(sha1($secret), 0, \SODIUM_CRYPTO_SECRETBOX_KEYBYTES); + $key->secret = mb_substr(hash('sha256', $secret, true), 0, \SODIUM_CRYPTO_SECRETBOX_KEYBYTES, '8bit');

paragonie-security · 2021-08-03T09:25:09Z

src/Symfony/Component/Encryption/SymfonyEncryptionToken.php

+
+        // Check if data has been modified
+        $hash = hash('sha256', $headersString.$ciphertext);
+        if (!hash_equals($hash, $hashSignature)) {


Misnomer: $hashSignature isn't, at all, a hash signature.

If you were doing the right thing, it would be a MAC, not a signature, but this just a SHA256 hash without any secrets. This is trivially bypassed: Just calculate hash('sha256', $headersString.$ciphertext) for any arbitrary data you want to feed in.

You're probably better off calling this a checksum, or equivalent, so as to not over-sell the security properties you're shooting for.

This setup also looks susceptible to canonicalization attacks. If the intent here is to have something like HMAC, you'll want to also revisit this.

fabpot · 2021-12-11T14:26:01Z

@Nyholm What do we do here?

Nyholm · 2021-12-11T15:11:01Z

Im gathering courage to make this mergable. I will not stall more than another month.

OskarStark · 2021-12-11T17:42:57Z

Must target 6.1 then, right?

soatok · 2021-12-21T16:41:10Z

src/Symfony/Component/Encryption/Sodium/SodiumEncryption.php

+            throw new DecryptionException(sprintf('Failed to decrypt message with algorithm "%s".', $algorithm), $exception);
+        }
+
+        if (false === $output) {


!is_string() would be a better condition for static analysis.

ro0NL · 2021-12-21T17:45:19Z

s/encryption/encryptor/ everywhere? 😅

fabpot · 2022-07-21T09:24:52Z

Let's close for now

Nyholm requested review from chalasr and wouterj as code owners December 5, 2020 23:07

carsonbot added Status: Needs Review Feature labels Dec 5, 2020

Nyholm added the Security label Dec 5, 2020

carsonbot changed the title ~~Adding symmetric and asymmetric encryption~~ [Security] Adding symmetric and asymmetric encryption Dec 5, 2020

Nyholm commented Dec 5, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Encryption/SymmetricEncryptionInterface.php Outdated Show resolved Hide resolved

Nyholm force-pushed the encryption branch from 91ec85f to 9a447b4 Compare December 6, 2020 00:31

fabpot requested changes Dec 6, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 6, 2020

jderusse reviewed Dec 6, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Encryption/AsymmetricEncryptionInterface.php Outdated Show resolved Hide resolved

src/Symfony/Component/Security/Core/Encryption/PhpseclibEncryption.php Outdated Show resolved Hide resolved

Nyholm mentioned this pull request Dec 6, 2020

[Security] Login link: Encrypt username #39275

Closed

jderusse added this to the 5.x milestone Dec 6, 2020

Nyholm removed the Security label Dec 6, 2020

Nyholm changed the title ~~[Security] Adding symmetric and asymmetric encryption~~ [Encryption] Adding symmetric and asymmetric encryption Dec 6, 2020

Nyholm force-pushed the encryption branch from 4a6c310 to e4c1e55 Compare December 6, 2020 21:29

Nyholm commented Dec 6, 2020

View reviewed changes

src/Symfony/Component/Encryption/Ciphertext.php Outdated Show resolved Hide resolved

norkunas reviewed Dec 8, 2020

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

jderusse reviewed Dec 8, 2020

View reviewed changes

src/Symfony/Component/Encryption/Provider/PhpseclibEncryption.php Outdated Show resolved Hide resolved

Nyholm force-pushed the encryption branch from 629a134 to 7708a90 Compare December 8, 2020 23:14

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 18, 2021

OskarStark requested a review from stof July 18, 2021 19:05

carsonbot added Status: Needs Review and removed Status: Reviewed labels Jul 18, 2021

OskarStark requested review from fabpot, nicolas-grekas and derrabus July 18, 2021 19:05

derrabus approved these changes Jul 18, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 18, 2021

Update src/Symfony/Bundle/FrameworkBundle/Resources/config/encryption…

fe7806f

….php Co-authored-by: Robin Chalas <chalasr@users.noreply.github.com>

jderusse reviewed Jul 19, 2021

View reviewed changes

paragonie-security suggested changes Aug 3, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Aug 3, 2021

paragonie-security mentioned this pull request Aug 10, 2021

What would a hypothetical sodium_compat v2 look like? paragonie/sodium_compat#137

Closed

X-Coder264 mentioned this pull request Sep 13, 2021

Encrypter contract has no getKey() method laravel/framework#38772

Closed

fabpot modified the milestones: 6.0, 6.1 Nov 3, 2021

soatok reviewed Dec 21, 2021

View reviewed changes

chalasr mentioned this pull request Jan 31, 2022

[RFC] Move SECRETS in a new components📦️ #44151

Closed

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

fabpot closed this Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Encryption] New component #39344

[Encryption] New component #39344

Nyholm commented Dec 5, 2020 •

edited

Loading

fabpot left a comment

Nyholm commented Dec 6, 2020

fabpot commented Dec 6, 2020

wouterj commented Dec 6, 2020

Nyholm commented Dec 7, 2020

Nyholm commented Dec 7, 2020 •

edited

Loading

Nyholm commented Dec 8, 2020

Nyholm commented Dec 8, 2020

sstok commented Dec 9, 2020

jvasseur commented Dec 9, 2020

jderusse Jul 19, 2021

Nyholm Jul 19, 2021

chalasr Jul 30, 2021

jderusse Jul 19, 2021

paragonie-security left a comment

paragonie-security Aug 3, 2021

paragonie-security Aug 3, 2021

soatok Aug 3, 2021

fabpot commented Dec 11, 2021

Nyholm commented Dec 11, 2021

OskarStark commented Dec 11, 2021

soatok Dec 21, 2021

ro0NL commented Dec 21, 2021

fabpot commented Jul 21, 2022

[Encryption] New component #39344

[Encryption] New component #39344

Conversation

Nyholm commented Dec 5, 2020 • edited Loading

We are not doing encryption

The interfaces

Keys

Encryption

Portability

The name of the component

fabpot left a comment

Choose a reason for hiding this comment

Nyholm commented Dec 6, 2020

fabpot commented Dec 6, 2020

wouterj commented Dec 6, 2020

Nyholm commented Dec 7, 2020

Nyholm commented Dec 7, 2020 • edited Loading

Nyholm commented Dec 8, 2020

Nyholm commented Dec 8, 2020

sstok commented Dec 9, 2020

jvasseur commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paragonie-security left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Dec 11, 2021

Nyholm commented Dec 11, 2021

OskarStark commented Dec 11, 2021

Choose a reason for hiding this comment

ro0NL commented Dec 21, 2021

fabpot commented Jul 21, 2022

Nyholm commented Dec 5, 2020 •

edited

Loading

Nyholm commented Dec 7, 2020 •

edited

Loading