Skip to content

Fix: gost-crypto hash incorrect if input data contains long 0xFF sequence #2391

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

Closed
wants to merge 4 commits into from

Conversation

Grundik
Copy link
Contributor

@Grundik Grundik commented Feb 18, 2017

https://bugs.php.net/bug.php?id=73127
Problem was lying in calculations of Sum-block.

@Grundik
Copy link
Contributor Author

Grundik commented Feb 18, 2017

Problem was within overflow detection: if result is less than any of two arguments, than there was overflow. But in fact there are three arguments, so with the right data overflow would not be detected: 0x00000001 + 0xFFFFFFFF+0xFFFFFFFF = 0xFFFFFFFF.

My fix changes operations to 16-bits, so there are no undetectable overflows possible.

@nikic
Copy link
Member

nikic commented Feb 18, 2017

Could you please add a test for this change?

Also, the implementation looks unnecessarily complicated. For example, this is how the same code in rhash looks like: https://github.com/rhash/RHash/blob/master/librhash/gost.c#L326

@nikic nikic added the Bug label Feb 18, 2017
@Grundik
Copy link
Contributor Author

Grundik commented Feb 18, 2017

I have modified overflow detection code and added test. Feel free to move test to more appropriate place if necessary.

@krakjoe
Copy link
Member

krakjoe commented Feb 22, 2017

@nikic can you take care of this one please ?

@Grundik
Copy link
Contributor Author

Grundik commented Feb 22, 2017

Since all versions of PHP are affected by this bug: https://3v4l.org/0IECu, it would be very convenient if this fix would be applied to all supported PHP versions.

@nikic
Copy link
Member

nikic commented Feb 24, 2017

Sorry for the delay, now merged into 7.0+ via eac8166. Thanks!

@nikic nikic closed this Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants