THash_SHA1.PBKDF2 create wrong Results

[arkadiusz-wolanski]

Calling THash_SHA1.PBKDF2 produces incorrect or different results for different key lengths:

THash_SHA1.PBKDF2('PassWord', 'Salt', 1000, 16) returns (5, 231, 179, 63, 147, 234, 29, 53, 147, 155, 50, 173, 89, 148, 239, 99).

THash_SHA1.PBKDF2('PassWord', 'Salt', 1000, 32) returns (25, 19, 104, 233, 243, 31, 227, 133, 0, 222, 240, 86, 147, 7, 76, 55, 22, 159, 117, 140, 25, 19, 104, 233, 243, 31, 227, 133, 0, 222, 240, 86)

THash_SHA1.PBKDF2('PassWord', 'Salt', 1000, 48) returns (25, 19, 104, 233, 243, 31, 227, 133, 0, 222, 240, 86, 147, 7, 76, 55, 22, 159, 117, 140, 25, 19, 104, 233, 243, 31, 227, 133, 0, 222, 240, 86, 147, 7, 76, 55, 22, 159, 117, 140, 159, 45, 208, 2, 26, 124, 20, 230)

My understanding is that the first 16 bytes of the result should be identical in all cases. Unfortunately this is not the case.

[MHumm]

You might be right about this or not. I'm not an expert in PBKDF2 either, but are you able to give a source for your assumption? It might help me investigating that sooner. For instance do you have a specification available for PBKDF2?

[arkadiusz-wolanski]

Unfortunately, I am not an expert either. The error occurred when we tried to decrypt a password with this method.

As a reference we used the following site:

https://onlinephp.io/hash-pbkdf2

Here the result is extended by the additional bytes of the requested key length.

16 Byte: 05e7b33f93ea1d35 32 Byte: 05e7b33f93ea1d35939b32ad5994ef63 48 Byte: 05e7b33f93ea1d35939b32ad5994ef6314f6170b191368e9

[denovosoftware]

Can confirm this issue.

Root cause appears to be an unexpected buffer reuse in TDECHashAuthentication.PBKDF2, specifically that Result is referring to same TBytes as T after initial round (maybe an optimization for nil cases?), which will cause next round to overwrite first block Result := Result + T; You can use something like SetLength(Result, Length(Result)); to make it work as expected.

PBKDF2 is described in RFC 2898, section 5.2

[MHumm]

I am currently trying to implement a regression test before looking at the proposed fix. But I have some issue: the result I get for the 16 byte length variant is the one listed for the 32 byte variant above. The linked PHP implementation delivers the result listed for 16 byte though. Is this part of the flaw in DEC and will get fixed with the proposed fix?

[MHumm]

About the proposed fix: Where would I need to put that SetLength call exactly and what does setting the length of the result to it's length do? If length = 16 it will be 16 before and after?

[denovosoftware]

SetLength is a bit excessive as it makes a copy every time, so I think that using an explicit copy during initial round is going to be clearer and also solves the issue:

      if I = 1 then
        Result := Copy(T)
      else
        Result := Result + T; 

But, if there are other cases like this, it might be better to have a proper method to call and encapsulate details inside.

[denovosoftware]

Those are the checks for TestRawByteString that validate the fix:

  result := StringOf(ValidFormat(TFormat_HEX).Encode(THash_SHA1.PBKDF2('password', 'salt', 1, 16)));
  CheckEquals('0C60C80F961F0E71F3A9B524AF601206', result, 'SHA1 password salt 1 16');

  result := StringOf(ValidFormat(TFormat_HEX).Encode(THash_SHA1.PBKDF2('password', 'salt', 1, 20)));
  CheckEquals('0C60C80F961F0E71F3A9B524AF6012062FE037A6', result, 'SHA1 password salt 1 20');

  result := StringOf(ValidFormat(TFormat_HEX).Encode(THash_SHA1.PBKDF2('password', 'salt', 1, 32)));
  CheckEquals('0C60C80F961F0E71F3A9B524AF6012062FE037A6E0F0EB94FE8FC46BDC637164', result, 'SHA1 password salt 1 32');

  // This is testing 3 blocks
  result := StringOf(ValidFormat(TFormat_HEX).Encode(THash_SHA1.PBKDF2('password', 'salt', 1, 48)));
  CheckEquals('0C60C80F961F0E71F3A9B524AF6012062FE037A6E0F0EB94FE8FC46BDC637164AC2E7A8E3F9D2E83ACE57E0D50E5E107', result, 'SHA1 password salt 1 48');

Note that 20 (40 hex encoded chars) bytes check is an existing one and that new ones are confirming that same sequence is being constructed. Without the fix, 32 and 48 bytes would fail and show completely different results.

I also went to PHP portal and result I got for 48 bytes was incomplete, as it was shorter than 48 bytes, likely a bug.

[MHumm]

I implemented your fix now. Thanks for providing. One remark though: it would have made me less guessing about where to put your "SetLength" replacement if you had written which line it should replace. But it works now, so I close this as fixed.

[fperana]

Sorry for commenting on an already closed case, but I think the solution applied is less than optimal (it's more of a workaround than a solution).

According to Delphi's inner workings, a function's Result variable is actually implemented as an implicit var parameter (look at this Stack Overflow's Q/A), so you either have to be sure that your Result variable is assigned with some certain value, or is initialized before you start adding values to.

The TDECHashAuthentication.PBKDF2 function adds values to Result without initializing it, so each subsequent call will simply add new values to those of previous calls.

The true solution for this case is to add Result := nil; at the very beginning of TDECHashAuthentication.PBKDF2.

And remember to always initialize the Result variable in functions if it's not directly assigned with some certain value.

[MHumm]

Sorry for just answering now. It's completely valid to reopen a discussion on some already closed issue if it I implemented something for this in developmend branch now . I didn't use nil but SetLength(Result, 0); I hope this is ok.

[fperana]

SetLength(Result, 0) is identical to Result := nil, no problems here.

It's better to put this line at the very beginning of the function (generally speaking), so in every possible situation we can be sure that the Result variable can't be undefined. Putting it in a test inside a loop, while in this specific case will work (the loop will definitely run at least once), could have two drawbacks:

1. if, for some reasons, the loop won't start (future manipulation of the code could cause this to happen), Result will remain undefined
2. the test will run at every iteration, which is suboptimal

Not big deals, but particularly the first one for me is a good reason to stay on the safe side, especially in a cryptographic library.

[MHumm]

While I agree with your statements I'm not 100% sure what you want to tell me with those: do I need to make any further change(s)? Otherwise I'd say this issue is really closed now. Correct?

[fperana]

Well, I've issued a pull request exactly for this reason ;) You can merge it into your code, it implements what I exposed here.