Missing padding for ECB mode

[M0nsieurT]

Describe the bug
I used FMX Cipher Demo to use both AES and DES encryptions to make them work with this site : https://www.devglan.com/online-tools/aes-encryption-decryption

The issue comes when you select ECB cipher mode but message length is not a multiple of 8

I already spent a lot of time with other Delphi encryption libraries and found this post years ago explaining ECB needs padding as well
https://stackoverflow.com/questions/55137264/lockbox-3-encrypting-not-matching-online-tool-suspect-padding-or-key-problem

PKCS5PadStringToBytes function described in the post solved my problem, please find below the part of code updated
`
function PKCS5PadStringToBytes(RawData: string; const PadSize: integer): System.SysUtils.TBytes;
var
DataLen: integer;
PKCS5PaddingCount: ShortInt;
begin
result := TEncoding.UTF8.GetBytes(RawData);
DataLen := Length(RawData);

PKCS5PaddingCount := PadSize - DataLen mod PadSize;
if PKCS5PaddingCount = 0 then
PKCS5PaddingCount := PadSize;
Inc(DataLen, PKCS5PaddingCount);

SetLength(result, DataLen);
FillChar(result[DataLen - PKCS5PaddingCount], PKCS5PaddingCount, PKCS5PaddingCount);
end;

procedure TFormMain.ButtonEncryptClick(Sender: TObject);
var
Cipher: TDECCipherModes;
InputFormatting: TDECFormatClass;
OutputFormatting: TDECFormatClass;
InputBuffer: TBytes;
OutputBuffer: TBytes;
begin
if not GetFormatSettings(InputFormatting, OutputFormatting) then
exit;

try
Cipher := GetInitializedCipherInstance;

try
  InputBuffer := System.SysUtils.BytesOf(EditPlainText.Text);

  if GetSelectedCipherMode = cmECBx then
  begin
     InputBuffer := PKCS5PadStringToBytes(EditPlainText.Text, 8);
  end;

...`

I wonder why you explicitly ignore padding in ECB mode ?

You will find below a screenshot with my fix, matching demo result with the website

[MHumm]

Thanks for not only posing this request but for supplying code and a test case as well! I have to think about where to integrate this exactly. In DECCipherBase there is already a type definition TBlockFillMode and TDECCipher even has a FillMode property, but it is not being used yet. I think that is a base for this which needs to be extended and used. But to be honest: it might be a bit harder to implement than in your demo, as I have to care for "the generic case" and I have to find the time to do all this ;-)
But again: thanks for this!

[M0nsieurT]

You are very welcome, I was so relieved to see an open source package supporting both AES and DES.

Thanks for the hint, I still have to check if padSize parameter is related to block size to have as you said a more generic case

[MHumm]

Haven't had the time to think it over, but I guess that's not the only "padding standard". Do you know other ones? My question would be if we could put them into a new class between DECipherBase and current child class? And of course we need to create unit tests for this. And if we get it properly designed and working the FMX demo should get a selection mechanism for the padding mode.

[M0nsieurT]

You are right, there are a bunch listed here : https://en.wikipedia.org/wiki/Padding_%28cryptography%29#Block_cipher_mode_of_operation

I only worked with PKCS#5 and PKCS#7 on my projects (they are pratically the same but the difference is explained on my first stackoverflow link), I am afraid I cannot help you on the others.

[MHumm]

These would be a start at least and we could use them to start some infrastructure for such algorithms! If you like I can create you a branch and give you commit rights. You could start there and then we merge when you feel it is finished. What do you think about this idea? Julien Tissier ***@***.***> schrieb am Di., 16. Mai 2023, 21:29:

…

You are right, there are a bunch listed here : https://en.wikipedia.org/wiki/Padding_%28cryptography%29#Block_cipher_mode_of_operation I only worked with PKCS#5 and PKCS#7 on my projects (they are pratically the same but the difference is explained on my first stackoverflow link), I am afraid I cannot help you on the others. — Reply to this email directly, view it on GitHub <#56 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHCTXHLWBEAYTISKYWL6UKLXGPISPANCNFSM6AAAAAAYD4UHPU> . You are receiving this because you were assigned.Message ID: ***@***.***>

[MHumm]

Sorry for the barely readable first sentence in the last post (I fixed it now). I typed it from my Android tablet and there auto correction always intervenes, as English is not my native language.

Now did you make up your mind about getting a branch and commit permission? I'd really value your contribution! With your contribution these longer term plans to have selectable padding shemes could get reality sooner!

[M0nsieurT]

I can give a try but I am pretty busy, I was in North America last week and found some time to test your package, now I am back in France but not sure to find a lot of it.

[MHumm]

It would be nice if you could give it a try. If it takes some time it's ok too, as long as it makes progress at all ;-)
Question: shall I set up a branch for this and give you commit rights?

[danielmarschall]

I am not sure what "missing padding" means in this ticket's problem description.

What is exactly the issue? Is it really missing (i.e. the size is too short) or is it filled with static zero-bytes?

[danielmarschall]

Oh, I think OP meant this spot in TDECCipherModes.EncodeECBx :

      if Size mod Context.BlockSize = 0 then
      begin
        ......
      end
      else
      begin
        FState := csPadded;
        ReportInvalidMessageLength(Self);
      end;

If that's the problem, then I think it could be easily padded with random data to a multiple of 8; or is there a risk that something doesn't work afterwards?