From f3c58a5ed6bbb22260d2d848cc2bfd36ed8b00e3 Mon Sep 17 00:00:00 2001 From: Dik Takken Date: Fri, 25 Sep 2020 15:54:28 +0200 Subject: [PATCH] Make handling of NULL bytes in file paths more consistent (WIP) Not all extensions consistently throw exceptions when the user passes a path name containing null bytes. Also, some extensions would throw a ValueError while others would throw a TypeError. Error messages also varied. Now a ValueError is thrown after all failed path checks, at least for as far as these occur in functions that are exposed to userland. Closes GH-6216. --- ext/dom/document.c | 41 ++++++++++--------- .../tests/DOMDocument_loadHTML_error2.phpt | 10 +++-- .../DOMDocument_loadHTMLfile_error2.phpt | 22 ++++++---- ext/dom/tests/DOMDocument_loadXML_error6.phpt | 16 ++++++++ ext/dom/tests/DOMDocument_load_error6.phpt | 27 ++++++++++++ ...ocument_saveHTMLFile_invalid_filename.phpt | 10 +++-- ...MDocument_schemaValidateSource_error3.phpt | 12 +++--- .../DOMDocument_schemaValidate_error3.phpt | 12 +++--- .../DOMDocument_schemaValidate_error6.phpt | 25 +++++++++++ ext/fileinfo/fileinfo.c | 2 +- ext/fileinfo/tests/finfo_file_001.phpt | 2 +- ext/fileinfo/tests/finfo_file_basic.phpt | 2 +- ext/fileinfo/tests/mime_content_type_001.phpt | 2 +- ext/sqlite3/sqlite3.c | 19 +-------- 14 files changed, 137 insertions(+), 65 deletions(-) create mode 100644 ext/dom/tests/DOMDocument_loadXML_error6.phpt create mode 100644 ext/dom/tests/DOMDocument_load_error6.phpt create mode 100644 ext/dom/tests/DOMDocument_schemaValidate_error6.phpt diff --git a/ext/dom/document.c b/ext/dom/document.c index d3a8fa43a5..23bdcae1e0 100644 --- a/ext/dom/document.c +++ b/ext/dom/document.c @@ -1197,6 +1197,7 @@ static xmlDocPtr dom_document_parser(zval *id, int mode, char *source, size_t so if (mode == DOM_LOAD_FILE) { char *file_dest; if (CHECK_NULL_PATH(source, source_len)) { + zend_value_error("Path to document must not contain any null bytes"); return NULL; } file_dest = _dom_get_valid_file_path(source, resolved_path, MAXPATHLEN); @@ -1305,8 +1306,8 @@ static void dom_parse_document(INTERNAL_FUNCTION_PARAMETERS, int mode) { } if (!source_len) { - php_error_docref(NULL, E_WARNING, "Empty string supplied as input"); - RETURN_FALSE; + zend_argument_value_error(1, "must not be empty"); + RETURN_THROWS(); } if (ZEND_SIZE_T_INT_OVFL(source_len)) { php_error_docref(NULL, E_WARNING, "Input string is too long"); @@ -1388,8 +1389,8 @@ PHP_METHOD(DOMDocument, save) } if (file_len == 0) { - php_error_docref(NULL, E_WARNING, "Invalid Filename"); - RETURN_FALSE; + zend_argument_value_error(1, "must not be empty"); + RETURN_THROWS(); } DOM_GET_OBJ(docp, id, xmlDocPtr, intern); @@ -1623,9 +1624,9 @@ static void _dom_document_schema_validate(INTERNAL_FUNCTION_PARAMETERS, int type RETURN_THROWS(); } - if (source_len == 0) { - php_error_docref(NULL, E_WARNING, "Invalid Schema source"); - RETURN_FALSE; + if (!source_len) { + zend_argument_value_error(1, "must not be empty"); + RETURN_THROWS(); } DOM_GET_OBJ(docp, id, xmlDocPtr, intern); @@ -1633,8 +1634,8 @@ static void _dom_document_schema_validate(INTERNAL_FUNCTION_PARAMETERS, int type switch (type) { case DOM_LOAD_FILE: if (CHECK_NULL_PATH(source, source_len)) { - php_error_docref(NULL, E_WARNING, "Invalid Schema file source"); - RETURN_FALSE; + zend_argument_value_error(1, "must not contain any null bytes"); + RETURN_THROWS(); } valid_file = _dom_get_valid_file_path(source, resolved_path, MAXPATHLEN); if (!valid_file) { @@ -1724,9 +1725,9 @@ static void _dom_document_relaxNG_validate(INTERNAL_FUNCTION_PARAMETERS, int typ RETURN_THROWS(); } - if (source_len == 0) { - php_error_docref(NULL, E_WARNING, "Invalid Schema source"); - RETURN_FALSE; + if (!source_len) { + zend_argument_value_error(1, "must not be empty"); + RETURN_THROWS(); } DOM_GET_OBJ(docp, id, xmlDocPtr, intern); @@ -1734,8 +1735,8 @@ static void _dom_document_relaxNG_validate(INTERNAL_FUNCTION_PARAMETERS, int typ switch (type) { case DOM_LOAD_FILE: if (CHECK_NULL_PATH(source, source_len)) { - php_error_docref(NULL, E_WARNING, "Invalid RelaxNG file source"); - RETURN_FALSE; + zend_argument_value_error(1, "must not contain any null bytes"); + RETURN_THROWS(); } valid_file = _dom_get_valid_file_path(source, resolved_path, MAXPATHLEN); if (!valid_file) { @@ -1823,8 +1824,8 @@ static void dom_load_html(INTERNAL_FUNCTION_PARAMETERS, int mode) /* {{{ */ } if (!source_len) { - php_error_docref(NULL, E_WARNING, "Empty string supplied as input"); - RETURN_FALSE; + zend_argument_value_error(1, "must not be empty"); + RETURN_THROWS(); } if (ZEND_LONG_EXCEEDS_INT(options)) { @@ -1834,8 +1835,8 @@ static void dom_load_html(INTERNAL_FUNCTION_PARAMETERS, int mode) /* {{{ */ if (mode == DOM_LOAD_FILE) { if (CHECK_NULL_PATH(source, source_len)) { - php_error_docref(NULL, E_WARNING, "Invalid file source"); - RETURN_FALSE; + zend_argument_value_error(1, "must not contain any null bytes"); + RETURN_THROWS(); } ctxt = htmlCreateFileParserCtxt(source, NULL); } else { @@ -1930,8 +1931,8 @@ PHP_METHOD(DOMDocument, saveHTMLFile) } if (file_len == 0) { - php_error_docref(NULL, E_WARNING, "Invalid Filename"); - RETURN_FALSE; + zend_argument_value_error(1, "must not be empty"); + RETURN_THROWS(); } DOM_GET_OBJ(docp, id, xmlDocPtr, intern); diff --git a/ext/dom/tests/DOMDocument_loadHTML_error2.phpt b/ext/dom/tests/DOMDocument_loadHTML_error2.phpt index 3167c01890..695f58868d 100644 --- a/ext/dom/tests/DOMDocument_loadHTML_error2.phpt +++ b/ext/dom/tests/DOMDocument_loadHTML_error2.phpt @@ -9,7 +9,11 @@ require_once('skipif.inc'); --FILE-- loadHTML(''); +try { + $doc->loadHTML(''); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} ?> ---EXPECTF-- -Warning: DOMDocument::loadHTML(): Empty string supplied as input in %s on line %d +--EXPECT-- +DOMDocument::loadHTML(): Argument #1 ($source) must not be empty diff --git a/ext/dom/tests/DOMDocument_loadHTMLfile_error2.phpt b/ext/dom/tests/DOMDocument_loadHTMLfile_error2.phpt index 55b36e712d..492bedda10 100644 --- a/ext/dom/tests/DOMDocument_loadHTMLfile_error2.phpt +++ b/ext/dom/tests/DOMDocument_loadHTMLfile_error2.phpt @@ -11,13 +11,19 @@ assert.bail=true --FILE-- loadHTMLFile(""); -assert($result === false); +try { + $result = $doc->loadHTMLFile(""); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + $doc = new DOMDocument(); -$result = $doc->loadHTMLFile("text.html\0something"); -assert($result === false); +try { + $result = $doc->loadHTMLFile("text.html\0something"); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} ?> ---EXPECTF-- -%r(PHP ){0,1}%rWarning: DOMDocument::loadHTMLFile(): Empty string supplied as input %s - -%r(PHP ){0,1}%rWarning: DOMDocument::loadHTMLFile(): Invalid file source %s +--EXPECT-- +DOMDocument::loadHTMLFile(): Argument #1 ($filename) must not be empty +DOMDocument::loadHTMLFile(): Argument #1 ($filename) must not contain any null bytes diff --git a/ext/dom/tests/DOMDocument_loadXML_error6.phpt b/ext/dom/tests/DOMDocument_loadXML_error6.phpt new file mode 100644 index 0000000000..9f22692ed9 --- /dev/null +++ b/ext/dom/tests/DOMDocument_loadXML_error6.phpt @@ -0,0 +1,16 @@ +--TEST-- +Test DOMDocument::loadXML() with empty file path +--SKIPIF-- + +--FILE-- +loadXML(""); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} +?> +--EXPECT-- +DOMDocument::loadXML(): Argument #1 ($source) must not be empty diff --git a/ext/dom/tests/DOMDocument_load_error6.phpt b/ext/dom/tests/DOMDocument_load_error6.phpt new file mode 100644 index 0000000000..70e270651c --- /dev/null +++ b/ext/dom/tests/DOMDocument_load_error6.phpt @@ -0,0 +1,27 @@ +--TEST-- +Test DOMDocument::load() with invalid paths +--SKIPIF-- + +--FILE-- +load(""); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} + +try { + $dom->load("/path/with/\0/byte"); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} + +// Path is too long +var_dump($dom->load(str_repeat(" ", PHP_MAXPATHLEN + 1))); +?> +--EXPECT-- +DOMDocument::load(): Argument #1 ($filename) must not be empty +Path to document must not contain any null bytes +bool(false) diff --git a/ext/dom/tests/DOMDocument_saveHTMLFile_invalid_filename.phpt b/ext/dom/tests/DOMDocument_saveHTMLFile_invalid_filename.phpt index 6ca66730eb..12b45a491b 100644 --- a/ext/dom/tests/DOMDocument_saveHTMLFile_invalid_filename.phpt +++ b/ext/dom/tests/DOMDocument_saveHTMLFile_invalid_filename.phpt @@ -19,7 +19,11 @@ $title = $doc->createElement('title'); $title = $head->appendChild($title); $text = $doc->createTextNode('This is the title'); $text = $title->appendChild($text); -$bytes = $doc->saveHTMLFile($filename); +try { + $doc->saveHTMLFile($filename); +} catch (ValueError $exception) { + echo $exception->getMessage() . "\n"; +} ?> ---EXPECTF-- -Warning: DOMDocument::saveHTMLFile(): Invalid Filename in %s on line %d +--EXPECT-- +DOMDocument::saveHTMLFile(): Argument #1 ($filename) must not be empty diff --git a/ext/dom/tests/DOMDocument_schemaValidateSource_error3.phpt b/ext/dom/tests/DOMDocument_schemaValidateSource_error3.phpt index 41f9015b92..dc7a1db901 100644 --- a/ext/dom/tests/DOMDocument_schemaValidateSource_error3.phpt +++ b/ext/dom/tests/DOMDocument_schemaValidateSource_error3.phpt @@ -12,10 +12,12 @@ $doc = new DOMDocument; $doc->load(__DIR__."/book.xml"); -$result = $doc->schemaValidateSource(''); -var_dump($result); +try { + $doc->schemaValidateSource(''); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} ?> ---EXPECTF-- -Warning: DOMDocument::schemaValidateSource(): Invalid Schema source in %s.php on line %d -bool(false) +--EXPECT-- +DOMDocument::schemaValidateSource(): Argument #1 ($source) must not be empty diff --git a/ext/dom/tests/DOMDocument_schemaValidate_error3.phpt b/ext/dom/tests/DOMDocument_schemaValidate_error3.phpt index 2ff2ce9487..0add6b2442 100644 --- a/ext/dom/tests/DOMDocument_schemaValidate_error3.phpt +++ b/ext/dom/tests/DOMDocument_schemaValidate_error3.phpt @@ -12,10 +12,12 @@ $doc = new DOMDocument; $doc->load(__DIR__."/book.xml"); -$result = $doc->schemaValidate(''); -var_dump($result); +try { + $doc->schemaValidate(''); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} ?> ---EXPECTF-- -Warning: DOMDocument::schemaValidate(): Invalid Schema source in %s.php on line %d -bool(false) +--EXPECT-- +DOMDocument::schemaValidate(): Argument #1 ($filename) must not be empty diff --git a/ext/dom/tests/DOMDocument_schemaValidate_error6.phpt b/ext/dom/tests/DOMDocument_schemaValidate_error6.phpt new file mode 100644 index 0000000000..1dbfb9a2af --- /dev/null +++ b/ext/dom/tests/DOMDocument_schemaValidate_error6.phpt @@ -0,0 +1,25 @@ +--TEST-- +DomDocument::schemaValidate() - invalid path to schema +--SKIPIF-- + +--FILE-- +load(__DIR__."/book.xml"); + +try { + $doc->schemaValidate("/path/with/\0/byte"); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + +var_dump($doc->schemaValidate(str_repeat(" ", PHP_MAXPATHLEN + 1))); + +?> +--EXPECTF-- +DOMDocument::schemaValidate(): Argument #1 ($filename) must not contain any null bytes + +Warning: DOMDocument::schemaValidate(): Invalid Schema file source in %s on line %d +bool(false) diff --git a/ext/fileinfo/fileinfo.c b/ext/fileinfo/fileinfo.c index 1b26736ad0..0a4ee3c34e 100644 --- a/ext/fileinfo/fileinfo.c +++ b/ext/fileinfo/fileinfo.c @@ -444,7 +444,7 @@ static void _php_finfo_get_type(INTERNAL_FUNCTION_PARAMETERS, int mode, int mime goto clean; } if (CHECK_NULL_PATH(buffer, buffer_len)) { - zend_argument_type_error(1, "must not contain null bytes"); + zend_argument_type_error(1, "must not contain any null bytes"); goto clean; } diff --git a/ext/fileinfo/tests/finfo_file_001.phpt b/ext/fileinfo/tests/finfo_file_001.phpt index 726103e02d..71d88bbd43 100644 --- a/ext/fileinfo/tests/finfo_file_001.phpt +++ b/ext/fileinfo/tests/finfo_file_001.phpt @@ -26,7 +26,7 @@ var_dump(finfo_file($fp, '&')); ?> --EXPECTF-- -finfo_file(): Argument #1 ($finfo) must not contain null bytes +finfo_file(): Argument #1 ($finfo) must not contain any null bytes finfo_file(): Argument #1 ($finfo) cannot be empty finfo_file(): Argument #1 ($finfo) cannot be empty string(9) "directory" diff --git a/ext/fileinfo/tests/finfo_file_basic.phpt b/ext/fileinfo/tests/finfo_file_basic.phpt index 08c296ac8b..821405b48c 100644 --- a/ext/fileinfo/tests/finfo_file_basic.phpt +++ b/ext/fileinfo/tests/finfo_file_basic.phpt @@ -25,4 +25,4 @@ try { string(28) "text/x-php; charset=us-ascii" string(22) "PHP script, ASCII text" string(25) "text/plain; charset=utf-8" -finfo_file(): Argument #1 ($finfo) must not contain null bytes +finfo_file(): Argument #1 ($finfo) must not contain any null bytes diff --git a/ext/fileinfo/tests/mime_content_type_001.phpt b/ext/fileinfo/tests/mime_content_type_001.phpt index 9e3cb60398..76897cca7f 100644 --- a/ext/fileinfo/tests/mime_content_type_001.phpt +++ b/ext/fileinfo/tests/mime_content_type_001.phpt @@ -48,4 +48,4 @@ mime_content_type(): Argument #2 must be of type resource|string, array given Warning: mime_content_type(foo/inexistent): Failed to open stream: No such file or directory in %s on line %d mime_content_type(): Argument #1 ($filename) cannot be empty -mime_content_type(): Argument #1 ($filename) must not contain null bytes +mime_content_type(): Argument #1 ($filename) must not contain any null bytes diff --git a/ext/sqlite3/sqlite3.c b/ext/sqlite3/sqlite3.c index 464df5ae21..cd91e68fd3 100644 --- a/ext/sqlite3/sqlite3.c +++ b/ext/sqlite3/sqlite3.c @@ -1243,12 +1243,7 @@ PHP_METHOD(SQLite3, openBlob) db_obj = Z_SQLITE3_DB_P(object); - if (zend_parse_parameters(ZEND_NUM_ARGS(), "ssl|sl", &table, &table_len, &column, &column_len, &rowid, &dbname, &dbname_len, &flags) == FAILURE) { - RETURN_THROWS(); - } - - if (ZEND_NUM_ARGS() >= 4 && CHECK_NULL_PATH(dbname, dbname_len)) { - zend_argument_type_error(4, "must not contain null bytes"); + if (zend_parse_parameters(ZEND_NUM_ARGS(), "ssl|pl", &table, &table_len, &column, &column_len, &rowid, &dbname, &dbname_len, &flags) == FAILURE) { RETURN_THROWS(); } @@ -1346,17 +1341,7 @@ PHP_METHOD(SQLite3, backup) sqlite3_backup *dbBackup; int rc; // Return code - if (zend_parse_parameters(ZEND_NUM_ARGS(), "O|ss", &destination_zval, php_sqlite3_sc_entry, &source_dbname, &source_dbname_length, &destination_dbname, &destination_dbname_length) == FAILURE) { - RETURN_THROWS(); - } - - if (ZEND_NUM_ARGS() >= 2 && CHECK_NULL_PATH(source_dbname, source_dbname_length)) { - zend_argument_type_error(2, "must not contain null bytes"); - RETURN_THROWS(); - } - - if (ZEND_NUM_ARGS() >= 3 && CHECK_NULL_PATH(destination_dbname, destination_dbname_length)) { - zend_argument_type_error(3, "must not contain null bytes"); + if (zend_parse_parameters(ZEND_NUM_ARGS(), "O|pp", &destination_zval, php_sqlite3_sc_entry, &source_dbname, &source_dbname_length, &destination_dbname, &destination_dbname_length) == FAILURE) { RETURN_THROWS(); } -- 2.50.1