Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:70849
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain rouvenwessling.de designates 5.35.242.46 as permitted sender)
Content-Type: text/plain; charset=iso-8859-1
Mime-Version: 1.0 (Mac OS X Mail 7.0 \(1822\))
In-Reply-To: <52B7209C.2040802@ajf.me>
Date: Mon, 23 Dec 2013 10:45:29 +0100
Cc: List PHP Mailing List <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <3EFE191D-2A93-47E9-805D-538950849C96@rouvenwessling.de>
References: <3014595E-B155-47F6-AC7B-71083D89525D@rouvenwessling.de> <52B7209C.2040802@ajf.me>
To: Andrea Faulds <ajf@ajf.me>,
 yohgaki@ohgaki.net,
 Tjerk Meesters <tjerk.meesters@gmail.com>,
 Joe Watkins <pthreads@pthreads.org>
Subject: Re: [PHP-DEV] [RFC] Timing attack safe string comparison function
From: me@rouvenwessling.de (=?iso-8859-1?Q?Rouven_We=DFling?=)

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.
>=20
> 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() =3D=3D=3D 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.
>=20
> The special branch for `known_len =3D=3D 0 && user_len !=3D 0` can be =
avoided by doing something like this:
>=20
>     mod_len =3D max(known_len, 1);
>=20
> 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.
>=20
> 	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=