Re: [RFC] Timing attack safe string comparison function

On Mon, Dec 23, 2013 at 5:45 PM, Rouven Weßling <me@rouvenwessling.de>wrote: > Thanks everyone for your feedback, answers inline. > > On 22.12.2013, at 18:25, Andrea Faulds <ajf@ajf.me> wrote: > > > I note your patch uses C++-style (// foobar) comments. However, > according to the coding standards[0], only C-style (/* foobar */) comments > should be used. > > Thanks for the hint, I changed the patch accordingly. > > On 23.12.2013, at 00:55, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > > > As you mentioned in code, users should not use when known or user > supplied string > > is null. > > > > How about add E_NOTICE error for that case? > > If user shouldn't then we are better to warn them. > > That's a fair point, but I expect people would in that case just do their > own check of strlen() === 0 and error out and nothing is gained. Maybe we > can find a way to not need the check at all. (see next response) > > > Comparison is good since it always does the same operation based on user > supplied > > string. (Unless compiler does optimizations that I don't expect) > > Thanks for checking that, the people do the better. > > On 23.12.2013, at 02:26, Tjerk Meesters <tjerk.meesters@gmail.com> wrote: > > > On the whole it looks okay. > > > > The special branch for known_len == 0 && user_len != > > 0 can be avoided > by doing something like this: > > > > mod_len = max(known_len, 1); > > > > And then use j % mod_len instead of j % > > known_len to avoid a > division by zero; since x mod 1 always yields > 0 you will always be > comparing against the null byte of the known string. > > This looks like a good approach, but I was under the impression, that PHP > strings aren't guaranteed to have a terminating null byte. Am I mistaken? > Strings are binary safe, but they're still null terminated, as can be seen from the definitions of ZVAL_STRING() and ZVAL_STRINGL(). http://lxr.php.net/xref/PHP_5_6/Zend/zend_API.h#576 > > On 23.12.2013, at 10:09, Joe Watkins <pthreads@pthreads.org> wrote: > > > This does not appear to solve any problems, it appears to add > another function, for that function to solve any problems it must be > deployed. > > So the RFC relies on everyone swapping out every security > sensitive string comparison with the new function, which simply will not > happen. > > It's true that adding this function won't magically make any application > safer. The whole point is making it easier for application developers - > especially those not using a huge framework - to use a string comparison > algorithm that - hopefully - many people have reviewed. Also if there's an > issue, this will be fixed by the regular PHP updates (or distributions > backports) > > > I'm up for doing something about security, however, this doesn't > actually do that, what it does is add a (generically named) function that > nobody is very likely to deploy, and doesn't fix the vulnerability in > existing code ... which surely has to be the aim of anything targeted at > security - existing code. > > > > Obviously, we cannot really change all string comparisons to use > security sensitive logic, so this isn't something we can really solve > everywhere from the core, some action must be taken by the user ... > > As you mention yourself, we obviously can't force every string comparison > to be time constant. This means there's no "magic bullet" and I think this > is the second best thing to do. > > > It might have more traction if the function were named > password_compare or hash_compare or something similar that gives everyone > the idea that it is not simply a string comparison function but the correct > way to verify in particular passwords/hashes or whatever. I'd be much more > inclined to say that's a good idea, providing a full set of tools for > password related foo. > > I don't care about the name at all (it's mentioned as an open issue in the > RFC), it's admittedly the second one that came to my mind (after > str_compare_time_constant which is way too long). hash_compare does sound > pretty good though. > > Best regards > Rouven -- -- Tjerk