Delphi AES encryption/decryption

[Guest]

13 minutes ago, TurboMagic said:

What's your opinion?

Either this to replace the above as that micro optimization is unneeded because we have dependent PBKDF2 implementation and the difference will be very small.

function Calc_HMAC(const Key, Text: TBytes; HashClass: TDECHashClass): TBytes;
var
  Hash: TDECHash;
  InnerKeyPad, OuterKeyPad: TBytes;
  I, KeyLength, BlockSize, DigestLength: Integer;
begin
  Hash := HashClass.Create;
  try
    BlockSize := Hash.BlockSize;
    DigestLength := Hash.DigestSize;
    KeyLength := Length(Key);
    I := 0;
    if KeyLength > BlockSize then
    begin
      Result := Hash.CalcBytes(Key);
      KeyLength := DigestLength;
    end
    else
      Result := Key;

    SetLength(InnerKeyPad,BlockSize);
    SetLength(OuterKeyPad,BlockSize);
    while I < KeyLength do
    begin
      InnerKeyPad[I] := Result[I] xor $36;
      OuterKeyPad[I] := Result[I] xor $5C;
      Inc(I);
    end;
    while I < BlockSize do
    begin
      InnerKeyPad[I] := $36;
      OuterKeyPad[I] := $5C;
      Inc(I);
    end;

    Hash.Init;
    Hash.Calc(InnerKeyPad[0], BlockSize);
    if Length(Text) > 0 then
      Hash.Calc(Text[0], Length(Text));
    Hash.Done;
    Result := Hash.DigestAsBytes;

    Hash.Init;
    Hash.Calc(OuterKeyPad[0], BlockSize);
    Hash.Calc(Result[0], DigestLength);
    Hash.Done;
    Result := Hash.DigestAsBytes;

  finally
    Hash.Free;
  end;
end;

Or just cast the constant with NativeUInt

  PNativeUInt(@InnerKeyPad[I])^ := PNativeUInt(@Result[I])^ xor NativeUInt(CONST_UINT_OF_0x36);
  PNativeUInt(@OuterKeyPad[I])^ := PNativeUInt(@Result[I])^ xor NativeUInt(CONST_UINT_OF_0x5C);

 

17 minutes ago, TurboMagic said:

Turning off range/index/ E/A compiler checking would fix this, but is this a good idea?

I always would leave that as last option.

[Guest]

@TurboMagic I want to point few things i don't like in the library and i am sorry asking you to look into them and not doing myself.

1) the over use of ProtectBuffer , it been called form both TDECHash.Destroy and TDECHash.Done , this is waste of time, i am not a fan of creating false sense of security by using such not useful defense, see if an attacker is watching and reading the memory then he easily could slow the CPU for that process 200 times (if needed) and grab any changes along the way, yet even better if he is the controlling the PC and can read the memory of any process then he already owned it, right ?
So my suggestion either remove that protect buffer from both and introduce a new procedure for secure erase that will call ProtectBuffer (overwrite the buffer, digest, counter..)

 

2) TDECHash.Done is calling ReallocMemory(FBuffer,0) , this also unneeded leave it there, no point of removing the buffer on Done call, if the user is using TDECHash wrong then this is will not help or secure anything.

3) We have this for Init

procedure TDECHash.Init;
begin
  FBufferIndex := 0;
  FBufferSize := BlockSize;
  // ReallocMemory instead of ReallocMem due to C++ compatibility as per 10.1 help
  FBuffer := ReallocMemory(FBuffer, FBufferSize);

  FillChar(FBuffer^, FBufferSize, 0);
  FillChar(FCount, SizeOf(FCount), 0);
  DoInit;
end;

FBuffer should be allocated within the Algorithm itself (the child) or you can make it as : allocate if not allocated and make sure to initialize it to nil in the constructor.

[TurboMagic 92]

16 minutes ago, Kas Ob. said:

@TurboMagic I want to point few things i don't like in the library and i am sorry asking you to look into them and not doing myself.

1) the over use of ProtectBuffer , it been called form both TDECHash.Destroy and TDECHash.Done , this is waste of time, i am not a fan of creating false sense of security by using such not useful defense, see if an attacker is watching and reading the memory then he easily could slow the CPU for that process 200 times (if needed) and grab any changes along the way, yet even better if he is the controlling the PC and can read the memory of any process then he already owned it, right ?
So my suggestion either remove that protect buffer from both and introduce a new procedure for secure erase that will call ProtectBuffer (overwrite the buffer, digest, counter..)

 

2) TDECHash.Done is calling ReallocMemory(FBuffer,0) , this also unneeded leave it there, no point of removing the buffer on Done call, if the user is using TDECHash wrong then this is will not help or secure anything.

3) We have this for Init

FBuffer should be allocated within the Algorithm itself (the child) or you can make it as : allocate if not allocated and make sure to initialize it to nil in the constructor.

While coding I didn't see you already answered. As I'm finishing coding for now I made a note about your suggestion of a modified HMAC implementation which doesn't trigger overflows. And yes, turning of the checks should always be a last ressort!
That's why I was asking.

 

About your protect buffer points:
I did sort of inherit this DEC library and thought it's too good to let it die, but I'm no really expert in these things.
I learned quite a few things along already though.

 

I will look at removing those, you might be right. I just didn't want to remove something not causing me real trouble from an inherited library
when I don't completely understand the implications/effects this creates.

That's also the thing with ReallocMemory. I never really understood its purpose and when I asked a few months ago in German DP because of some
C++ Builder compatibility issues I didn't get really useful pointers why this might be used here. I only got some vague C++ Builder compatibility
information as answers. So I left as is. But we can remove this as well.

 

Just clearing the memory somehow at some point might reduce attack surface a bit. Yes, if the attacker can slow down that process well enough to
be able to have enough time to read out memory while it runs that protection will not help. It  just helps not having possibly sensible information
lying around for longer than necessary.

[TurboMagic 92]

Another small info: I implemented your PBKDF2 implementation along with the unit tests meanwhile.

 

I also toy with the idea to create a new descendend class from TDECHashBase where I move all KDF, MGF, HMAC and
PBKDF2 implementations into and all other classes inherit from that one. That might introduce a new unit to keep the
individual parts a bit shorter.

[Guest]

3 minutes ago, TurboMagic said:

That's also the thing with ReallocMemory. I never really understood its purpose and when I asked a few months ago in German DP because of some
C++ Builder compatibility issues I didn't get really useful pointers why this might be used here. I only got some vague C++ Builder compatibility
information as answers. So I left as is. But we can remove this as well.

This question need someone more qualified than me with C++ Builder to answer, but i think replacing it with TBytes will be better and more clean solution.

 

As for the protecting, as you pointed i left it as suggestion either remove or enhance a little, see after calling Done there is only two option to call, Free(->Destroy) or Init, so user can call Free and it might call the secure erase, or Init which will overwrite it, or a new procedure as secure clear.

[TurboMagic 92]

That sounds sensible: move it to free as the other common option init should overwrite it. But I need to check this first. If init just frees and allocates memory nothing might be overwritten if the memory is not allocated at the same place.

But you're right: if done, it should be done intelligently.

[Guest]

I meant something like this 

constructor TDECHash.Create;
begin
  inherited;
  FBufferSize := 0;
  FBuffer := nil;
end;

destructor TDECHash.Destroy;
begin
  SecureErase;  // This could be enabled/disabled by compiler directive switch and can enbaled by default
  FreeMem(FBuffer, FBufferSize);

  inherited Destroy;
end;

procedure TDECHash.Init;
begin
  FBufferIndex := 0;
  // ReallocMemory instead of ReallocMem due to C++ compatibility as per 10.1 help
  if (FBuffer = nil) or (FBufferSize <> BlockSize) then
    begin
      FBufferSize := BlockSize;
      FBuffer := ReallocMemory(FBuffer, FBufferSize);
    end;

  FillChar(FBuffer^, FBufferSize, 0);
  FillChar(FCount, SizeOf(FCount), 0);
  DoInit;
end;

procedure TDECHash.Done;
begin
  DoDone;
end;

procedure TDECHash.SecureErase;
begin
  ProtectBuffer(Digest^, DigestSize);
  if FBuffer=nil then
    ProtectBuffer(FBuffer^, FBufferSize);
end;

 

Edited January 23, 2021 by Guest

[TurboMagic 92]

I implemented this now and renamed Protect in CipherBase class to SecureErase for naming consistency.

[Guest]

I made a mistake here with this

procedure TDECHash.SecureErase;
begin
  ProtectBuffer(Digest^, DigestSize);
  if FBuffer <> nil then
    ProtectBuffer(FBuffer^, FBufferSize);
end;

And i think this version is better to build on , as i am afraid of this line "Result := Result + T", and we are accessing T elements, this could be a problem

function PBKDF2(const Password, Salt: TBytes; Iterations: Integer; KeyLength: Integer; HashClass: TDECHashClass): TBytes;
var
  Hash: TDECHash;
  I, J, C: Integer;
  BlockCount, HashLengthRounded, SaltLength: Integer;
  PassLength, DigestLength, BlockSize: Integer;
  InnerKeyPad, OuterKeyPad: TBytes;
  SaltEx, T, U, TrimmerPassword: TBytes;
begin
  Hash := HashClass.Create;
  try
    // Setup parameters
    DigestLength := Hash.DigestSize;
    HashLengthRounded := DigestLength - SizeOf(NativeUInt) + 1;
    BlockSize := Hash.BlockSize;
    BlockCount := Trunc((KeyLength + DigestLength - 1) / DigestLength);
    PassLength := Length(Password);
    SaltLength := Length(Salt);
    SaltEx := Salt;
    SetLength(SaltEx, SaltLength + 4);  // reserve 4 bytes for INT_32_BE(i)
    SetLength(T, DigestLength);

    // Prepare Password for HMAC calculation
    // PrepareKeyForHMAC;
    I := 0;
    if PassLength > BlockSize then
    begin
      TrimmerPassword := Hash.CalcBytes(Password);
      PassLength := DigestLength;
    end
    else
      TrimmerPassword := Password;

    SetLength(InnerKeyPad, BlockSize);
    SetLength(OuterKeyPad, BlockSize);
    while I < PassLength do
    begin
      InnerKeyPad[I] := TrimmerPassword[I] xor $36;
      OuterKeyPad[I] := TrimmerPassword[I] xor $5C;
      Inc(I);
    end;
    while I < BlockSize do
    begin
      InnerKeyPad[I] := $36;
      OuterKeyPad[I] := $5C;
      Inc(I);
    end;

    // Calculate DK
    SetLength(Result, BlockCount * DigestLength);
    for I := 1 to BlockCount do
    begin
      SaltEx[SaltLength + 0] := Byte(I shr 24);   // INT_32_BE(i)
      SaltEx[SaltLength + 1] := Byte(I shr 16);
      SaltEx[SaltLength + 2] := Byte(I shr 8);
      SaltEx[SaltLength + 3] := Byte(I shr 0);
      FillChar(T[0], DigestLength, 0);            // reset Ti / F
      U := SaltEx;                                // initialize U to U1 = Salt + INT_32_BE(i)
      // Calculate F(Password, Salt, c, i) = U1 ^ U2 ^ ... ^ Uc
      for C := 1 to Iterations do
      begin
        Hash.Init;
        Hash.Calc(InnerKeyPad[0], BlockSize);
        Hash.Calc(U[0], Length(U));
        Hash.Done;
        U := Hash.DigestAsBytes;

        Hash.Init;
        Hash.Calc(OuterKeyPad[0], BlockSize);
        Hash.Calc(U[0], DigestLength);
        Hash.Done;
        U := Hash.DigestAsBytes;                  // Ui
        // F = U1 ^ U2 ^ ... ^ Uc
        J := 0;
        while J < HashLengthRounded do
        begin
          PNativeUInt(@T[J])^ := PNativeUInt(@T[J])^ xor PNativeUInt(@U[J])^;
          Inc(J, SizeOf(NativeUInt));
        end;
        while J < DigestLength do
        begin
          T[J] := T[J] xor U[J];
          Inc(J);
        end;
      end;

      Move(T[0], Result[(I - 1) * DigestLength], DigestLength); // DK += F    , DK = DK || Ti
    end;
  finally
    Hash.Free;
  end;

  // Trim to the needed key length
  SetLength(Result, KeyLength);
end;

 

[Jasja 3]

On 10/21/2020 at 8:50 PM, stijnsanders said:

If you're interested in another alternative, I've started from the root document to make a pure-Delphi version under a permissive license: https://github.com/stijnsanders/tools/blob/master/crypto/aes.pas

I also did HMAC and PKDF2 here

Hello Stein,

 

I've downloaded your github tools/encrypt archive because I like to use your EAS encrypt/decrypt routines.

Do you have an example how the EASBlock and EASxxxkey needs to be initialised ?? or how I can encrypt a TBytes or RawByteString ??

Thanks for your reply.

 

[TurboMagic 92]

Sorry for not answering earlier. I'm not often on English DelphiPraxis forum and it seems I don't get E-Mail notifications. Need to check settings for those.

Aboue EAS: I guess you mean AES?

The key is the key used for encryption descyption. means: both parties need to use the same key.

If you just use the AES class the variant of AES algorithm picked/created depends on the length of the key you give.

At least in the version in development branch (not sure if that's already in 6.4.1) there are AES128, AES192 and AES256 classes now which

do not really check key length to find out which one to create.

 

Init vector: if you use any block mode except ECB (and ECB is not recommended as this is the least secure one) the preceeding
block is somehow combined with the data of the currently processed block. That means: if you encrypt the very same data twice, it

usually will be encrypted differently because blocks depend on each other. This enhances security. When using ECB mode
this is not the case and thus is should not be used.

 

This init vector is just the data which is treated as "output of the former block processed" when calculating the very first block.
So it must have a length of block size of the algorithm and you need to supply the same data for encryption and decryption.

The recommendation is to use a different value each time you encrypt something.

 

I hope this helps, if not let me know.