Cache AES one-shot cipher handles#129996
Conversation
Reuse ECB and CBC lite ciphers for repeated AES one-shot operations while guarding one-shot and key mutation paths with ConcurrencyBlock. Clear cached ciphers when the key changes or the instance is disposed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
| if (_keyBox is null) | ||
| { | ||
| GenerateKey(); | ||
| Span<byte> key = stackalloc byte[KeySize / BitsPerByte]; |
There was a problem hiding this comment.
We can't use GenerateKey() here because ConcurrencyBlock is not re-entrant. GetKey can be called while a ConcurrencyBlock is already being held and GenerateKey is also under a ConcurrencyBlock. We could have a GenerateKeyUnchecked or similar but given that's is 2 lines of code seemed not worth it.
| encrypting: false)); | ||
|
|
||
| using (cipher) | ||
| using (ConcurrencyBlock.Enter(ref _block)) |
There was a problem hiding this comment.
Even though CFB is not cached it felt odd to leave it out of the ConcurrencyBlock. We can take it out but I want to more or less make it official: don't call these things concurrently.
| private ILiteSymmetricCipher? _decryptEcbCipher; | ||
| private ILiteSymmetricCipher? _encryptCbcCipher; | ||
| private ILiteSymmetricCipher? _decryptCbcCipher; | ||
| private ConcurrencyBlock _block; |
There was a problem hiding this comment.
We use a single ConcurrencyBlock for everything. In theory mix-and-matching concurrent calls to EncryptCbc and DecryptCbc would "work" if we had a per-handle block but that is just making the intentions less clear. I want to avoid "Sometimes concurrency is okay, sometimes it isn't" and just go for "it isn't".
There was a problem hiding this comment.
Pull request overview
This PR updates the internal AesImplementation used by Aes.Create() to cache and reuse ILiteSymmetricCipher handles for AES one-shot ECB/CBC encrypt/decrypt operations, and introduces ConcurrencyBlock to reject concurrent use on the same Aes instance.
Changes:
- Cache ECB/CBC one-shot cipher handles (
_encrypt*/_decrypt*) and reuse them viaReset(iv)(GetOrCreateCachedLiteCipher). - Add
ConcurrencyBlockto serialize one-shot operations and key mutation (KeySize,SetKeyCore, one-shot cores), plus cache clearing on key changes/dispose. - Refactor key initialization to generate key material directly in
GetKey().
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesImplementation.cs | Adds cached one-shot cipher handle reuse and concurrency blocking; refactors key initialization and cache invalidation. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
This caches the handles used by the AES algorithm's one-shot handles so that repeated calls to
{Encrypt,Decrypt}{Cbc,Ecb}can re-use their handles. This means any subsequent calls to these methods don't have to re-create the key handle and do key expansion.Because the previous implementations were more or less thread-safe, but not intentionally, we add
ConcurrencyBlockhere. We do this prevent accidental misuse of these APIs concurrently. Even though they look "safe" because they are one-shots and don't mutate state, that is not the case with the underlying platform's implementations on each handle.For example, in OpenSSL, it internally maintains the previous block of data during CBC processing because it needs to chain the blocks. Using the cipher concurrently is a sure way to lead to data corruption.
We do not do this for CFB because 1. CFB is a less common code and 2. the feedback size is part of the handle on some platforms. That means we would need a cached handle per operation, per feedback size. That makes the
Aesclass bigger with no clear benefit. We also do not do this for 3DES/DES/RC2 because these algorithms should no longer be used on any performance critical path.Summarizing the benchmarks:
"Single-use" calls to the one-shots will have a small allocation regression. This regression is a fixed amount and not dependent on the input size of the data. Because the
Aesclass itself is bigger now to hold these fields, allocating it increased its size by ~40 bytes.The use of
ConcurrencyBlockintroduces minimal overhead and appears to be within noise.Any additional calls to the methods that have been cached will see big improvements. Both in wall-clock time since the handle is being re-used, and allocations are down because they do not allocate the handles. In the right circumstances they are even allocation free.
Benchmark code: https://gist.github.com/vcsjones/f6f571224aff928d7e13d6b7ac3d404e
Benchmark results: