Jump to content
Soji

Delphi AES encryption/decryption

Recommended Posts

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.

Share this post


Link to post

@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.

Share this post


Link to post
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.

Share this post


Link to post

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.

  • Like 1

Share this post


Link to post
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.

Share this post


Link to post

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.

Share this post


Link to post

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 by Kas Ob.

Share this post


Link to post

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

Share this post


Link to post

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;

 

  • Like 1

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×