Skip to content

Add padding when encrypt and remove padding when decrypt - #1545

Merged
Rob-Hague merged 16 commits into
sshnet:developfrom
scott-xu:padding
Dec 22, 2024
Merged

Add padding when encrypt and remove padding when decrypt#1545
Rob-Hague merged 16 commits into
sshnet:developfrom
scott-xu:padding

Conversation

@scott-xu

@scott-xuscott-xu commented Nov 30, 2024

Copy link
Copy Markdown
Collaborator
  • Add PKCS7 padding when encrypt no matter the input is divisible or not if pkcs7padding is specified.
  • Add manual padding when encrypt if input is not divisible and pkcs7padding is not specified and cipher mode is CFB or OFB.
  • Remove PKCS7 padding when decrypt if pkcs7padding is specified.
  • Remove manual padding when decrypt if pkcs7padding is not specified and there was manual padding added.

Originally I was to use BCL TripleDES/DES to replace managed implementation. Then I found the padding is wrong and was going to make a fix. During the fix, I noticed that I'm on the same path with @Rob-Hague and @zybexXL 😄 See #1238 and https://github.com/zybexXL/SSH.NET/compare/feature_AES_CSP...zybexXL:SSH.NET:fix_padding?diff=split

…` passed to `EncryptBlock` if padding is specified, no matter input is divisible or not.
…putBuffer` passed to `DecryptBlock` and unpadding for the final output if padding is not specified and mode is CFB or OFB.
…putBuffer` passed to `EncryptBlock` and unpadding for the final output if padding is not specified and mode is CFB or OFB.
scott-xuand others added 2 commits November 30, 2024 21:05
Comment threadsrc/Renci.SshNet/Security/Cryptography/BlockCipher.cs
/// </summary>
/// <param name="input">The input.</param>
/// <returns>The padd count.</returns>
public abstract int PadCount(byte[] input);

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

We can consider replacing CipherPadding and PKCS7Padding with BouncyCastle's Pkcs7Padding

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes, I think we should take this opportunity to delete the useless CipherPadding and replace it with BC IBlockCipherPadding, and delete PKCS7Padding and replace it with BC Pkcs7Padding, if possible

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Let me have a try

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Just had a quick look. It would be easier to remove it in my next PR #1546.

@scott-xu

Copy link
Copy Markdown
CollaboratorAuthor

There would be pros and cons about different ways to fix the issue, but anyway, I think we all agree that it is an issue and should be fixed. @Rob-Hague@zybexXL

@Rob-Hague

Copy link
Copy Markdown
Collaborator

I have started reviewing this, but I need more time to refamiliarise myself in this area. I have been a bit busy lately

@scott-xu

Copy link
Copy Markdown
CollaboratorAuthor

Thanks! Reviewing is usually more difficult than DIY. No hurry, take your time.

Comment threadsrc/Renci.SshNet/Security/Cryptography/BlockCipher.cs

@zybexXLzybexXL left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me 👍

@Rob-HagueRob-Hague left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks good

/// </summary>
/// <param name="input">The input.</param>
/// <returns>The padd count.</returns>
public abstract int PadCount(byte[] input);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

yes, I think we should take this opportunity to delete the useless CipherPadding and replace it with BC IBlockCipherPadding, and delete PKCS7Padding and replace it with BC Pkcs7Padding, if possible

@Rob-HagueRob-Hague mentioned this pull request Dec 22, 2024
…AesCipherTest.Gen.cs.txt
Co-authored-by: Rob Hague <rob.hague00@gmail.com>
This was referenced Aug 12, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@scott-xu@Rob-Hague@zybexXL