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