New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
THash_SHA1.PBKDF2 create wrong Results #46
Comments
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? |
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 |
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 PBKDF2 is described in RFC 2898, section 5.2 |
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. |
About the proposed fix: |
Started to implement regression test. Removed compatibility with compiler versions < 28.0 (XE7)
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:
But, if there are other cases like this, it might be better to have a proper method to call and encapsulate details inside. |
Those are the checks for TestRawByteString that validate the fix:
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. |
Proposed fix runs correctly with proposed test data. Also did some formatting in the HMAC unit tests.
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. |
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 The The true solution for this case is to add And remember to always initialize the Result variable in functions if it's not directly assigned with some certain value. |
Added Result value initialization to TDECHashAuthentication.PBKDF2 and deleted workaround previously added to fix MHumm#46
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. |
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.
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. |
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? |
Well, I've issued a pull request exactly for this reason ;) |
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.
The text was updated successfully, but these errors were encountered: