Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

One Shot ECB#52510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bartonjs merged 40 commits intodotnet:mainfromvcsjones:2406-one-shot-ecb
Jun 28, 2021
Merged

One Shot ECB#52510

bartonjs merged 40 commits intodotnet:mainfromvcsjones:2406-one-shot-ecb
Jun 28, 2021

Conversation

@vcsjones
Copy link
Member

@vcsjonesvcsjones commentedMay 8, 2021•
edited
Loading

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

  • XML documentation
  • The base implementation inSymmetricAlgorithm throws aNotImplementedException. 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 theKey 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, andRijndaelManaged 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

@ghost
Copy link

Note regarding thenew-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
Copy link

Tagging subscribers to this area:@bartonjs,@vcsjones,@krwq,@GrabYourPitchforks
See info inarea-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 inSymmetricAlgorithm throws aNotImplementedException. 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 theKey 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, andRijndaelManaged will not get a custom implementation since they are being obsoleted.
Author:vcsjones
Assignees:-
Labels:

area-System.Security,new-api-needs-documentation

Milestone:-

The CNG types were calculating their padding size based on an instancemethod that used the Mode property instead of the actual mode beingused.Most of the time, this will just end up being whatever the block size ofthe algorithm is going to be. However, in the case of CFB8, that is nottrue. Even if the algorithm instance is in CFB8 mode, the EncryptEcb andDecryptEcb one-shots should continue to function as ECB mode.
The overlapping buffer check was renting a buffer every time unlessthe input and output overlap exactly. We only need to rent when theydo overlap partially, or no overlapping at all.
@vcsjones
Copy link
MemberAuthor

@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.


[Theory]
[MemberData(nameof(EcbTestCases))]
publicstaticvoidEcbRoundtrip(byte[]plaintext,byte[]ciphertext,PaddingModepadding)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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:

publicsealedclassSignVerify_Span:SignVerify
{
protectedoverridebyte[]SignData(RSArsa,byte[]data,HashAlgorithmNamehashAlgorithm,RSASignaturePaddingpadding)=>
TryWithOutputArray(dest=>rsa.TrySignData(data,dest,hashAlgorithm,padding,outintbytesWritten)?(true,bytesWritten):(false,0));
protectedoverridebyte[]SignHash(RSArsa,byte[]hash,HashAlgorithmNamehashAlgorithm,RSASignaturePaddingpadding)=>
TryWithOutputArray(dest=>rsa.TrySignHash(hash,dest,hashAlgorithm,padding,outintbytesWritten)?(true,bytesWritten):(false,0));
protectedoverrideboolVerifyData(RSArsa,byte[]data,byte[]signature,HashAlgorithmNamehashAlgorithm,RSASignaturePaddingpadding)=>
rsa.VerifyData((ReadOnlySpan<byte>)data,(ReadOnlySpan<byte>)signature,hashAlgorithm,padding);
protectedoverrideboolVerifyHash(RSArsa,byte[]hash,byte[]signature,HashAlgorithmNamehashAlgorithm,RSASignaturePaddingpadding)=>
rsa.VerifyHash((ReadOnlySpan<byte>)hash,(ReadOnlySpan<byte>)signature,hashAlgorithm,padding);

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
MemberAuthor

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.

bartonjs reacted with thumbs up emoji

@vcsjonesvcsjones marked this pull request as ready for reviewJune 24, 2021 21:02
@vcsjones
Copy link
MemberAuthor

@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.

Copy link
Member

@bartonjsbartonjs left a comment

Choose a reason for hiding this comment

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

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

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@vcsjones
Copy link
MemberAuthor

@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
Copy link
Member

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

@vcsjones
Copy link
MemberAuthor

@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??"

@bartonjsbartonjs merged commit6b5dbf6 intodotnet:mainJun 28, 2021
@bartonjs
Copy link
Member

Woohoo! Thanks,@vcsjones.

@vcsjonesvcsjones deleted the 2406-one-shot-ecb branchJune 28, 2021 17:00
@vcsjones
Copy link
MemberAuthor

It's done — Frodo

Will get CBC / CFB in ASAP.

Sergio0694 reacted with hooray emojiSergio0694 reacted with heart emoji

@ghostghost locked asresolvedand limited conversation to collaboratorsJul 28, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@GrabYourPitchforksGrabYourPitchforksGrabYourPitchforks left review comments

@stephentoubstephentoubstephentoub left review comments

@bartonjsbartonjsbartonjs approved these changes

+1 more reviewer

@filipnavarafilipnavarafilipnavara left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vcsjones@bartonjs@GrabYourPitchforks@filipnavara@stephentoub
[8]ページ先頭
©2009-2025 Movatter.jp