From c840f71524067aa474c00c3eacfb83bd860bfc8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Mon, 23 Jan 2023 21:15:24 +0100 Subject: [PATCH 1/5] crypt: Fix validation of malformed BCrypt hashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PHP’s implementation of crypt_blowfish differs from the upstream Openwall version by adding a “PHP Hack”, which allows one to cut short the BCrypt salt by including a `$` character within the characters that represent the salt. Hashes that are affected by the “PHP Hack” may erroneously validate any password as valid when used with `password_verify` and when comparing the return value of `crypt()` against the input. The PHP Hack exists since the first version of PHP’s own crypt_blowfish implementation that was added in 1e820eca02dcf322b41fd2fe4ed2a6b8309f8ab5. No clear reason is given for the PHP Hack’s existence. This commit removes it, because BCrypt hashes containing a `$` character in their salt are not valid BCrypt hashes. --- ext/standard/crypt_blowfish.c | 8 -- .../tests/crypt/bcrypt_salt_dollar.phpt | 82 +++++++++++++++++++ 2 files changed, 82 insertions(+), 8 deletions(-) create mode 100644 ext/standard/tests/crypt/bcrypt_salt_dollar.phpt diff --git a/ext/standard/crypt_blowfish.c b/ext/standard/crypt_blowfish.c index 3806a290aee..351d4030808 100644 --- a/ext/standard/crypt_blowfish.c +++ b/ext/standard/crypt_blowfish.c @@ -371,7 +371,6 @@ static const unsigned char BF_atoi64[0x60] = { #define BF_safe_atoi64(dst, src) \ { \ tmp = (unsigned char)(src); \ - if (tmp == '$') break; /* PHP hack */ \ if ((unsigned int)(tmp -= 0x20) >= 0x60) return -1; \ tmp = BF_atoi64[tmp]; \ if (tmp > 63) return -1; \ @@ -399,13 +398,6 @@ static int BF_decode(BF_word *dst, const char *src, int size) *dptr++ = ((c3 & 0x03) << 6) | c4; } while (dptr < end); - if (end - dptr == size) { - return -1; - } - - while (dptr < end) /* PHP hack */ - *dptr++ = 0; - return 0; } diff --git a/ext/standard/tests/crypt/bcrypt_salt_dollar.phpt b/ext/standard/tests/crypt/bcrypt_salt_dollar.phpt new file mode 100644 index 00000000000..32e335f4b08 --- /dev/null +++ b/ext/standard/tests/crypt/bcrypt_salt_dollar.phpt @@ -0,0 +1,82 @@ +--TEST-- +bcrypt correctly rejects salts containing $ +--FILE-- + +--EXPECT-- +string(8) "$2y$04$$" +string(2) "*0" +bool(false) +string(9) "$2y$04$0$" +string(2) "*0" +bool(false) +string(10) "$2y$04$00$" +string(2) "*0" +bool(false) +string(11) "$2y$04$000$" +string(2) "*0" +bool(false) +string(12) "$2y$04$0000$" +string(2) "*0" +bool(false) +string(13) "$2y$04$00000$" +string(2) "*0" +bool(false) +string(14) "$2y$04$000000$" +string(2) "*0" +bool(false) +string(15) "$2y$04$0000000$" +string(2) "*0" +bool(false) +string(16) "$2y$04$00000000$" +string(2) "*0" +bool(false) +string(17) "$2y$04$000000000$" +string(2) "*0" +bool(false) +string(18) "$2y$04$0000000000$" +string(2) "*0" +bool(false) +string(19) "$2y$04$00000000000$" +string(2) "*0" +bool(false) +string(20) "$2y$04$000000000000$" +string(2) "*0" +bool(false) +string(21) "$2y$04$0000000000000$" +string(2) "*0" +bool(false) +string(22) "$2y$04$00000000000000$" +string(2) "*0" +bool(false) +string(23) "$2y$04$000000000000000$" +string(2) "*0" +bool(false) +string(24) "$2y$04$0000000000000000$" +string(2) "*0" +bool(false) +string(25) "$2y$04$00000000000000000$" +string(2) "*0" +bool(false) +string(26) "$2y$04$000000000000000000$" +string(2) "*0" +bool(false) +string(27) "$2y$04$0000000000000000000$" +string(2) "*0" +bool(false) +string(28) "$2y$04$00000000000000000000$" +string(2) "*0" +bool(false) +string(29) "$2y$04$000000000000000000000$" +string(2) "*0" +bool(false) +string(30) "$2y$04$0000000000000000000000$" +string(60) "$2y$04$000000000000000000000u2a2UpVexIt9k3FMJeAVr3c04F5tcI8K" +bool(false) From a92acbad873a05470af1a47cb785a18eadd827b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= Date: Mon, 23 Jan 2023 22:13:57 +0100 Subject: [PATCH 2/5] crypt: Fix possible buffer overread in php_crypt() --- ext/standard/crypt.c | 1 + ext/standard/tests/password/password_bcrypt_short.phpt | 8 ++++++++ 2 files changed, 9 insertions(+) create mode 100644 ext/standard/tests/password/password_bcrypt_short.phpt diff --git a/ext/standard/crypt.c b/ext/standard/crypt.c index 8c105cf910e..8316c8b9606 100644 --- a/ext/standard/crypt.c +++ b/ext/standard/crypt.c @@ -135,6 +135,7 @@ PHPAPI zend_string *php_crypt(const char *password, const int pass_len, const ch } else if ( salt[0] == '$' && salt[1] == '2' && + salt[2] != 0 && salt[3] == '$') { char output[PHP_MAX_SALT_LEN + 1]; diff --git a/ext/standard/tests/password/password_bcrypt_short.phpt b/ext/standard/tests/password/password_bcrypt_short.phpt new file mode 100644 index 00000000000..085bc8a2390 --- /dev/null +++ b/ext/standard/tests/password/password_bcrypt_short.phpt @@ -0,0 +1,8 @@ +--TEST-- +Test that password_hash() does not overread buffers when a short hash is passed +--FILE-- + +--EXPECT-- +bool(false) From af2ddc64262216848d2e95108c121967a2b0a8a9 Mon Sep 17 00:00:00 2001 From: Stanislav Malyshev Date: Sun, 12 Feb 2023 20:53:06 -0700 Subject: [PATCH 3/5] Update NEWS --- NEWS | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS b/NEWS index 907a06b90ec..82d773a3b2a 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,9 @@ PHP NEWS ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||| ?? ??? ????, PHP 8.0.28 +- Core: + . Fixed bug #81744 (Password_verify() always return true with some hash). + (CVE-2023-0567). (Tim Düsterhus) 05 Jan 2023, PHP 8.0.27 From ec10b28d64decbc54aa1e585dce580f0bd7a5953 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 27 Jan 2023 19:28:27 +0100 Subject: [PATCH 4/5] Fix array overrun when appending slash to paths Fix it by extending the array sizes by one character. As the input is limited to the maximum path length, there will always be place to append the slash. As the php_check_specific_open_basedir() simply uses the strings to compare against each other, no new failures related to too long paths are introduced. We'll let the DOM and XML case handle a potentially too long path in the library code. --- ext/dom/document.c | 2 +- ext/xmlreader/php_xmlreader.c | 2 +- main/fopen_wrappers.c | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/dom/document.c b/ext/dom/document.c index dbbabb8bffd..fa6a6377ad5 100644 --- a/ext/dom/document.c +++ b/ext/dom/document.c @@ -1174,7 +1174,7 @@ static xmlDocPtr dom_document_parser(zval *id, int mode, char *source, size_t so int validate, recover, resolve_externals, keep_blanks, substitute_ent; int resolved_path_len; int old_error_reporting = 0; - char *directory=NULL, resolved_path[MAXPATHLEN]; + char *directory=NULL, resolved_path[MAXPATHLEN + 1]; if (id != NULL) { intern = Z_DOMOBJ_P(id); diff --git a/ext/xmlreader/php_xmlreader.c b/ext/xmlreader/php_xmlreader.c index ecc52aa1d6f..d8f8105010c 100644 --- a/ext/xmlreader/php_xmlreader.c +++ b/ext/xmlreader/php_xmlreader.c @@ -1024,7 +1024,7 @@ PHP_METHOD(XMLReader, XML) xmlreader_object *intern = NULL; char *source, *uri = NULL, *encoding = NULL; int resolved_path_len, ret = 0; - char *directory=NULL, resolved_path[MAXPATHLEN]; + char *directory=NULL, resolved_path[MAXPATHLEN + 1]; xmlParserInputBufferPtr inputbfr; xmlTextReaderPtr reader; diff --git a/main/fopen_wrappers.c b/main/fopen_wrappers.c index 39a9c82d598..5e4a619c8fe 100644 --- a/main/fopen_wrappers.c +++ b/main/fopen_wrappers.c @@ -129,10 +129,10 @@ PHPAPI ZEND_INI_MH(OnUpdateBaseDir) */ PHPAPI int php_check_specific_open_basedir(const char *basedir, const char *path) { - char resolved_name[MAXPATHLEN]; - char resolved_basedir[MAXPATHLEN]; + char resolved_name[MAXPATHLEN + 1]; + char resolved_basedir[MAXPATHLEN + 1]; char local_open_basedir[MAXPATHLEN]; - char path_tmp[MAXPATHLEN]; + char path_tmp[MAXPATHLEN + 1]; char *path_file; size_t resolved_basedir_len; size_t resolved_name_len; From b5ccaaf613066e3862cca86dbe1f39d3c376649a Mon Sep 17 00:00:00 2001 From: Stanislav Malyshev Date: Sun, 12 Feb 2023 21:04:31 -0700 Subject: [PATCH 5/5] Update NEWS --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 82d773a3b2a..a6dfbec2629 100644 --- a/NEWS +++ b/NEWS @@ -5,6 +5,8 @@ PHP NEWS - Core: . Fixed bug #81744 (Password_verify() always return true with some hash). (CVE-2023-0567). (Tim Düsterhus) + . Fixed bug #81746 (1-byte array overrun in common path resolve code). + (CVE-2023-0568). (Niels Dossche) 05 Jan 2023, PHP 8.0.27