Skip to content
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

Use hash_equals for constant-time string comparison #4206

Closed
wants to merge 1 commit into from

Conversation

dunglas
Copy link
Contributor

@dunglas dunglas commented Sep 2, 2014

Use the hash_equals function (introduced in PHP 5.6) for timing attack safe string comparison when available.
Add in the DocBlock that length will leak (see php/php-src#792).

@wilsonge
Copy link
Contributor

wilsonge commented Sep 2, 2014

#3832

Similar issue, alternative fix, we received a while back (less good but full PHP version support)

@dunglas
Copy link
Contributor Author

dunglas commented Sep 2, 2014

@wilsonge I don't know if this better to use HMAC or hash_equals for passwords but, if this method is part of the Joomla API both patch can be merged. It will benefit at least to plugins using it.

@sarciszewski
Copy link

👍 for using native hash_equals() if it exists.

@RCheesley
Copy link

Can you please provide test instructions?

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4206.

@brianteeman
Copy link
Contributor

As this requires php 5.6 which is higher than our minimum requirement how can this be merged?

Setting to Needs Review so the CMS maintainers can make a decision


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4206.

@sarciszewski
Copy link

As this requires php 5.6 which is higher than our minimum requirement how can this be merged?

Have you even read what the pull request does? It's wrapped inside of a if (function_exists('hash_equals')) block...

@Hackwar
Copy link
Member

Hackwar commented Feb 3, 2015

The PR looks good, but please update the PR to latest staging so that Travis is run and can give us a proper result.

@mbabker
Copy link
Contributor

mbabker commented Aug 21, 2015

I don't see an issue with this. We've done similar things in the past where we use native PHP functions conditionally and fallback to something else if it isn't available. As requested previously though, can this PR be sync'd with the staging branch so it can be properly tested by both users and the CI suite? Also, there is one codestyle issue to fix; the opening bracket for the if statement should be on a new line.

@zero-24
Copy link
Contributor

zero-24 commented Aug 23, 2015

This PR: #7754 fixes the codestyle issue and replaces this here. Thanks @dunglas !

@zero-24 zero-24 closed this Aug 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants