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