Runtime: AesCng much slower than AesCryptoServiceProvider

[sdrapkin]

I get the following timings on Windows 2012R2 with freshly-installed .NET 4.6.2 final and running in 64bit release mode (via 64bit LinqPad):

CNG 00:00:02.3278742
CSP 00:00:00.0392550

CNG 00:00:02.3332983
CSP 00:00:00.0408160

CNG 00:00:02.2949133
CSP 00:00:00.0398848

CNG 00:00:02.2959543
CSP 00:00:00.0392522

The quick & crude benchmark code: https://gist.github.com/anonymous/39929346722e641027b313a45a4f44ab

Is this perf difference intentional/expected?

[bartonjs]

I have replicated these results. I'm not sure where it'd be going wrong, because A) Since Win7 CSP has been a legacy wrapper over CNG, B) they both look dead simple.

So this is going to require some time in a profiler.

[karelz]

We need confirmation if the slow down is in .NET Core code (something we could potentially change) or in Windows libraries (something we can't change).

[sdrapkin]

I assume this issue is assigned to someone on the corefx team to resolve. The new AesCng is unusable since it takes over 2 seconds to ECB-encrypt 16 bytes.

Is someone finally taking personal ownership/responsibility to debug & fix, or are _we_ going to continue describing what _we_ need, such as "time in a profiler", "finding where the slowdown is", etc.?

[karelz]

@sdrapkin no, it is not assigned currently to anyone. It is marked "up for grabs" - so anyone from .NET Core community can take it and work on it.

We do not consider the issue critical for next release (1.2) - the perf difference is there for quite some time and this is the first and only complaint, so it didn't make the cut. Also there is non-trivial chance the problem is in Windows code - external issue to .NET Core.
If it has huge impact on significant number of customers we are not aware of, please let us know.

[bartonjs]

Investigation has happened without coming back to update this issue, sorry (we had one internal, and one on github, and they weren't linked).

We have a pretty good handle on the problem, partly because it stopped reproing in .NET Core. Local testing showed over a 100x improvement (when calling TransformBlock 16 bytes at a time), making it faster than AesCryptoServiceProvider (which matches our expectations of AesCryptoServiceProvider -> CAPI -> CNG being slower than AesCng -> CNG), and hopefully the change can make it into the next release of .NET Framework.

That said, unless you're using AesCng specifically to use a named/persisted key, you shouldn't use that type. Or AesCryptoServiceProvider. Or AesManaged. (And definitely not RijndaelManaged). You "should" just use Aes.Create(). (If that had FIPS mode problems we fixed that in 4.6.2... but I don't remember if AES was one of the problem children, or not).

[sdrapkin]

Thank you for finally addressing this issue.

unless you're using AesCng specifically to use a named/persisted key, you shouldn't use that type.

You and I might know that. However, most MSDN readers and .NET API consumers likely do not. Can documentation be improved? Typically, the right way to indicate that an API should not be used is to not make it public - either at the class level, or at the undesired-ctor level.

You "should" just use Aes.Create().

Not so fast. Aes.Create() is ~38% slower on .NET 4.7 than new AesCryptoServiceProvider(), even though both return an instance of AesCryptoServiceProvider. Should I file another issue for that, or will that also be resolved in the next .NET framework release?

While the next .NET Framework release is awaited, can you share details on how you have fixed the problem? Perhaps I could do an adhoc fix myself to get a 100x improvement now on .NET 4.7?

[bartonjs]

Can documentation be improved?

We're working on tooling for "you really shouldn't be calling this". Which is sort of like, but different from, deprecation. All of the algorithm-implementation ctors will probably be on that list.

the right way to indicate that an API should not be used is to not make it public

I agree, having the AES implementation types (except AesCng) be public was not a great decision. AesCng is public because it exposes CNG persistent/out-of-proc keys.

Not so fast. Aes.Create() is ~38% slower on .NET 4.7 than new AesCryptoServiceProvider(), even though both return an instance of AesCryptoServiceProvider

The method Aes.Create() slower than directly calling a constructor? Sure, it has to be slower, there's a dictionary lookup in CryptoConfig involved, and a reflect-construct. Aes objects can be long-lived, each CreateEncryptor()/CreateDecryptor() is unrelated to any other. Many middleware frameworks use object caches for SymmetricAlgorithm objects, which also helps with garbage collection.

can you share details on how you have fixed the problem? Perhaps I could do an adhoc fix myself to get a 100x improvement now on .NET 4.7?

It's deep down in the plumbing. We treated the ephemeral keys like persisted keys, and had to pay for that with out-of-proc operation overhead. By separating the pathways for ephemeral vs persisted keys we avoided the IPC overhead, which was almost the entirety of the cost (it dwarfed the cryptographic operation).

[gulbanana]

the suggestion of reusing an Aes and just making new encryptors/decryptors sounds good, but how would you deal with IVs? GenerateIV(), like GenerateKey(), modifies the Aes itself - which means you can't cache the thing, it wouldn't be threadsafe either

[sdrapkin]

@gulbanana There are CreateEncryptor(key, iv) and equivalent CreateDecryptor overloads, which take key and iv explicitly. However, that would require one to _not_ use Aes for kev/iv generation or containment. Another serious problem is that Aes is also responsible for Chaining and Padding modes - I think changing these would cause all Aes-derived CryptoTransforms to behave differently, thus breaking the independence.

The bottom line is that Aes and CryptoTransforms were never designed to be independent, and treating Aes as a stateless factory for independent CryptoTransforms is dangerous -- I don't agree with that suggestion either.

[bartonjs]

Another serious problem is that Aes is also responsible for Chaining and Padding modes - I think changing these would cause all Aes-derived CryptoTransforms to behave differently, thus breaking the independence.

All of our types are independent. CreateEncryptor passes the PaddingMode (et cetera) to the new object, not a reference back to the Aes (et al) object.

GenerateIV(), like GenerateKey(), modifies the Aes itself - which means you can't cache the thing

Sure you can.

```C#
if (!s_concurrentQueue.TryDequeue(out aes))
{
aes = Aes.Create();
}

// do stateful stuff with the object here, like call GenerateIV()

// When done:
s_concurrentQueue.Enqueue(aes);
```

The bottom line is that Aes and CryptoTransforms were never designed to be independent,

I disagree. They very much seem to be designed to be independent. Of course, third-party implementations may not be as rigorous. But using an object cache (instead of "one big shared object") doesn't have that problem anyways.

[gulbanana]

that's a pretty cool way to pass around ownership! things get tricky if you're using the CNG model where an algorithm instance "is" a key or the CryptoApi CSP stuff where an instance "is" a handle to some piece of specialised hardware- they might not be so blithely poolable- but it works for some useful cases.

[jnm2]

things get tricky if you're using the CNG model where an algorithm instance "is" a key

Yeah, because someone decided we weren't even going to try to add to .NET Core the existing .NET concept of encrypting memory at rest to make it vanishingly unlikely to get paged out to disk in the clear, aka SecureString doing what it says... 😈 (Anyone remember heartbleed by the way? Private keys sitting unencrypted in memory?)

So I'm with you, trying to minimize the number of copies of the private key by keeping it inside the crypto objects.

[sdrapkin]

The mechanics of obj-pool caching are trivial - SqlClient code does the same thing. However, note that the ConcurrentQueue in SqlClient never shrinks - thus it is forever stuck at largest concurrent number of crypto-calls ever made, which is incredibly wasteful, can be a potential security risk, puts needless pressure on the LOH, etc, etc.

On the other hand, creating a fresh CNG-based Aes instance adds an additional NCrypt handle creation/duplication cost and GC work, but saves on queuing/dequeing. Can NCrypt run out of handles, and thus require a pooling approach (done properly)? If yes, then it should be built-in and not consumers' responsibility. If no, then isn't the handle creation/duplication cost negligible?