The Wayback Machine - https://web.archive.org/web/20220208033329/https://github.com/dotnet/runtime/pull/64949
Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test and document decision around CFB8 padding #64949

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

@vcsjones
Copy link
Member

@vcsjones vcsjones commented Feb 7, 2022

I initially thought I found a small bug in how CFB8 padding was validated with the one shots. Fortunately, I remembered why the code was the way that it is. Unfortunately, this decision was neither documented in tests nor with a comment.

This PR aims to document why it is the way that it is, both with a comment and a test.


The core issue was that a place we expect to be passed "padding size" was being passed "block size". This is not correct for CFB since padding and block size are not the same for CFB8. However, this is a compatibility decision to be compatible with the .NET Framework CFB8 cipher texts. The .NET Framework always padded CFB to the block size, and we want the one-shot to be able to continue to do that.

That was not immediately obvious why decryption was using block size and not padding size.

Encryption is fine - it always pads to the feedback size.

For compatibility purposes with .NET Framework ciphertexts, we permit CFB8 to contain
excess, up to BlockSizeInBytes, padding.
@msftbot
Copy link
Contributor

@msftbot msftbot bot commented Feb 7, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

I initially thought I found a small bug in how CFB8 padding was validated with the one shots. Fortunately, I remembered why the code was the way that it is. Unfortunately, this decision was neither documented in tests nor with a comment.

This PR aims to document why it is the way that it is, both with a comment and a test.


The core issue was that a place we expect to be passed "padding size" was being passed "block size". This is not correct for CFB since padding and block size are not the same for CFB8. However, this is a compatibility decision to be compatible with the .NET Framework CFB8 cipher texts. The .NET Framework always padded CFB to the block size, and we want the one-shot to be able to continue to do that.

That was not immediately obvious why decryption was using block size and not padding size.

Encryption is fine - it always pads to the feedback size.

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -
alg.Key = Key;

// This is a plaintext that is manually padded. Though it is going to be
// encrypted with CFB8, it's padded to BlockSizeInBytes-1.
Copy link
Member

@bartonjs bartonjs Feb 7, 2022

Choose a reason for hiding this comment

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

By writing BlockSizeInBytes-1 and having 1 semantic byte, we're really manually padding to the block size.... which I assume is what you meant.

At least, to me, "padded to BlockSizeInBytes-1" would be "we turned a one byte message into a 15 byte message" (for AES. 7 for DES)

Copy link
Member Author

@vcsjones vcsjones Feb 7, 2022

Choose a reason for hiding this comment

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

Ah okay, yeah I see how that was not clear. It was 1 byte of data with N-1 bytes of padding, to make a block-size worth of data. Hopefully that is more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants