Require CFB8 for persisted CNG keys in CFB mode by vcsjones · Pull Request #55615 · dotnet/runtime

vcsjones · 2021-07-14T01:32:16Z

This changes AesCng and TripleDESCng to require the feedback size to be set to '8' when in CFB mode and using a persisted CNG key.

Prior to this change, BasicSymmetricCipherNCrypt ignored the feedback size which resulted in CFB8 always being used, even if the FeedbackSize was set to another value. However, when padding was applied, the padding size would be padded to the feedback size. In the case of AesCng, this would mean it would be encrypted with CFB8 and padded as if it were CFB128.

It was also determined that persisted CNG keys also do not support setting the feedback size, so that gives us the only option to throw.

This changes the implementation so that the feedback size is required to be set to 8 for persisted keys. No change is made for ephemeral keys.

Developers that are impacted by this change can work around this by setting the FeedbackSize to 8, since that is how it is actually encrypted. Even though it was padded with additional padding, it will be handled by the decryption since this was actually the behavior of .NET Framework (it was always padded to the block size). So the decryption handles the extra padding already, and encryption will start producing smaller ciphertexts since it will contain at most one byte of padding.

As noted in the issue, this breaking change will have a small impact.

Closes#55477.

This changes AesCng and TripleDESCng to require the feedback size to be set to '8' when in CFB mode and using a persisted CNG key. Prior to this change, BasicSymmetricCipherNCrypt ignored the feedback size which resulted in CFB8 always being used, even if the FeedbackSize was set to another value. However, when padding was applied, the padding size would be padded to the feedback size. In the case of AesCng, this would mean it would be encrypted with CFB8 and padded as if it were CFB128. This changes the implementation so that the feedback size is required to be set to 8 for persisted keys. No change is made for ephemeral keys.

ghost · 2021-07-14T01:32:22Z

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

Issue Details

This changes AesCng and TripleDESCng to require the feedback size to be set to '8' when in CFB mode and using a persisted CNG key.

Prior to this change, BasicSymmetricCipherNCrypt ignored the feedback size which resulted in CFB8 always being used, even if the FeedbackSize was set to another value. However, when padding was applied, the padding size would be padded to the feedback size. In the case of AesCng, this would mean it would be encrypted with CFB8 and padded as if it were CFB128.

It was also determined that persisted CNG keys also do not support setting the feedback size, so that gives us the only option to throw.

This changes the implementation so that the feedback size is required to be set to 8 for persisted keys. No change is made for ephemeral keys.

Developers that are impacted by this change can work around this by setting the FeedbackSize to 8, since that is how it is actually encrypted. Even though it was padded with additional padding, it will be handled by the decryption since this was actually the behavior of .NET Framework (it was always padded to the block size). So the decryption handles the extra padding already, and encryption will start producing smaller ciphertexts since it will contain at most one byte of padding.

As noted in the issue, this breaking change will have a small impact.

Closes #55477.

Author:	vcsjones
Assignees:	-
Labels:	`area-System.Security`
Milestone:	-

vcsjones · 2021-07-14T01:33:14Z

 //
 // The delegate must instantiate a new CngKey, based on a new underlying NCryptKeyHandle, each time is called.
 //
- public BasicSymmetricCipherNCrypt(Func<CngKey> cngKeyFactory, CipherMode cipherMode, int blockSizeInBytes, byte[]? iv, bool encrypting, int feedbackSizeInBytes, int paddingSize)


We can't do anything with the feedback size here, so stop accepting it so new callers don't assume it does anything.

vcsjones · 2021-07-14T01:34:05Z

 {
 using (SymmetricAlgorithm alg = persistedFunc(keyName))
 {
+ alg.Mode = CipherMode.CFB;


The positive "it works with CFB8" tests were added in the CFB one-shot PR. #55480

ghost · 2021-07-14T01:36:54Z

Tagging @dotnet/compat for awareness of the breaking change.

vcsjones · 2021-07-14T01:38:47Z

This should get an outerloop run once the changes look good and innerloop looks reasonable.

vcsjones · 2021-07-14T02:13:02Z

 Link="CommonTest\System\Security\Cryptography\AlgorithmImplementations\RSA\SignVerify.netcoreapp.cs" />
 <Compile Condition="'$(TargetsWindows)' == 'true'" Include="AesCngTests.cs" />
 <Compile Condition="'$(TargetsWindows)' == 'true'" Include="TripleDESCngTests.cs" />
+ <Compile Include="$(CommonTestPath)System\Security\Cryptography\AlgorithmImplementations\TripleDES\TripleDESContractTests.cs"


Turns out we were never running these contract tests under CNG.

vcsjones · 2021-07-14T02:14:46Z

So the decryption handles the extra padding already, and encryption will start producing smaller ciphertexts since it will contain at most one byte of padding.

I would have bet some sum of money that this test already existed... but I couldn't find it, so I added one. If the CFB8-but-block-length-padded tests are duplicate I would be happy to remove them.

bartonjs · 2021-07-14T02:58:50Z

Thanks for continuing to be quite speedy, @vcsjones!

bartonjs · 2021-07-14T02:59:25Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-07-14T02:59:37Z

Azure Pipelines successfully started running 1 pipeline(s).

vcsjones · 2021-07-14T14:33:16Z

All failures appear unrelated.

bartonjs · 2021-07-14T19:21:35Z

All failures appear unrelated.

I concur.

bartonjs · 2021-09-30T18:47:38Z

Documentation issue opened: dotnet/docs#26326

ghost added the area-System.Security label Jul 14, 2021

Add a comment back
f260327

vcsjones commented Jul 14, 2021
View reviewed changes

vcsjones added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jul 14, 2021

ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 14, 2021

vcsjones marked this pull request as ready for review July 14, 2021 01:38

vcsjones added this to the 6.0.0 milestone Jul 14, 2021

Add tests to validate CFB8 can decrypt block-padded data
88ac833

vcsjones force-pushed the persisted-cfb-cng-feedback-size branch from 9474b27 to 88ac833Compare July 14, 2021 02:12

vcsjones commented Jul 14, 2021
View reviewed changes

bartonjs reviewed Jul 14, 2021
View reviewed changes

Comment threadsrc/libraries/System.Security.Cryptography.Cng/src/System/Security/Cryptography/AesCng.cs

bartonjs approved these changes Jul 14, 2021
View reviewed changes

bartonjs merged commit f6e9532 into dotnet:mainJul 14, 2021

vcsjones deleted the persisted-cfb-cng-feedback-size branch July 14, 2021 19:22

ghost locked as resolved and limited conversation to collaborators Aug 13, 2021

ericstj assigned bartonjs Sep 30, 2021

bartonjs removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 30, 2021

bartonjs removed their assignment Sep 30, 2021

Uh oh!

Conversation

vcsjones commented Jul 14, 2021

Uh oh!

ghost commented Jul 14, 2021

Uh oh!

vcsjonesJul 14, 2021

Choose a reason for hiding this comment

Uh oh!

vcsjonesJul 14, 2021

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 14, 2021

Uh oh!

vcsjones commented Jul 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcsjonesJul 14, 2021

Choose a reason for hiding this comment

Uh oh!

vcsjones commented Jul 14, 2021

Uh oh!

Uh oh!

bartonjs commented Jul 14, 2021

Uh oh!

bartonjs commented Jul 14, 2021

Uh oh!

azure-pipelinesBot commented Jul 14, 2021

Uh oh!

vcsjones commented Jul 14, 2021

Uh oh!

bartonjs commented Jul 14, 2021

Uh oh!

bartonjs commented Sep 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vcsjones commented Jul 14, 2021•
edited
Loading