From a958f8e365923b11cebd4a03da261ab30deee96a Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 16 Mar 2024 17:08:54 +0100 Subject: [PATCH 1/3] Implement request #71571: XSLT processor should provide option to change maxDepth There are two depth limiting parameters for XSLT templates. 1) maxTemplateDepth This corresponds to the recursion depth of a template. For very complicated templates this can be hit. 2) maxTemplateVars This is the total number of live variables. When using recursive templates with lots of parameters you can hit this limit. This patch introduces two new properties to XSLTProcessor that corresponds to the above variables. --- NEWS | 2 + UPGRADING | 2 + ext/dom/xpath_callbacks.c | 5 +- ext/xsl/php_xsl.c | 60 +++++++++++++++++++++ ext/xsl/php_xsl.h | 3 ++ ext/xsl/php_xsl.stub.php | 4 ++ ext/xsl/php_xsl_arginfo.h | 14 ++++- ext/xsl/tests/bug71571_a.phpt | 48 +++++++++++++++++ ext/xsl/tests/bug71571_b.phpt | 61 ++++++++++++++++++++++ ext/xsl/tests/maxTemplateDepth_errors.phpt | 44 ++++++++++++++++ ext/xsl/tests/maxTemplateVars_errors.phpt | 44 ++++++++++++++++ ext/xsl/tests/new_without_constructor.phpt | 23 ++++++++ ext/xsl/xsltprocessor.c | 8 +++ 13 files changed, 315 insertions(+), 3 deletions(-) create mode 100644 ext/xsl/tests/bug71571_a.phpt create mode 100644 ext/xsl/tests/bug71571_b.phpt create mode 100644 ext/xsl/tests/maxTemplateDepth_errors.phpt create mode 100644 ext/xsl/tests/maxTemplateVars_errors.phpt create mode 100644 ext/xsl/tests/new_without_constructor.phpt diff --git a/NEWS b/NEWS index bf8838e9ea4a..6b0e1a603315 100644 --- a/NEWS +++ b/NEWS @@ -224,5 +224,7 @@ PHP NEWS . Implement request #64137 (XSLTProcessor::setParameter() should allow both quotes to be used). (nielsdos) . Implemented "Improve callbacks in ext/dom and ext/xsl" RFC. (nielsdos) + . Added XSLTProcessor::$maxTemplateDepth and XSLTProcessor::$maxTemplateVars. + (nielsdos) <<< NOTE: Insert NEWS from last stable release here prior to actual release! >>> diff --git a/UPGRADING b/UPGRADING index 9996047cae2d..229d63080f49 100644 --- a/UPGRADING +++ b/UPGRADING @@ -259,6 +259,8 @@ PHP 8.4 UPGRADE NOTES quotes. . It is now possible to pass any callable to registerPhpFunctions(). RFC: https://wiki.php.net/rfc/improve_callbacks_dom_and_xsl + . Added XSLTProcessor::$maxTemplateDepth and XSLTProcessor::$maxTemplateVars + to control the recursion depth of XSL template evaluation. ======================================== 3. Changes in SAPI modules diff --git a/ext/dom/xpath_callbacks.c b/ext/dom/xpath_callbacks.c index a4dbc4e3f8c8..3c67534514c0 100644 --- a/ext/dom/xpath_callbacks.c +++ b/ext/dom/xpath_callbacks.c @@ -67,8 +67,9 @@ PHP_DOM_EXPORT void php_dom_xpath_callbacks_clean_argument_stack(xmlXPathParserC xmlXPathFreeObject(obj); } - /* Push sentinel value */ - valuePush(ctxt, xmlXPathNewString((const xmlChar *) "")); + /* Don't push a sentinel value here. If this is called from an error situation, then by *not* pushing a sentinel + * the execution will halt. If this is called from a regular situation, then it is the caller's responsibility + * to ensure the stack remains balanced. */ } PHP_DOM_EXPORT void php_dom_xpath_callbacks_dtor(php_dom_xpath_callbacks *registry) diff --git a/ext/xsl/php_xsl.c b/ext/xsl/php_xsl.c index 5beccee2e62a..277eab204066 100644 --- a/ext/xsl/php_xsl.c +++ b/ext/xsl/php_xsl.c @@ -118,10 +118,69 @@ zend_object *xsl_objects_new(zend_class_entry *class_type) intern->parameter = zend_new_array(0); php_dom_xpath_callbacks_ctor(&intern->xpath_callbacks); + /* Default initialize properties that could not be default initialized at the stub because they depend on library + * configuration parameters. */ + ZVAL_LONG(xsl_prop_max_template_depth(&intern->std), xsltMaxDepth); + ZVAL_LONG(xsl_prop_max_template_vars(&intern->std), xsltMaxVars); + return &intern->std; } /* }}} */ +#if ZEND_DEBUG +# define XSL_DEFINE_PROP_ACCESSOR(c_name, php_name, prop_index) \ + zval *xsl_prop_##c_name(zend_object *object) \ + { \ + zend_string *prop_name = ZSTR_INIT_LITERAL(php_name, false); \ + const zend_property_info *prop_info = zend_get_property_info(xsl_xsltprocessor_class_entry, prop_name, 0); \ + zend_string_release_ex(prop_name, false); \ + ZEND_ASSERT(OBJ_PROP_TO_NUM(prop_info->offset) == prop_index); \ + return OBJ_PROP_NUM(object, prop_index); \ + } +#else +# define XSL_DEFINE_PROP_ACCESSOR(c_name, php_name, prop_index) \ + zval *xsl_prop_##c_name(zend_object *object) \ + { \ + return OBJ_PROP_NUM(object, prop_index); \ + } +#endif + +XSL_DEFINE_PROP_ACCESSOR(max_template_depth, "maxTemplateDepth", 2) +XSL_DEFINE_PROP_ACCESSOR(max_template_vars, "maxTemplateVars", 3) + +static zval *xsl_objects_write_property_with_validation(zend_object *object, zend_string *member, zval *value, void **cache_slot, zval *property) +{ + /* Read old value so we can restore it if necessary. The value is not refcounted as its type is IS_LONG. */ + ZEND_ASSERT(Z_TYPE_P(property) == IS_LONG); + zend_long old_property_value = Z_LVAL_P(property); + + /* Write new property, which will also potentially perform coercions. */ + zend_std_write_property(object, member, value, cache_slot); + + /* Validate value *after* coercions have been performed, and restore the old value if necessary. */ + if (UNEXPECTED(Z_LVAL_P(property) < 0)) { + Z_LVAL_P(property) = old_property_value; + zend_value_error("%s::$%s must be greater than or equal to 0", ZSTR_VAL(object->ce->name), ZSTR_VAL(member)); + return &EG(error_zval); + } + + return property; +} + +static zval *xsl_objects_write_property(zend_object *object, zend_string *member, zval *value, void **cache_slot) +{ + /* Extra validation for maxTemplateDepth and maxTemplateVars */ + if (zend_string_equals_literal(member, "maxTemplateDepth")) { + zval *property = xsl_prop_max_template_depth(object); + return xsl_objects_write_property_with_validation(object, member, value, cache_slot, property); + } else if (zend_string_equals_literal(member, "maxTemplateVars")) { + zval *property = xsl_prop_max_template_vars(object); + return xsl_objects_write_property_with_validation(object, member, value, cache_slot, property); + } else { + return zend_std_write_property(object, member, value, cache_slot); + } +} + /* {{{ PHP_MINIT_FUNCTION */ PHP_MINIT_FUNCTION(xsl) { @@ -130,6 +189,7 @@ PHP_MINIT_FUNCTION(xsl) xsl_object_handlers.clone_obj = NULL; xsl_object_handlers.free_obj = xsl_objects_free_storage; xsl_object_handlers.get_gc = xsl_objects_get_gc; + xsl_object_handlers.write_property = xsl_objects_write_property; xsl_xsltprocessor_class_entry = register_class_XSLTProcessor(); xsl_xsltprocessor_class_entry->create_object = xsl_objects_new; diff --git a/ext/xsl/php_xsl.h b/ext/xsl/php_xsl.h index accdecf46bdb..8590b7ad9222 100644 --- a/ext/xsl/php_xsl.h +++ b/ext/xsl/php_xsl.h @@ -77,6 +77,9 @@ void xsl_objects_free_storage(zend_object *object); void xsl_ext_function_string_php(xmlXPathParserContextPtr ctxt, int nargs); void xsl_ext_function_object_php(xmlXPathParserContextPtr ctxt, int nargs); +zval *xsl_prop_max_template_depth(zend_object *object); +zval *xsl_prop_max_template_vars(zend_object *object); + PHP_MINIT_FUNCTION(xsl); PHP_MSHUTDOWN_FUNCTION(xsl); PHP_RINIT_FUNCTION(xsl); diff --git a/ext/xsl/php_xsl.stub.php b/ext/xsl/php_xsl.stub.php index fffcc5dfc93c..c01e218162ee 100644 --- a/ext/xsl/php_xsl.stub.php +++ b/ext/xsl/php_xsl.stub.php @@ -75,6 +75,10 @@ class XSLTProcessor public bool $cloneDocument = false; + public int $maxTemplateDepth; + + public int $maxTemplateVars; + /** * @param DOMDocument|DOM\Document|SimpleXMLElement $stylesheet * @tentative-return-type diff --git a/ext/xsl/php_xsl_arginfo.h b/ext/xsl/php_xsl_arginfo.h index 2d8427c95f84..033e1e8e3b21 100644 --- a/ext/xsl/php_xsl_arginfo.h +++ b/ext/xsl/php_xsl_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: ae093276a1caf53c34d429076458bc8b802d69e9 */ + * Stub hash: f7f9951cbb437f1985016dc06490d55f711bc725 */ ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_INFO_EX(arginfo_class_XSLTProcessor_importStylesheet, 0, 1, _IS_BOOL, 0) ZEND_ARG_TYPE_INFO(0, stylesheet, IS_OBJECT, 0) @@ -131,5 +131,17 @@ static zend_class_entry *register_class_XSLTProcessor(void) zend_declare_typed_property(class_entry, property_cloneDocument_name, &property_cloneDocument_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_BOOL)); zend_string_release(property_cloneDocument_name); + zval property_maxTemplateDepth_default_value; + ZVAL_UNDEF(&property_maxTemplateDepth_default_value); + zend_string *property_maxTemplateDepth_name = zend_string_init("maxTemplateDepth", sizeof("maxTemplateDepth") - 1, 1); + zend_declare_typed_property(class_entry, property_maxTemplateDepth_name, &property_maxTemplateDepth_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_LONG)); + zend_string_release(property_maxTemplateDepth_name); + + zval property_maxTemplateVars_default_value; + ZVAL_UNDEF(&property_maxTemplateVars_default_value); + zend_string *property_maxTemplateVars_name = zend_string_init("maxTemplateVars", sizeof("maxTemplateVars") - 1, 1); + zend_declare_typed_property(class_entry, property_maxTemplateVars_name, &property_maxTemplateVars_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_LONG)); + zend_string_release(property_maxTemplateVars_name); + return class_entry; } diff --git a/ext/xsl/tests/bug71571_a.phpt b/ext/xsl/tests/bug71571_a.phpt new file mode 100644 index 000000000000..7f16b77a16f3 --- /dev/null +++ b/ext/xsl/tests/bug71571_a.phpt @@ -0,0 +1,48 @@ +--TEST-- +Request #71571 (XSLT processor should provide option to change maxDepth) - variant A +--EXTENSIONS-- +xsl +--INI-- +error_reporting=E_ALL +--FILE-- + + + + + + + + + + +EOF; + +$xsl = new DOMDocument(); +$xsl->loadXML($myxsl); + +$doc = new DOMDocument(); + +$proc = new XSLTProcessor; +$proc->maxTemplateDepth = 2; +$proc->importStyleSheet($xsl); +$proc->transformToDoc($doc); + +?> +--EXPECTF-- +Warning: XSLTProcessor::transformToDoc(): runtime error: file %s line 8 element call-template in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): xsltApplySequenceConstructor: A potential infinite template recursion was detected. +You can adjust xsltMaxDepth (--maxdepth) in order to raise the maximum number of nested template calls and variables/params (currently set to 2). in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): Templates: in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #0 name recurse in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #1 name recurse in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #2 name / in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): Variables: in %s on line %d diff --git a/ext/xsl/tests/bug71571_b.phpt b/ext/xsl/tests/bug71571_b.phpt new file mode 100644 index 000000000000..3b8358e7d41d --- /dev/null +++ b/ext/xsl/tests/bug71571_b.phpt @@ -0,0 +1,61 @@ +--TEST-- +Request #71571 (XSLT processor should provide option to change maxDepth) - variant B +--EXTENSIONS-- +xsl +--INI-- +error_reporting=E_ALL +--FILE-- + + + + + + + + 1 + + + + + +EOF; + +$xsl = new DOMDocument(); +$xsl->loadXML($myxsl); + +$doc = new DOMDocument(); + +$proc = new XSLTProcessor; +// Set the template depth limit so high that we will certainly hit the variable depth limit first. +$proc->maxTemplateDepth = 2**30; +$proc->maxTemplateVars = 2; +$proc->importStyleSheet($xsl); +$proc->transformToDoc($doc); + +?> +--EXPECTF-- +Warning: XSLTProcessor::transformToDoc(): runtime error: file %s line 8 element param in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): xsltApplyXSLTTemplate: A potential infinite template recursion was detected. +You can adjust maxTemplateVars (--maxvars) in order to raise the maximum number of variables/params (currently set to 2). in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): Templates: in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #0 name recurse in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #1 name recurse in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #2 name / in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): Variables: in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #0 in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): COUNT in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): #1 in %s on line %d + +Warning: XSLTProcessor::transformToDoc(): param COUNT in %s on line %d diff --git a/ext/xsl/tests/maxTemplateDepth_errors.phpt b/ext/xsl/tests/maxTemplateDepth_errors.phpt new file mode 100644 index 000000000000..ec70927ea9c2 --- /dev/null +++ b/ext/xsl/tests/maxTemplateDepth_errors.phpt @@ -0,0 +1,44 @@ +--TEST-- +XSLTProcessor::$maxTemplateDepth errors +--EXTENSIONS-- +xsl +--INI-- +error_reporting=E_ALL & ~E_DEPRECATED +--FILE-- +maxTemplateDepth; + +try { + $processor->maxTemplateDepth = -1; +} catch (ValueError $e) { + echo $e->getMessage(), "\n"; +} + +var_dump($processor->maxTemplateDepth === $oldValue); + +try { + $processor->maxTemplateDepth = -32.1; +} catch (ValueError $e) { + echo $e->getMessage(), "\n"; +} + +var_dump($processor->maxTemplateDepth === $oldValue); + +try { + $processor->maxTemplateDepth = "-1"; +} catch (ValueError $e) { + echo $e->getMessage(), "\n"; +} + +var_dump($processor->maxTemplateDepth === $oldValue); + +?> +--EXPECT-- +XSLTProcessor::$maxTemplateDepth must be greater than or equal to 0 +bool(true) +XSLTProcessor::$maxTemplateDepth must be greater than or equal to 0 +bool(true) +XSLTProcessor::$maxTemplateDepth must be greater than or equal to 0 +bool(true) diff --git a/ext/xsl/tests/maxTemplateVars_errors.phpt b/ext/xsl/tests/maxTemplateVars_errors.phpt new file mode 100644 index 000000000000..7ad9c7241e74 --- /dev/null +++ b/ext/xsl/tests/maxTemplateVars_errors.phpt @@ -0,0 +1,44 @@ +--TEST-- +XSLTProcessor::$maxTemplateVars errors +--EXTENSIONS-- +xsl +--INI-- +error_reporting=E_ALL & ~E_DEPRECATED +--FILE-- +maxTemplateVars; + +try { + $processor->maxTemplateVars = -1; +} catch (ValueError $e) { + echo $e->getMessage(), "\n"; +} + +var_dump($processor->maxTemplateVars === $oldValue); + +try { + $processor->maxTemplateVars = -32.1; +} catch (ValueError $e) { + echo $e->getMessage(), "\n"; +} + +var_dump($processor->maxTemplateVars === $oldValue); + +try { + $processor->maxTemplateVars = "-1"; +} catch (ValueError $e) { + echo $e->getMessage(), "\n"; +} + +var_dump($processor->maxTemplateVars === $oldValue); + +?> +--EXPECT-- +XSLTProcessor::$maxTemplateVars must be greater than or equal to 0 +bool(true) +XSLTProcessor::$maxTemplateVars must be greater than or equal to 0 +bool(true) +XSLTProcessor::$maxTemplateVars must be greater than or equal to 0 +bool(true) diff --git a/ext/xsl/tests/new_without_constructor.phpt b/ext/xsl/tests/new_without_constructor.phpt new file mode 100644 index 000000000000..aa1de6275868 --- /dev/null +++ b/ext/xsl/tests/new_without_constructor.phpt @@ -0,0 +1,23 @@ +--TEST-- +XSLTProcessor: new instance without constructor +--EXTENSIONS-- +xsl +--FILE-- +newInstanceWithoutConstructor(); +var_dump($processor); + +?> +--EXPECTF-- +object(XSLTProcessor)#2 (4) { + ["doXInclude"]=> + bool(false) + ["cloneDocument"]=> + bool(false) + ["maxTemplateDepth"]=> + int(%d) + ["maxTemplateVars"]=> + int(%d) +} diff --git a/ext/xsl/xsltprocessor.c b/ext/xsl/xsltprocessor.c index e7a04e673a25..c4f577958530 100644 --- a/ext/xsl/xsltprocessor.c +++ b/ext/xsl/xsltprocessor.c @@ -360,6 +360,14 @@ static xmlDocPtr php_xsl_apply_stylesheet(zval *id, xsl_object *intern, xsltStyl ctxt->xinclude = zend_is_true(doXInclude); zend_string_release_ex(member, 0); + zval *max_template_depth = xsl_prop_max_template_depth(Z_OBJ_P(id)); + ZEND_ASSERT(Z_TYPE_P(max_template_depth) == IS_LONG); + ctxt->maxTemplateDepth = Z_LVAL_P(max_template_depth); + + zval *max_template_vars = xsl_prop_max_template_vars(Z_OBJ_P(id)); + ZEND_ASSERT(Z_TYPE_P(max_template_vars) == IS_LONG); + ctxt->maxTemplateVars = Z_LVAL_P(max_template_vars); + secPrefsValue = intern->securityPrefs; /* if securityPrefs is set to NONE, we don't have to do any checks, but otherwise... */ From ce1058703d7198a81eda492e1e84b56bcdb18637 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 18 Mar 2024 20:40:48 +0100 Subject: [PATCH 2/3] Improve error message --- UPGRADING.INTERNALS | 2 ++ ext/libxml/libxml.c | 17 +++++------- ext/libxml/php_libxml.h | 7 +++++ ext/xsl/php_xsl.c | 50 ++++++++++++++++++++++++++++++++++- ext/xsl/tests/bug71571_a.phpt | 2 +- ext/xsl/tests/bug71571_b.phpt | 2 +- 6 files changed, 67 insertions(+), 13 deletions(-) diff --git a/UPGRADING.INTERNALS b/UPGRADING.INTERNALS index e0f0ac46f813..a656d0232fe9 100644 --- a/UPGRADING.INTERNALS +++ b/UPGRADING.INTERNALS @@ -182,6 +182,8 @@ PHP 8.4 INTERNALS UPGRADE NOTES d. ext/libxml - Added php_libxml_pretend_ctx_error_ex() to emit errors as if they had come from libxml. + - Added php_libxml_error_handler_va() to pass libxml errors, and + corresponding php_libxml_error_level enum. - Removed the "properties" HashTable field from php_libxml_node_object. - Added a way to attached private data to a php_libxml_ref_obj. - Added a way to fix a class type onto php_libxml_ref_obj. diff --git a/ext/libxml/libxml.c b/ext/libxml/libxml.c index 15f6e0cb830a..096f07e45f7d 100644 --- a/ext/libxml/libxml.c +++ b/ext/libxml/libxml.c @@ -45,9 +45,6 @@ #include "php_libxml.h" #define PHP_LIBXML_LOADED_VERSION ((char *)xmlParserVersion) -#define PHP_LIBXML_ERROR 0 -#define PHP_LIBXML_CTX_ERROR 1 -#define PHP_LIBXML_CTX_WARNING 2 #include "libxml_arginfo.h" @@ -647,12 +644,12 @@ void php_libxml_issue_error(int level, const char *msg) } } -static void php_libxml_internal_error_handler_ex(int error_type, void *ctx, const char **msg, va_list ap, int line, int column) +static void php_libxml_internal_error_handler_ex(php_libxml_error_level error_type, void *ctx, const char *msg, va_list ap, int line, int column) { char *buf; int len, len_iter, output = 0; - len = vspprintf(&buf, 0, *msg, ap); + len = vspprintf(&buf, 0, msg, ap); len_iter = len; /* remove any trailing \n */ @@ -685,7 +682,7 @@ static void php_libxml_internal_error_handler_ex(int error_type, void *ctx, cons } } -static void php_libxml_internal_error_handler(int error_type, void *ctx, const char **msg, va_list ap) +PHP_LIBXML_API void php_libxml_error_handler_va(php_libxml_error_level error_type, void *ctx, const char *msg, va_list ap) { int line = 0; int column = 0; @@ -831,7 +828,7 @@ PHP_LIBXML_API void php_libxml_pretend_ctx_error_ex(const char *file, int line, { va_list args; va_start(args, msg); - php_libxml_internal_error_handler_ex(PHP_LIBXML_CTX_ERROR, NULL, &msg, args, line, column); + php_libxml_internal_error_handler_ex(PHP_LIBXML_CTX_ERROR, NULL, msg, args, line, column); va_end(args); /* Propagate back into libxml */ @@ -853,7 +850,7 @@ PHP_LIBXML_API void php_libxml_ctx_error(void *ctx, const char *msg, ...) { va_list args; va_start(args, msg); - php_libxml_internal_error_handler(PHP_LIBXML_CTX_ERROR, ctx, &msg, args); + php_libxml_error_handler_va(PHP_LIBXML_CTX_ERROR, ctx, msg, args); va_end(args); } @@ -861,7 +858,7 @@ PHP_LIBXML_API void php_libxml_ctx_warning(void *ctx, const char *msg, ...) { va_list args; va_start(args, msg); - php_libxml_internal_error_handler(PHP_LIBXML_CTX_WARNING, ctx, &msg, args); + php_libxml_error_handler_va(PHP_LIBXML_CTX_WARNING, ctx, msg, args); va_end(args); } @@ -878,7 +875,7 @@ PHP_LIBXML_API void php_libxml_error_handler(void *ctx, const char *msg, ...) { va_list args; va_start(args, msg); - php_libxml_internal_error_handler(PHP_LIBXML_ERROR, ctx, &msg, args); + php_libxml_error_handler_va(PHP_LIBXML_ERROR, ctx, msg, args); va_end(args); } diff --git a/ext/libxml/php_libxml.h b/ext/libxml/php_libxml.h index 9385c1848a92..cd34678bc94b 100644 --- a/ext/libxml/php_libxml.h +++ b/ext/libxml/php_libxml.h @@ -141,6 +141,12 @@ static zend_always_inline void php_libxml_invalidate_node_list_cache_from_doc(xm typedef void * (*php_libxml_export_node) (zval *object); +typedef enum { + PHP_LIBXML_ERROR = 0, + PHP_LIBXML_CTX_ERROR = 1, + PHP_LIBXML_CTX_WARNING = 2, +} php_libxml_error_level; + PHP_LIBXML_API int php_libxml_increment_node_ptr(php_libxml_node_object *object, xmlNodePtr node, void *private_data); PHP_LIBXML_API int php_libxml_decrement_node_ptr(php_libxml_node_object *object); PHP_LIBXML_API int php_libxml_increment_doc_ref(php_libxml_node_object *object, xmlDocPtr docp); @@ -157,6 +163,7 @@ PHP_LIBXML_API void php_libxml_error_handler(void *ctx, const char *msg, ...); PHP_LIBXML_API void php_libxml_ctx_warning(void *ctx, const char *msg, ...); PHP_LIBXML_API void php_libxml_pretend_ctx_error_ex(const char *file, int line, int column, const char *msg,...); PHP_LIBXML_API void php_libxml_ctx_error(void *ctx, const char *msg, ...); +PHP_LIBXML_API void php_libxml_error_handler_va(php_libxml_error_level error_type, void *ctx, const char *msg, va_list args); PHP_LIBXML_API int php_libxml_xmlCheckUTF8(const unsigned char *s); PHP_LIBXML_API void php_libxml_switch_context(zval *context, zval *oldcontext); PHP_LIBXML_API void php_libxml_issue_error(int level, const char *msg); diff --git a/ext/xsl/php_xsl.c b/ext/xsl/php_xsl.c index 277eab204066..8d1ce159cc3f 100644 --- a/ext/xsl/php_xsl.c +++ b/ext/xsl/php_xsl.c @@ -181,6 +181,54 @@ static zval *xsl_objects_write_property(zend_object *object, zend_string *member } } +/* Tries to output an error message where a part was replaced by another string. + * Returns true if the search string was found and the error message with replacement was outputted. + * Return false otherwise. */ +static bool xsl_try_output_replaced_error_message( + void *ctx, + const char *msg, + va_list args, + const char *search, + size_t search_len, + const char *replace +) +{ + const char *msg_replace_location = strstr(msg, search); + if (msg_replace_location != NULL) { + php_libxml_ctx_error(ctx, "%.*s%s%s", (int) (msg_replace_location - msg), msg, replace, msg_replace_location + search_len); + return true; + } + return false; +} + +/* Helper macro so the string length doesn't need to be passed separately. + * Only allows literal strings for `search` and `replace`. */ +#define XSL_TRY_OUTPUT_REPLACED_ERROR_MESSAGE(ctx, msg, args, search, replace) \ + xsl_try_output_replaced_error_message(ctx, msg, args, "" search, sizeof("" search) - 1, "" replace) + +/* We want to output PHP-tailored error messages for some libxslt error messages, such that + * the errors refer to PHP properties instead of libxslt-specific fields. */ +static void xsl_libxslt_error_handler(void *ctx, const char *msg, ...) +{ + va_list args; + va_start(args, msg); + + if (strcmp(msg, "%s") == 0) { + /* Adjust error message to be more descriptive */ + const char *msg_arg = va_arg(args, const char *); + bool output = XSL_TRY_OUTPUT_REPLACED_ERROR_MESSAGE(ctx, msg_arg, args, "xsltMaxDepth (--maxdepth)", "$maxTemplateDepth") + || XSL_TRY_OUTPUT_REPLACED_ERROR_MESSAGE(ctx, msg_arg, args, "maxTemplateVars (--maxvars)", "$maxTemplateVars"); + + if (!output) { + php_libxml_ctx_error(ctx, "%s", msg_arg); + } + } else { + php_libxml_error_handler_va(PHP_LIBXML_ERROR, ctx, msg, args); + } + + va_end(args); +} + /* {{{ PHP_MINIT_FUNCTION */ PHP_MINIT_FUNCTION(xsl) { @@ -205,7 +253,7 @@ PHP_MINIT_FUNCTION(xsl) xsltRegisterExtModuleFunction ((const xmlChar *) "function", (const xmlChar *) "http://php.net/xsl", xsl_ext_function_object_php); - xsltSetGenericErrorFunc(NULL, php_libxml_error_handler); + xsltSetGenericErrorFunc(NULL, xsl_libxslt_error_handler); register_php_xsl_symbols(module_number); diff --git a/ext/xsl/tests/bug71571_a.phpt b/ext/xsl/tests/bug71571_a.phpt index 7f16b77a16f3..ee19a9189650 100644 --- a/ext/xsl/tests/bug71571_a.phpt +++ b/ext/xsl/tests/bug71571_a.phpt @@ -35,7 +35,7 @@ $proc->transformToDoc($doc); Warning: XSLTProcessor::transformToDoc(): runtime error: file %s line 8 element call-template in %s on line %d Warning: XSLTProcessor::transformToDoc(): xsltApplySequenceConstructor: A potential infinite template recursion was detected. -You can adjust xsltMaxDepth (--maxdepth) in order to raise the maximum number of nested template calls and variables/params (currently set to 2). in %s on line %d +You can adjust $maxTemplateDepth in order to raise the maximum number of nested template calls and variables/params (currently set to 2). in %s on line %d Warning: XSLTProcessor::transformToDoc(): Templates: in %s on line %d diff --git a/ext/xsl/tests/bug71571_b.phpt b/ext/xsl/tests/bug71571_b.phpt index 3b8358e7d41d..b40076bfd368 100644 --- a/ext/xsl/tests/bug71571_b.phpt +++ b/ext/xsl/tests/bug71571_b.phpt @@ -40,7 +40,7 @@ $proc->transformToDoc($doc); Warning: XSLTProcessor::transformToDoc(): runtime error: file %s line 8 element param in %s on line %d Warning: XSLTProcessor::transformToDoc(): xsltApplyXSLTTemplate: A potential infinite template recursion was detected. -You can adjust maxTemplateVars (--maxvars) in order to raise the maximum number of variables/params (currently set to 2). in %s on line %d +You can adjust $maxTemplateVars in order to raise the maximum number of variables/params (currently set to 2). in %s on line %d Warning: XSLTProcessor::transformToDoc(): Templates: in %s on line %d From b18b1b3e0f3da1724b50ab7c1c9e4bf49ffa4a71 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 18 Mar 2024 22:09:55 +0100 Subject: [PATCH 3/3] Block indirect modifications of validated properties --- ext/xsl/php_xsl.c | 40 ++++++++++++- ...eDepth_modification_validation_bypass.phpt | 57 +++++++++++++++++++ ...teVars_modification_validation_bypass.phpt | 57 +++++++++++++++++++ 3 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 ext/xsl/tests/maxTemplateDepth_modification_validation_bypass.phpt create mode 100644 ext/xsl/tests/maxTemplateVars_modification_validation_bypass.phpt diff --git a/ext/xsl/php_xsl.c b/ext/xsl/php_xsl.c index 8d1ce159cc3f..605a933c286a 100644 --- a/ext/xsl/php_xsl.c +++ b/ext/xsl/php_xsl.c @@ -155,7 +155,7 @@ static zval *xsl_objects_write_property_with_validation(zend_object *object, zen zend_long old_property_value = Z_LVAL_P(property); /* Write new property, which will also potentially perform coercions. */ - zend_std_write_property(object, member, value, cache_slot); + zend_std_write_property(object, member, value, NULL); /* Validate value *after* coercions have been performed, and restore the old value if necessary. */ if (UNEXPECTED(Z_LVAL_P(property) < 0)) { @@ -181,6 +181,41 @@ static zval *xsl_objects_write_property(zend_object *object, zend_string *member } } +static bool xsl_is_validated_property(const zend_string *member) +{ + return zend_string_equals_literal(member, "maxTemplateDepth") || zend_string_equals_literal(member, "maxTemplateVars"); +} + +static zval *xsl_objects_get_property_ptr_ptr(zend_object *object, zend_string *member, int type, void **cache_slot) +{ + if (xsl_is_validated_property(member)) { + return NULL; + } + + return zend_std_get_property_ptr_ptr(object, member, type, cache_slot); +} + +static zval *xsl_objects_read_property(zend_object *object, zend_string *member, int type, void **cache_slot, zval *rv) +{ + /* read handler is being called as a fallback after get_property_ptr_ptr returned NULL */ + if (type != BP_VAR_IS && type != BP_VAR_R && xsl_is_validated_property(member)) { + zend_throw_error(NULL, "Indirect modification of %s::$%s is not allowed", ZSTR_VAL(object->ce->name), ZSTR_VAL(member)); + return &EG(uninitialized_zval); + } + + return zend_std_read_property(object, member, type, cache_slot, rv); +} + +static void xsl_objects_unset_property(zend_object *object, zend_string *member, void **cache_slot) +{ + if (xsl_is_validated_property(member)) { + zend_throw_error(NULL, "Cannot unset %s::$%s", ZSTR_VAL(object->ce->name), ZSTR_VAL(member)); + return; + } + + zend_std_unset_property(object, member, cache_slot); +} + /* Tries to output an error message where a part was replaced by another string. * Returns true if the search string was found and the error message with replacement was outputted. * Return false otherwise. */ @@ -238,6 +273,9 @@ PHP_MINIT_FUNCTION(xsl) xsl_object_handlers.free_obj = xsl_objects_free_storage; xsl_object_handlers.get_gc = xsl_objects_get_gc; xsl_object_handlers.write_property = xsl_objects_write_property; + xsl_object_handlers.get_property_ptr_ptr = xsl_objects_get_property_ptr_ptr; + xsl_object_handlers.read_property = xsl_objects_read_property; + xsl_object_handlers.unset_property = xsl_objects_unset_property; xsl_xsltprocessor_class_entry = register_class_XSLTProcessor(); xsl_xsltprocessor_class_entry->create_object = xsl_objects_new; diff --git a/ext/xsl/tests/maxTemplateDepth_modification_validation_bypass.phpt b/ext/xsl/tests/maxTemplateDepth_modification_validation_bypass.phpt new file mode 100644 index 000000000000..48305a512bd8 --- /dev/null +++ b/ext/xsl/tests/maxTemplateDepth_modification_validation_bypass.phpt @@ -0,0 +1,57 @@ +--TEST-- +XSLTProcessor::$maxTemplateDepth modification validation bypass +--EXTENSIONS-- +xsl +--FILE-- +maxTemplateDepth = $value; + } catch (ValueError $e) { + echo $e->getMessage(), "\n"; + } +} + +echo "--- Get reference ---\n"; + +try { + $ref =& $proc->maxTemplateDepth; +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +echo "--- Unset ---\n"; + +try { + unset($proc->maxTemplateDepth); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +echo "--- Dump ---\n"; + +var_dump($proc); + +?> +--EXPECT-- +--- Set to 1 --- +--- Set to -1 --- +XSLTProcessor::$maxTemplateDepth must be greater than or equal to 0 +--- Get reference --- +Indirect modification of XSLTProcessor::$maxTemplateDepth is not allowed +--- Unset --- +Cannot unset XSLTProcessor::$maxTemplateDepth +--- Dump --- +object(XSLTProcessor)#1 (4) { + ["doXInclude"]=> + bool(false) + ["cloneDocument"]=> + bool(false) + ["maxTemplateDepth"]=> + int(1) + ["maxTemplateVars"]=> + int(15000) +} diff --git a/ext/xsl/tests/maxTemplateVars_modification_validation_bypass.phpt b/ext/xsl/tests/maxTemplateVars_modification_validation_bypass.phpt new file mode 100644 index 000000000000..d2e52aa6099b --- /dev/null +++ b/ext/xsl/tests/maxTemplateVars_modification_validation_bypass.phpt @@ -0,0 +1,57 @@ +--TEST-- +XSLTProcessor::$maxTemplateVars modification validation bypass +--EXTENSIONS-- +xsl +--FILE-- +maxTemplateVars = $value; + } catch (ValueError $e) { + echo $e->getMessage(), "\n"; + } +} + +echo "--- Get reference ---\n"; + +try { + $ref =& $proc->maxTemplateVars; +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +echo "--- Unset ---\n"; + +try { + unset($proc->maxTemplateVars); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +echo "--- Dump ---\n"; + +var_dump($proc); + +?> +--EXPECT-- +--- Set to 1 --- +--- Set to -1 --- +XSLTProcessor::$maxTemplateVars must be greater than or equal to 0 +--- Get reference --- +Indirect modification of XSLTProcessor::$maxTemplateVars is not allowed +--- Unset --- +Cannot unset XSLTProcessor::$maxTemplateVars +--- Dump --- +object(XSLTProcessor)#1 (4) { + ["doXInclude"]=> + bool(false) + ["cloneDocument"]=> + bool(false) + ["maxTemplateDepth"]=> + int(3000) + ["maxTemplateVars"]=> + int(1) +}