Re: [RFC] Timing attack safe string comparison function

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? 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