Add modern cryptors by olegbaturin · Pull Request #71 · yiisoft/security

olegbaturin · 2026-05-16T11:38:08Z

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌

add tests

codecov · 2026-05-16T11:40:04Z

Codecov Report

❌ Patch coverage is 94.14414% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.00%. Comparing base (552f49e) to head (fa57456).

Files with missing lines	Patch %	Lines
src/Crypto/Cipher/OpenSSLAeadCipher.php	91.17%	3 Missing ⚠️
src/Crypto/Cipher/OpenSSLWrapCipher.php	89.65%	3 Missing ⚠️
src/Crypto/Cipher/SodiumAeadCipher.php	94.11%	2 Missing ⚠️
src/Crypto/Kdf/KdfKey.php	86.66%	2 Missing ⚠️
src/Crypto/Kdf/KdfPasswordPbkdf2.php	87.50%	2 Missing ⚠️
src/Crypto/Kdf/KdfPasswordArgon2.php	92.85%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##              master      #71      +/-   ##
=============================================
- Coverage     100.00%   96.00%   -4.00%     
- Complexity        37      133      +96     
=============================================
  Files              7       16       +9     
  Lines            103      325     +222     
=============================================
+ Hits             103      312     +209     
- Misses             0       13      +13

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

# Conflicts: # .github/workflows/build.yml

samdark

The truncated tag issue from my previous review is still there. The reply in that thread isn't correct: openssl_decrypt() sets the tag at whatever length it's given and compares only those bytes. With the default CHACHA20-POLY1305 a zero-length tag is always accepted.

I've checked it at the current head: KdfCryptor with default KdfKey and OpenSSLAeadCipher decrypts any 44 random bytes to an empty string without knowing the secret, and the same goes through VersionedCryptor. EnvelopeCryptor does the same for any real ciphertext cut down to its header. Details are inline.

samdark · 2026-10-27T21:00:34Z

+        if (StringHelper::byteLength($nonce) !== $this->nonceSize) {
+            throw new EncryptionException("Nonce must be {$this->nonceSize} bytes long.");
+        }
+


There's still no length check here. If $data is shorter than 16 bytes, the whole input becomes the tag and OpenSSL checks only that many bytes. An empty input with CHACHA20-POLY1305 always decrypts to '', and a 1-byte tag passes about once in 256 tries for any of the allowed ciphers. Please reject short input before slicing:

Suggested change

if (StringHelper::byteLength($data) < self::TAG_SIZE) {

throw new EncryptionException('Encrypted data is too short.');

}

samdark · 2026-10-27T21:00:34Z

+        string $secret,
+        string $context = '',
+    ): string {
+        if (StringHelper::byteLength($data) < $this->headerLength) {


The minimum length doesn't include the cipher tag, so a header-only payload (salt + nonce, 44 bytes by default) gets to the cipher with empty data. Combined with the cipher issue, that's an empty-string forgery without the secret. The cryptor shouldn't depend on every CipherInterface implementation to catch it:

Suggested change

if (StringHelper::byteLength($data) < $this->headerLength) {

if (StringHelper::byteLength($data) < $this->headerLength + $this->cipher->getOverheadSize()) {

samdark · 2026-10-27T21:00:34Z

+        string $secret,
+        string $context = '',
+    ): string {
+        if (StringHelper::byteLength($data) < $this->headerLength) {


Same here: the data tag isn't part of headerLength. Any real ciphertext cut down to its header passes the wrapped DEK check and then decrypts to ''.

Suggested change

if (StringHelper::byteLength($data) < $this->headerLength) {

if (StringHelper::byteLength($data) < $this->headerLength + $this->cipher->getOverheadSize()) {

samdark · 2026-10-27T21:00:34Z

+        $tagSize = $cipherInstance->getOverheadSize();
+
+        $this->expectException(EncryptionException::class);
+        $cipherInstance->decrypt(substr($ciphertext, 0, -$tagSize), $key, $nonce); // remove auth tag


This doesn't cover the bug: the plaintext is 15 bytes, so after removing the tag there are still 15 bytes left, and they're used as the tag. Please add cases for decrypt(''), every input length from 1 to 15, and an empty plaintext with its tag truncated. Also test KdfCryptor/EnvelopeCryptor with real ciphers rather than mocks, using input truncated to the header length. These should fail on the current code.

samdark · 2026-10-27T21:00:35Z

+        }
+
+        [$this->keySize, $nonceSize] = self::ALLOWED_CIPHERS[$this->cipher];
+        $this->dummyNonce = str_repeat("\0", $nonceSize);


For AES-*-WRAP, OpenSSL doesn't ignore the IV. It uses it as the integrity check value. With zeros, the output doesn't match RFC 3394: the §4.1 vector gives faacb56e… instead of 1fa68b0a…. It's still authenticated, but no other AES-KW implementation can unwrap it, and once released the format can't be changed. Use the RFC default IV, update the docblock above, and replace the current expected values in OpenSSLWrapCipherTest with the RFC 3394 test vectors.

Suggested change

$this->dummyNonce = str_repeat("\0", $nonceSize);

$this->dummyNonce = str_repeat("\xA6", $nonceSize);

samdark · 2026-10-27T21:00:35Z

+        if (StringHelper::byteLength($key) !== $this->keySize) {
+            throw new EncryptionException("Key must be {$this->keySize} bytes long.");
+        }
+


decrypt('') returns '' instead of throwing. Wrapped data is at least 16 bytes and a multiple of 8, so please check that:

Suggested change

$length = StringHelper::byteLength($data);

if ($length < 16 || $length % 8 !== 0) {

throw new EncryptionException('Wrapped data must be at least 16 bytes long and a multiple of 8 bytes.');

}

samdark · 2026-10-27T21:00:35Z

+use const OPENSSL_RAW_DATA;
+
+/**
+ * Key wrapping cipher using OpenSSL (RFC 5649 / AES-KW).


AES-*-WRAP is RFC 3394. RFC 5649 is the padded variant (-WRAP-PAD).

Suggested change

* Key wrapping cipher using OpenSSL (RFC 5649 / AES-KW).

* Key wrapping cipher using OpenSSL (RFC 3394 / AES-KW).

samdark · 2026-10-27T21:00:35Z

+    '__construct()' => [
+        'cryptors' => ReferencesArray::from([
+            "\x01" => Reference::to(KdfCryptor::class),
+            "\x96" => Reference::to(EnvelopeCryptor::class},


Typo, } instead of ).

Suggested change

"\x96" => Reference::to(EnvelopeCryptor::class},

"\x96" => Reference::to(EnvelopeCryptor::class),

olegbaturin added 17 commits May 11, 2026 17:33

add cryptor

917f747

fix style

6156063

add readonly

c5bd2ac

update doc

90205a7

add kdf doc

be74463

add doc to the cryptors

1e2dbd0

replace mb_substr with StringHelper

395e497

replace mb_strlen

74ab9ee

update config

ab88999

fix config

e3d033e

update envelope cryptor

f3371b9

add tests

add copher tests

024ab0f

add kdf tests

a9b2fed

fix psalm

e3b42cc

add versioned test

1ada6d7

remove config

e30b789

update changelog

c31e2e2

olegbaturin added 12 commits May 16, 2026 18:48

fix style

14c6555

fix style

5a261fc

fix style

7469377

fix style

b53adc4

fix psalm

dd005ec

update tests

ef21a64

update composer.json

b2cb9da

update ci

ac79a08

update ci

bf24623

update ci

8b64a32

update phpdoc

1001c88

update readme

2750e71

olegbaturin added 4 commits June 20, 2026 01:05

update readme kdf

33e578d

update tests

fb4b9b0

fix ci

473428d

update readme examples

75181ab

olegbaturin requested review from samdark and vjik June 20, 2026 17:07

olegbaturin and others added 23 commits June 21, 2026 04:14

update readme

5c125ce

update ciphers

326c18b

update composer-require-checker

8d7bd75

fix tests

0fb370d

update readony classes

a7cae86

update psalm

8a74908

Merge branch 'master' into add-cryptor

da307c1

fix style

4e66f4d

update readme

4e09f62

update internals doc

37127e0

update internal docs

fb09b54

update workflow

ce0b037

updateworkflow

cb7ccae

update kdf key tests

4641627

update tests

e461e57

update kdfkey test

f0df068

update phpdoc

388f071

fix readme

ff92620

update kdf interface

bd24826

fix test

8fbaccb

update readme

8e7db7d

Merge remote-tracking branch 'origin/master' into HEAD

a8e401b

# Conflicts: # .github/workflows/build.yml

Merge branch 'master' into add-cryptor

fa57456

samdark requested changes Oct 27, 2026

View reviewed changes

Navigation Menu

Repository navigation

Add modern cryptors - #71

Add modern cryptors#71
olegbaturin wants to merge 74 commits into
yiisoft:masterfrom
olegbaturin:add-cryptor

olegbaturin commented May 16, 2026

Uh oh!

codecov Bot commented May 16, 2026 •

edited

Loading

Uh oh!

samdark left a comment

Uh oh!

samdark Oct 27, 2026

Uh oh!

samdark Oct 27, 2026

Uh oh!

samdark Oct 27, 2026

Uh oh!

samdark Oct 27, 2026

Uh oh!

samdark Oct 27, 2026

Uh oh!

samdark Oct 27, 2026

Uh oh!

samdark Oct 27, 2026

Uh oh!

samdark Oct 27, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

+        if (StringHelper::byteLength($data) < self::TAG_SIZE) {
+            throw new EncryptionException('Encrypted data is too short.');
+        }

	if (StringHelper::byteLength($data) < $this->headerLength) {
	if (StringHelper::byteLength($data) < $this->headerLength + $this->cipher->getOverheadSize()) {

	$this->dummyNonce = str_repeat("\0", $nonceSize);
	$this->dummyNonce = str_repeat("\xA6", $nonceSize);

+        $length = StringHelper::byteLength($data);
+        if ($length < 16 || $length % 8 !== 0) {
+            throw new EncryptionException('Wrapped data must be at least 16 bytes long and a multiple of 8 bytes.');
+        }

	* Key wrapping cipher using OpenSSL (RFC 5649 / AES-KW).
	* Key wrapping cipher using OpenSSL (RFC 3394 / AES-KW).

	"\x96" => Reference::to(EnvelopeCryptor::class},
	"\x96" => Reference::to(EnvelopeCryptor::class),

Conversation

olegbaturin commented May 16, 2026

Uh oh!

codecov Bot commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

samdark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented May 16, 2026 •

edited

Loading