mogilvie / EncryptBundle

Encryption bundle
89 stars 29 forks source link

[Malfunction] `onEncryptionLoadKey` purpose gets violated by `postLoad` #21

Closed Volmarg closed 1 year ago

Volmarg commented 3 years ago
Since I want to use Your package in next project I will try to explain the issue the best I can right now.

Flow of data - sending between projects

image

Details

parameters:
    encrypt_key: "0000000000000000000000000000000000000000000" # (random key - not used - needs to be set to suppress the encryption bundle error)

Flow of data - user logs in

image

Cron call no 1. - Insert Passwords Groups

foreach($insertRequest->getPasswordsGroupsArrays() as $passwordGroupArray){
   $passwordGroupEntity = PasswordGroup::fromArray($passwordGroupArray);
   ...
   $this->passwordGroupController->save($passwordGroupEntity);
}

Status: Works

Cron call no 2. - Insert Passwords and assign them to groups

foreach($insertRequest->getPasswordsArrays() as $passwordArray){
    $passwordEntity      = Password::fromArray($passwordArray);
    $passwordGroupEntity = $this->passwordGroupController->getOneForId($passwordEntity->getGroupId());
    ...
    $this->passwordController->save($passwordEntity);
}

Status: Does not work

Under which circumstances the problem happens

  1. The passwordGroupEntity is already saved with first cron call, and has correct encryption string from PMS (1),
  2. Now I fetch the passwordGroupEntity from DB
  3. Seems like Your bundle decrypts the values in there
  4. I set the relation between passwordGroupEntity and passwordEntity
  5. I save the passwordEntity
  6. Doctrine messes up encrypts the passwordGroupEntity fields with key stored in yaml which is 0000

Where is Your code related to the issue

            if($isEncryptOperation) {
                $encryptedValue = $this->encryptor->encrypt($value);
                $refProperty->setValue($entity, $encryptedValue);
            } else {
                $decryptedValue = $this->decryptValue($value);
                $refProperty->setValue($entity, $decryptedValue); 
                //we don't want the object to be dirty immediately after reading
                $unitOfWork->setOriginalEntityProperty($oid, $refProperty->getName(), $value);
            }

This makes sense because "Hello how will Your bundle know that key is replaced otherwise?". Yet the current situation makes the onEncryptionLoadKey kinda useless.

End note

I've spent tones of time trying to find out why / where this happens. I cannot propose the solution as I don't fully understand why such thing has place. I have my own solution in project which is saving via plain sql - I can use that in current project, but next one is a no since I want to encrypt entire DB.

mogilvie commented 3 years ago

Hi Volmarg,

The encryptor/decryptor is constructed with the default configuration key.

When getSecretKey is called by an encryption or decryption process it allows you to insert an alternative key using the event dispatcher. If the event returns null then the defined configuration key is used.

Take a look at the following in the OpenSSLEncryptor. Check if the subscriber you are using in the second system is returning a key.

 private function getSecretKey(){

        // Throw an event to allow encryption keys to be defined during runtime.
        $getKeyEvent = new EncryptKeyEvent();
        $this->dispatcher->dispatch($getKeyEvent, EncryptKeyEvents::LOAD_KEY);

        // If the event is returned with a key, then override the parameter defined key.
        if(null !== $getKeyEvent->getKey()){
            $this->secretKey = $getKeyEvent->getKey();
        }

        // If the key is still empty, then throw an exception.
        if(empty($this->secretKey)){
            throw new EncryptException('The bundle specshaper\encrypt-bundle requires a parameter.yml value for "encrypt_key"
            Use cli command "php bin/console encrypt:genkey" to create a key, or set via a listener on the EncryptKeyEvents::LOAD_KEY event');
        }

        // Decode the key
        $key = base64_decode($this->secretKey);

        $keyLengthOctet = mb_strlen($key, '8bit');

        if (32 !== $keyLengthOctet) {
            throw new \Exception("Needs a 256-bit key, '".($keyLengthOctet * 8)."'bit given!");
        }
Volmarg commented 3 years ago

Hi,

I know what You mean, and I agree with what You wrote. The thing here is that the second system has a default key 000000..., and the real key is being set only upon logging-in by the user. Sending key via API is not allowed.

Therefore the api-endpoint in PMS-IO (target project which takes the encrypted data), does not have the real key. I would expect the data just to be inserted in DB without the decrypt, and then encrypt again.

mogilvie commented 3 years ago

Forgive the repetition, I'm trying to get my head around the intent.

Project No1. - PMS (some sort of back end admin system?) A user is created, a password is entered, and a unique key is generated to encrypt the password. The user, complete with encrypted password and key string is persisted to PMS-DB. These details are then communicated back to a user somehow? Is that a concern that the credentials need to be transmitted together?

Project No2 - PMS-Bridge I'm not following this so well. Is this a separate symfony project? or does it exist in the same environment as Project No 1 as the green shading might indicate?

Does project 1 send the new user+password+key to Project 3 using a the cron in Project 2? Or is it just the basic user details and an encrypted password?

Are user credentials transmitted just on a scheduled cron job to fetch new users and transfer them across projects? The Symfony Messenger Component might be something to consider.

Why is this bidirectional (black arrows between project 2 and project 1) Does a new user registration in Project 3 also get persisted back to Project 1?

Project No3 - PMS-IO Some user facing application where the user logs in with username, password, and encryption key. Are new user details registered in Project 3 persisted back into project 1?

Suggestions I think the issue is in the highlighted box below.

image

In your authentication you have a form that gets submitted with a username, password and encryption key. The normal guard authentication will query the DB for any user credentials to match the username. If no match, then the authentication will fail.

However, if the username exists, then the authentication will load the user entity in order to get the DB stored hashed password. The authentication system then hashes the user supplied login form password and compares it against the DB stored hashed password. If they don't match then authentication fails.

The problem is that when the authenticator is loading the user entity, if you have marked the password as @encrypted via this bundle, then the postLoad doctrine event listener will try and decrypt the password. If the user specific encrypt key doesn't exist in project 3 then the encryptKeyEvent object returns nothing and the decryptor will default to the parameter defined encryption key. It will decrypt the password incorrectly. The comparison between the login form password/key combination and the DB stored encrypted password will not be successful.

My suggestion is to create your own authenticator in project 3 that queries the PMS-IO DB with a select statement, instead of hydrating the entire user entity. This would avoid triggering the doctrine postLoad event and will allow you to encrypt your login form password with the login form supplied encryption key, and compare that against the DB stored encrypted password. If this is successful then you grant the user authentication, and persist or store the encryption key for future use in Project 3.

Having written all the above, I'd be concerned about a public login form where the user can enter their password, and an encryption key, and get confirmation back that the password/key combination works or doesn't.

I would also be concerned that if the Project 1 or 3 DB was compromised, and the encryption key and the password are in the same set of data, then it could be reverse engineered to decrypt the rest of the encrypted fields. Or perhaps you are only temporarily storing the encrypt key in session for Project 3 somehow?

Volmarg commented 3 years ago

Yeah, it’s normal – get a headaches at work sometimes as well trying to understand what’s going on.

Project No1. - PMS

Project No. 2 - PMS-Bridge

Project No3 – PMS-IO

Validation form

This works a bit differently.

So actually this what You wrote here

My suggestion is to create your own authenticator in project 3 that queries the PMS-IO DB with a select statement, instead of hydrating the entire user entity. This would avoid triggering the doctrine postLoad event and will allow you to encrypt your login form password with the login form supplied encryption key, and compare that against the DB stored encrypted password. If this is successful then you grant the user authentication, and persist or store the encryption key for future use in Project 3.

Is not actually needed because the user login-in is fine.

Concerns

Data compromising

More highlight

Generally both projects are for private usage only – I created them on my own for personal usage – the code source of both is accessible for everyone, but I write this all for myself

Project No1. PMS

Project No3. PMS-IO

and so on – I’m a bit obsessed with that because like You said data breach and I don’t want this.

So still, there is some problem with that code that I mentioned up there, I wonder if it would be somehow possible to save the data into DB using standard persist / flush without triggering decryption and encryption with local key. In my opinion the data should only be saved in DB in that logic, nothing else should happen there.

mogilvie commented 1 year ago

Just coming back to close this due in activity, but before I do:

In your last statement:

So still, there is some problem with that code that I mentioned up there, I wonder if it would be somehow possible to save the data into DB using standard persist / flush without triggering decryption and encryption with local key. In my opinion the data should only be saved in DB in that logic, nothing else should happen there.

You could use executeStatement on an SQL. These don't trigger the doctrine events, so won't encrypt/decrypt with the subscriber. For example:

 $result = $this->em->getConnection()->executeStatement(
            'UPDATE subscription_xero_oauth_token SET access_token = ?, refresh_token = ?, expires = ? WHERE id = ?',
            [
                $accessToken->getToken(),
                $accessToken->getRefreshToken(),
                $accessToken->getExpires(),
                $tokenID,
            ]
        );