Re: [RFC] Timing attack safe string comparison function

On 12/23/2013 04:46 PM, Rouven Weßling wrote:

Hi Marco. On 23.12.2013, at 12:04, Marco Pivetta <ocramius@gmail.com> wrote:

I was wondering why such an API must be implemented in PHP core (which means C, which means that the usual 15~20 people can fix it if borked, which is bad) and cannot be just left in userland as it already happens, for example, with https://github.com/zendframework/zf2/blob/master/library/Zend/Crypt/Utils.php#L17-L44 and similar libraries that have some decent security policies themselves (nothing to say about PHP - you guys are doing great!). Why do we need this in core? Why can't a user copy-paste those rows (if it's a monkey-patcher) or just use a library?

Obviously this doesn't have to be in core, but there are a number of advantages: * Increased awareness * Robuster implementation since it's in a lower level language (see Stas' E-Mail earlier) * Wider security review For example the linked Zend example has a small issue because it actually leaks length information. That's unimportant if one is comparing a hash (they usually have a constant length) but for other applications it might be an issue. Also the simple fact that basically everyone (Joomla, Zend, Symfony2) ships some function for this, shows that there's a high demand for this functionality. Why not provide it out of the box?

Don't get me wrong: I am all for security, but I don't see a difference between a php-core implementation and a userland implementation.

The hope is, like with the password hashing function, that by making it easier to use best practices, coders will follow them. On 23.12.2013, at 12:25, Marco Pivetta <ocramius@gmail.com> wrote:

On 23 December 2013 12:16, Joe Watkins <krakjoe@php.net> wrote:

291    /* We're using this method instead of == in order to provide
292     * resistence towards timing attacks. This is a constant time
293     * equality check that will always check every byte of both
294     * values. */
295    for (i = 0; i < hash_len; i++) {
296        status |= (ret[i] ^ hash[i]);
297    }

So that puts in perspective the what if it borks argument, and the complication argument too, since the new function and old can share a static inline implementation of the same logic ... do you really want me to explain why static inline c is better than PHP, or is that obvious at this point ??

The performance question is irrelevant to me - I don't think I'd ever code a performance-sensitive API with this sort of function, but maybe someone has a real world example for that. Slower is probably even better here :P Let me re-state: "performance is not a problem here".

Indeed, this is not a function where performance is critical and it will likely be so rarely called even by its heaviest users that the difference of C vs PHP won't even make a dent in resource usage. I've updated the patch with Tjerk's suggestion and renamed the function to hash_compare. I've also updated the RFC accordingly. Best regards Rouven

It might belong in ext/hash, not sure ... It's not a great idea to have it with the password stuff, as I first suggested, but I do think the two should share code, there's no reason to have two internal implementations of this, can you not make the code from password into a ZEND_API function and share it with this wherever you put it ?? Cheers Joe