Xor-el / CryptoLib4Pascal

Crypto for Modern Object Pascal
MIT License
213 stars 65 forks source link

AES-CBC cipher works without providing an IV parameter. Is this correct? #44

Open mikelbikkel opened 2 weeks ago

mikelbikkel commented 2 weeks ago

Describe the bug A cipher with MODE = CBC requires an IV. When I create a cipher "AES/CBC/PKCS7PADDING" and I do NOT provide an IV parameter, the cipher works (encrypts an decrypts) without any warning or error. I would expect an error if the IV parameter is not provided.

To Reproduce var FCipher: IBufferedCipher; FParams: ICipherParameters; begin FCipher := TCipherUtilities.getCipher('AES/CBC/PKCS7PADDING'); FParams := TParameterUtilities.CreateKeyParameter('AES', arKey); // arKey is a TBytes with the key. FCipher.Init(True, FParams); // I would expect an exception at this point: MISSING IV. FCipher.FCipher.ProcessBytes(.... etc. // Encryption succeeds without IV. end;

The issue seems to be at this point: procedure TCbcBlockCipher.Init(forEncryption: Boolean; const parameters: ICipherParameters);

if Supports(Lparameters, IParametersWithIV, ivParam) then begin iv := ivParam.GetIV(); if (System.Length(iv) <> FblockSize) then begin raise EArgumentCryptoLibException.CreateRes(@SInvalidIVLength); end; System.Move(iv[0], FIV[0], System.Length(iv) * System.SizeOf(Byte)); end; // MY REMARK: there is no "else"-branch that handles the missing IV parameter. // The cipher works without IV because FIV is a 16-byte zero-filled array. These zeros are used as IV. Reset();

Expected behavior if Supports(Lparameters, IParametersWithIV, ivParam) then begin iv := ivParam.GetIV(); if (System.Length(iv) <> FblockSize) then begin raise EArgumentCryptoLibException.CreateRes(@SInvalidIVLength); end; System.Move(iv[0], FIV[0], System.Length(iv) * System.SizeOf(Byte)); else raise EArgumentCryptoLibException.CreateRes(@SMissingIV); end; Raise an exception if IV is missing Reset();

Screenshots NA

Environment (please complete the following information):

Additional context Add any other context about the problem here.

Xor-el commented 2 weeks ago

Hello @mikelbikkel as you can see here and as you have rightly stated, the IV is zero initialized. while that isn't recommended, it isn't necessarily disallowed. if you wish to pass a custom IV, you can do this

KeyParametersWithIV := TParametersWithIV.Create(TParameterUtilities.CreateKeyParameter('AES', arKey), IVBytes);
mikelbikkel commented 2 weeks ago

It's tricky in the current implementation. Makes it a bit too easy to forget that certain modes require an IV. An explanation in the documentation comments would help. Is tis codebase still actively maintained? I don't see activity in github for 5 years.... I m sure you put a lot of work in it, so it would be a pity if opportunities for improvements are not used.

Michel

Xor-el commented 2 weeks ago

It's tricky in the current implementation. Makes it a bit too easy to forget that certain modes require an IV. An explanation in the documentation comments would help. Is tis codebase still actively maintained? I don't see activity in github for 5 years.... I m sure you put a lot of work in it, so it would be a pity if opportunities for improvements are not used.

Michel

@mikelbikkel Yes the code base is still maintained just that there hasn't been any new features added due to time constraints. concerning the documentation, would you do the honours of creating a PR that documents this?

mikelbikkel commented 2 weeks ago

Happy to help. It will be my first PR ever, so i need some time to prepare. Let's get started