Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:99877
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: softfail (pb1.pair.com: domain golemon.com does not designate 74.125.82.46 as permitted sender)
MIME-Version: 1.0
Sender: php@golemon.com
Date: Sat, 15 Jul 2017 17:09:24 -0400
Message-ID: <CAESVnVpm3W0OfuG62p4iCg4Ff8oY1WpvmH0w24fTezW5aWMDVQ@mail.gmail.com>
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: php_crypt() problems
From: pollita@php.net (Sara Golemon)

While exploring password_*() functions, I found myself in php_crypt()
and noticed a use-after-free in the PHP_USE_PHP_CRYPT_R==0 case.


char *crypt_res;
{
#if something
  struct crypt_data buffer;
#else
  CRYPTD buffer;
#endif
  crypt_res = crypt_r(password, salt, &buffer);
}
return zend_string_init(crypt_res, strlen(crypt_res), 0);


The above *looks* fine, until you realize that crypt_r() returns a
pointer to a portion of `buffer`.  Buffer has fallen out of scope
though, so according to spec, it's undefined as to whether or not the
memory address will still be trustable.  In practice, gcc is okay, but
again, not due to defined behavior.

---

But wait, there's more.  I started moving a few things around to fix
this (which turned into something of a refactor) and when I ran the
tests, everything was fine.  But of course, my build has
PHP_USE_PHP_CRYPT_R==1 (as many do, I suspect).  So I rebuilt and ran
the tests manually flipping PHP_USE_PHP_CRYPT_R to Zero. and the
following tests now fail:

Test DES with invalid fallback
[ext/standard/tests/crypt/des_fallback_invalid_salt.phpt]
Test deprecated operation of password_hash()
[ext/standard/tests/password/password_deprecated_salts.phpt]
Test normal operation of password_hash()
[ext/standard/tests/password/password_hash.phpt]
Test normal operation of password_verify)
[ext/standard/tests/password/password_verify.phpt]
Bug #73058 crypt broken when salt is 'too' long
[ext/standard/tests/strings/bug73058.phpt]
crypt() function [ext/standard/tests/strings/crypt.phpt]
Official blowfish tests [ext/standard/tests/strings/crypt_blowfish.phpt]
crypt() function - characters > 0x80
[ext/standard/tests/strings/crypt_chars.phpt]
crypt(): *0 should return *1 [ext/standard/tests/strings/crypt_des_error.phpt]

In some cases, this is just a matter of the two implementations
falling out of sync, however a few come down to actual behavioral
differences between php_crypt_r() and crypt_r().  This may, of course,
be down to my crypt() being broken somehow and that's *why* my build
is using the more portable php_crypt_r() implementation...

---

TL;DR - Do we have any idea (maybe from QA reports?) how many people
are even /using/ system crypt(), and does it make any kind of sense to
just remove support for it (and all the myriad #ifdef checks) given
that we have a much more portable implementation?

-Sara