One Shot ECB

[vcsjones]

This is a work-in progress for ECB one shots. Remaining tasks:

• XML documentation
• The base implementation in SymmetricAlgorithm throws a NotImplementedException. Ideally for existing inherited classes, they can fallback to the transform methods
• Cache the handles used by the one shot instead of opening and closing them per-invocation. Consideration needs to be considered with invalidating the handles. (Using the setter on the Key property for example)
• AesCng, TripleDESCng, etc. is not implemented.
  • Implement and test in-memory keys
  • Implement and test persisted ncrypt keys.
• Decide if we want to implement this on the CryptoServiceProviders.
• Test edge cases and bad inputs

Implementation notes so far:

1. Overlapping buffers for our own implementations is not supported for decryption. This is because handling padding is problematic. We can't validate the padding until it's been decrypted, but if we decrypt directly in to a user buffer and then throw, we'll leave bad data in the decrypt's output buffer. If an overlapping buffer is used, we allocate a new one and copy after the padding is validated.
2. Overlapping buffers for encryption is supported. Most crypto implementations support in-place encryption if they overlap with an offset of zero. If the offset is not zero, we rent, encrypt, copy.
3. AesCryptoServiceProvider, AesManaged, and RijndaelManaged will not get a custom implementation since they are being obsoleted.

My intention is to implement ECB "fully" and get it reviewed, then it can more or less serve as a pattern for CBC and CFB.

Contributes to #2406

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is a work-in progress for ECB one shots. Remaining tasks:

• XML documentation
• The base implementation in SymmetricAlgorithm throws a NotImplementedException. Ideally for existing inherited classes, they can fallback to the transform methods
• Cache the handles used by the one shot instead of opening and closing them per-invocation. Consideration needs to be considered with invalidating the handles. (Using the setter on the Key property for example)
• AesCng is not implemented.

Implementation notes so far:

1. Overlapping buffers for our own implementations is not supported for decryption. This is because handling padding is problematic. We can't validate the padding until it's been decrypted, but if we decrypt directly in to a user buffer and then throw, we'll leave bad data in the decrypt's output buffer. If an overlapping buffer is used, we allocate a new one and copy after the padding is validated.
2. Overlapping buffers for encryption is supported. Most crypto implementations support in-place encryption if they overlap with an offset of zero. If the offset is not zero, we rent, encrypt, copy.
3. AesCryptoServiceProvider, AesManaged, and RijndaelManaged will not get a custom implementation since they are being obsoleted.

Author:	vcsjones
Assignees:	-
Labels:	area-System.Security, new-api-needs-documentation
Milestone:	-

[vcsjones]

@bartonjs This is not yet complete but, as time permits for you, this is probably nearing close to done minus the remaining tasks above and could be reviewed at least for design / approach.

[bartonjs]

I wonder (genuine curiosity, not presupposing an answer) if doing subclass for ICryptoTransform vs OneShot would be goodness like with the places we use subclassing for Array vs Span (e.g. RSA)

[vcsjones]

Not sure I follow, can you expand on that a bit?

[bartonjs]

We already have a bunch of tests that are encrypting or decrypting AES (and 3DES, etc) via the CreateEncryptor/CreateDecryptor methods.

• Move the data verification tests to a class different than the property validators and such (assuming they aren't already).
• Make all the methods instance methods, instead of static methods.
• Make them call (e.g.) protected abstract byte[] Decrypt(Aes cipher, ReadOnlySpan<byte> iv, ReadOnlySpan<byte> ciphertext, PaddingMode paddingMode, CipherMode blockMode, int feedbackSize);
• Make N derived types, AesCipherTests_CryptoTransform, AesCipherTests_OneShot, (AesCipherTests_CryptoTransform_Chunked, AesCipherTests_CryptoTransform_OneByteAtATime, etc)
• Each derived type writes its version of Encrypt/Decrypt, and now all of the same scenario/theory/whatever data is being tried in all the input models.

Comparative example:

runtime/src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/SignVerify.netcoreapp.cs

Lines 8 to 20 in d71182a

	public sealed class SignVerify_Span : SignVerify
	{
	protected override byte[] SignData(RSA rsa, byte[] data, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding) =>
	TryWithOutputArray(dest => rsa.TrySignData(data, dest, hashAlgorithm, padding, out int bytesWritten) ? (true, bytesWritten) : (false, 0));
	protected override byte[] SignHash(RSA rsa, byte[] hash, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding) =>
	TryWithOutputArray(dest => rsa.TrySignHash(hash, dest, hashAlgorithm, padding, out int bytesWritten) ? (true, bytesWritten) : (false, 0));
	protected override bool VerifyData(RSA rsa, byte[] data, byte[] signature, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding) =>
	rsa.VerifyData((ReadOnlySpan<byte>)data, (ReadOnlySpan<byte>)signature, hashAlgorithm, padding);
	protected override bool VerifyHash(RSA rsa, byte[] hash, byte[] signature, HashAlgorithmName hashAlgorithm, RSASignaturePadding padding) =>
	rsa.VerifyHash((ReadOnlySpan<byte>)hash, (ReadOnlySpan<byte>)signature, hashAlgorithm, padding);

[bartonjs]

(OneShot can also then use OneShot_InPlace, OneShot_ForwardOverlapped, OneShot_NegativeOverlapped, etc)

[vcsjones]

Hm, I typically shy away from that approach, mostly as a matter of preference and more confidence that we were not losing any existing test coverage mistakenly through a refactoring.

I see the desire though. One of my remaining check items is some test work. I'll noodle with it to see if turns in to something I don't dislike.

[bartonjs]

We're only a couple of weeks away from the target platform complete date. What's left for the ECB part, and do you need help? I don't want to have ECB and CBC split across a release boundary, so we either need to finish this off relatively soon or push it out to 7.

[vcsjones]

What's left for the ECB part, and do you need help?

I think the "musts" that are remaining are XML docs and a few more test cases to make me feel better about failure scenarios.

I can commit to getting ECB in shape by end of the week (Sunday if not sooner) and it should be relatively straight forward to do the work for CBC/CFB.

[vcsjones]

@bartonjs this should be in a reviewable state now, caveated on the things in the top post are not done where they are not checked off.

[bartonjs]

Added a whole heap'o periods, a few missing <returns>es, and two pieces of technical feedback.

[vcsjones]

@bartonjs sincere thank you for the proofing and taking the time to put all of the suggestions in. Will address the rest of the feedback in the next day or two.

[GrabYourPitchforks]

I came into this late, but it looks great! Feeling kinda guilty that the only comments I can offer are stylistic nits.

[vcsjones]

@bartonjs I think I got all your feedback applied consistently.

@GrabYourPitchforks

Feeling kinda guilty that the only comments I can offer are stylistic nits.

There will be plenty more opportunities to say, "Kevin... what on earth are you doing??"

[bartonjs]

Woohoo! Thanks, @vcsjones.

[vcsjones]

It's done — Frodo

Will get CBC / CFB in ASAP.