Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:70864
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=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 7.0 \(1822\))
In-Reply-To: <CADyq6sJESC+UG_Vb7Wq=nHQWF0-5zL=7Yzpi2L8rr4CQwyr5ug@mail.gmail.com>
Date: Mon, 23 Dec 2013 17:46:14 +0100
Cc: List PHP Mailing List <internals@lists.php.net>
Content-Transfer-Encoding: quoted-printable
Message-ID: <7CB7C22A-A16D-4D90-A063-143BAC522077@rouvenwessling.de>
References: <3014595E-B155-47F6-AC7B-71083D89525D@rouvenwessling.de> <52B80C5B.2050208@sugarcrm.com> <6C0AE2F4-26DA-4081-914A-F6D18C46911C@rouvenwessling.de> <52B81559.8080409@php.net> <CADyq6sJESC+UG_Vb7Wq=nHQWF0-5zL=7Yzpi2L8rr4CQwyr5ug@mail.gmail.com>
To: Marco Pivetta <ocramius@gmail.com>
Subject: Re: [PHP-DEV] [RFC] Timing attack safe string comparison function
From: me@rouvenwessling.de (=?iso-8859-1?Q?Rouven_We=DFling?=)

Hi Marco.

On 23.12.2013, at 12:04, Marco Pivetta <ocramius@gmail.com> wrote:

> I was wondering why such an API must be implemented in PHP core (which =
means C, which means that the usual 15~20 people can fix it if borked, =
which is bad) and cannot be just left in userland as it already happens, =
for example, with =
https://github.com/zendframework/zf2/blob/master/library/Zend/Crypt/Utils.=
php#L17-L44 and similar libraries that have some decent security =
policies themselves (nothing to say about PHP - you guys are doing =
great!).
>=20
> Why do we need this in core?
> Why can't a user copy-paste those rows (if it's a monkey-patcher) or =
just use a library?

Obviously this doesn't have to be in core, but there are a number of =
advantages:
* Increased awareness
* Robuster implementation since it's in a lower level language (see =
Stas' E-Mail earlier)
* Wider security review

For example the linked Zend example has a small issue because it =
actually leaks length information. That's unimportant if one is =
comparing a hash (they usually have a constant length) but for other =
applications it might be an issue.

Also the simple fact that basically everyone (Joomla, Zend, Symfony2) =
ships some function for this, shows that there's a high demand for this =
functionality. Why not provide it out of the box?

> Don't get me wrong: I am all for security, but I don't see a =
difference between a php-core implementation and a userland =
implementation.

The hope is, like with the password hashing function, that by making it =
easier to use best practices, coders will follow them.

On 23.12.2013, at 12:25, Marco Pivetta <ocramius@gmail.com> wrote:

> On 23 December 2013 12:16, Joe Watkins <krakjoe@php.net> wrote:
>=20
>> 291    /* We're using this method instead of =3D=3D in order to =
provide
>> 292     * resistence towards timing attacks. This is a constant time
>> 293     * equality check that will always check every byte of both
>> 294     * values. */
>> 295    for (i =3D 0; i < hash_len; i++) {
>> 296        status |=3D (ret[i] ^ hash[i]);
>> 297    }
>>=20
>> So that puts in perspective the what if it borks argument, and the
>> complication argument too, since the new function and old can share a
>> static inline implementation of the same logic ... do you really want =
me to
>> explain why static inline c is better than PHP, or is that obvious at =
this
>> point ??
>=20
> The performance question is irrelevant to me - I don't think I'd ever =
code
> a performance-sensitive API with this sort of function, but maybe =
someone
> has a real world example for that. Slower is probably even better here =
:P
>=20
> Let me re-state: "performance is not a problem here".

Indeed, this is not a function where performance is critical and it will =
likely be so rarely called even by its heaviest users that the =
difference of C vs PHP won't even make a dent in resource usage.

I've updated the patch with Tjerk's suggestion and renamed the function =
to hash_compare. I've also updated the RFC accordingly.

Best regards
Rouven