Skip to content

Conversation

buskamuza
Copy link
Contributor

@buskamuza buskamuza commented May 10, 2018

1. Possibly, 2.3.x patch version. The implementation should be fully backward compatible
2. Use Sodium library for encryption, as this is the latest encryption library supported natively by the latest PHP version (PHP 7.2)
3. Ensure encryption is possible on PHP 7.1, which is also supported by Magneto 2.3
4. Data is migrated to the new algorithm if necessary
Copy link
Contributor

Choose a reason for hiding this comment

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

migrated to be compatible with the new algorithm ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

## 3. Data migration

* Limited or expected-to-be small amount of data to be converted during upgrade process
* Large amount of data to be migrated on the fly: the data is re-encrypted when read and stored again during application work. Currently used encryption algorithms are secure enough to allow the data stay.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should specify a procedure of converting all the remaining data at some point. Because not all data will be accessed over the specific period of time.

The time of the upgrade was a concerns for SIs during migration to Magento 2.2 because of serialize to json_encode conversion, we need to make sure to prevent similar issues this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if we don't migrate the data, the application will work fine, we will still support old algorithms. While, for serialize -> json_encode task, we had to migrate all the data during upgrade because we had to eliminate usages of serialize.
I'll add to the document (as a separate PR) as we investigate options for data migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added example of CLI implementation.

@piotrekkaminski
Copy link

piotrekkaminski commented May 11, 2018

Adding comment from @paragonie-scott on the difference between aead and secretbox functions:

This really comes down to a matter of taste.

  • crypto_aead_xchacha20poly1305_{en,de}crypt uses XChaCha20-Poly1305 (eXtended-nonce ChaCha20 with the Poly1305 authenticator)
  • crypto_secretbox uses XSalsa20/20-Poly1305 (eXtended-nonce Salsa20/20 with the Poly1305 authenticator)

Given the two options: I'd choose crypto_aead_xchacha20poly1305_ietf_encrypt() and crypto_aead_xchacha20poly1305_ietf_decrypt() over the secretbox API, but neither option is bad.

  1. ChaCha8 has a better diffusion than Salsa20/8, which makes cryptographers more confident in its full round variant (ChaCha20 vs Salsa20/20). See http://www.ecrypt.eu.org/stream/papersdir/2007/010.pdf for the Salsa20 weakness (which doesn't extend to Salsa20/20).
  2. The AEAD interface allows you to include additional data in the calculation of the authentication tag.

Let's say you wanted to encrypt password hashes in the database, per user.

A naive approach that would work would be this:

  private function setPassword($password, $key)
  {
      $hash = password_hash($password, PASSWORD_DEFAULT);
      $nonce = random_bytes(24);
      $encrypted = sodium_crypto_aead_xchacha20poly1305_encrypt(
           $hash,
           $this->rowid,
           $nonce,
           $key
      );
      return base64_encode($nonce . $encrypted);
  }
  private function verifyPassword($password, $ciphertext, $key)
  {
      $decoded = base64_decode($ciphertext);
      $nonce = mb_substr($decoded, 0, 24, '8bit');
      $payload = mb_substr($decoded, 24, NULL, '8bit');

      $decrypted = sodium_crypto_aead_xchacha20poly1305_decrypt(
           $payload,
           $this->rowid,
           $nonce,
           $key
      );
      if (!is_string($decrypted)) {
          return false;
      }
      return password_verify($password, $decrypted);
  }

The benefit of this API change is subtle.

Consider the following scenario where you have access to a website's database (via read/write SQL injection) but not the filesystem (so, not the keys).

If you wanted to steal users' passwords, you'd need to first be able to decrypt the hashes then crack them. To do so you need the key. If you can't steal it from the filesytem, both secretbox and AEAD approaches will stop this attack. So far so good.

Suppose the attacker simply wants to impersonate a user. So what they do is:

  1. Register an account, for which the password is known.
  2. Replace their target's encrypted password hash with the password hash for their dummy account, using an UPDATE query.
  3. Login to their target's account with the known password.

In this case, the AEAD approach stops the attack since the ciphertext will fail to decrypt, because their serial IDs are mismatched. The secretbox approach will not, because it has no context; it only cares about encryption and decryption.

While I do prefer the AEAD approach in my own designs, if you don't foresee any use for the additional data being included in the authenticated tag and the thin margin between the security postures of Salsa20 and ChaCha aren't a concern to you, feel free to use secretbox (which uses xsalsa20 instead of xchacha20).

@paliarush paliarush merged commit 372f890 into magento:master Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants