Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:70863
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.51 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <3EFE191D-2A93-47E9-805D-538950849C96@rouvenwessling.de>
References: <3014595E-B155-47F6-AC7B-71083D89525D@rouvenwessling.de>
	<52B7209C.2040802@ajf.me>
	<3EFE191D-2A93-47E9-805D-538950849C96@rouvenwessling.de>
Date: Mon, 23 Dec 2013 22:15:06 +0800
Message-ID: <CAHMUw2EDwZZS+fWwAUSQS_oTdSqCB9nAwxCHvxOTXbVsuhRXfA@mail.gmail.com>
To: =?ISO-8859-1?Q?Rouven_We=DFling?= <me@rouvenwessling.de>
Cc: Andrea Faulds <ajf@ajf.me>, Yasuo Ohgaki <yohgaki@ohgaki.net>, Joe Watkins <pthreads@pthreads.org>, 
	List PHP Mailing List <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=089e010d973031dda604ee3441d3
Subject: Re: [PHP-DEV] [RFC] Timing attack safe string comparison function
From: tjerk.meesters@gmail.com (Tjerk Meesters)

--089e010d973031dda604ee3441d3
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

On Mon, Dec 23, 2013 at 5:45 PM, Rouven We=DFling <me@rouvenwessling.de>wro=
te:

> 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 */) comment=
s
> 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() =3D=3D=3D 0 and error out and nothing is gained. Ma=
ybe 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 use=
r
> 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 =3D=3D 0 && user_len !=3D 0` can be a=
voided
> by doing something like this:
> >
> >     mod_len =3D 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 a=
n
> 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 thi=
s
> 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 corre=
ct
> way to verify in particular passwords/hashes or whatever. I'd be much mor=
e
> 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 th=
e
> 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




--=20
--
Tjerk

--089e010d973031dda604ee3441d3--