From 3a8a9ded1e5966bb0f20119125c63cb7c3a452ce Mon Sep 17 00:00:00 2001 From: Gustavo Lopes Date: Mon, 5 Feb 2024 18:01:15 +0000 Subject: [PATCH] Address review comments --- .../src/extension/commands/request_shutdown.c | 97 +++++++++++---- appsec/src/extension/ddappsec.c | 2 +- appsec/src/extension/entity_body.c | 28 +---- appsec/src/extension/entity_body.h | 2 +- appsec/src/extension/request_lifecycle.c | 10 +- .../extension/convert_json_max_depth.phpt | 36 ++++++ appsec/tests/extension/convert_xml_basic.phpt | 113 ++++++++---------- .../convert_xml_external_entity.phpt | 8 +- .../rshutdown_command_body_user_request.phpt | 22 ++-- ...down_command_body_user_request_stream.phpt | 22 ++-- .../extension/rshutdown_command_body_xml.phpt | 22 ++-- ...utdown_command_body_xml_explicit_utf8.phpt | 22 ++-- .../appsec/php/integration/CommonTests.groovy | 3 +- .../php/integration/RoadRunnerTests.groovy | 3 +- .../integration/src/test/waf/recommended.json | 3 +- 15 files changed, 213 insertions(+), 180 deletions(-) create mode 100644 appsec/tests/extension/convert_json_max_depth.phpt diff --git a/appsec/src/extension/commands/request_shutdown.c b/appsec/src/extension/commands/request_shutdown.c index 99ecd64e8b0..a7e037e39bc 100644 --- a/appsec/src/extension/commands/request_shutdown.c +++ b/appsec/src/extension/commands/request_shutdown.c @@ -14,8 +14,21 @@ #include "request_shutdown_arginfo.h" #include "zend_exceptions.h" #include +#include #include +#if PHP_VERSION_ID < 70200 +typedef void (*json_decode_ex_t)(zval *return_value, char *str, size_t str_len, + zend_long options, zend_long depth); +#elif PHP_VERSION_ID < 80200 +typedef int (*json_decode_ex_t)(zval *return_value, const char *str, + size_t str_len, zend_long options, zend_long depth); +#else +typedef zend_result (*json_decode_ex_t)(zval *return_value, const char *str, + size_t str_len, zend_long options, zend_long depth); +#endif +static json_decode_ex_t _json_decode_ex; + static dd_result _request_pack(mpack_writer_t *nonnull w, void *nonnull ctx); static void _pack_headers_no_cookies_llist( mpack_writer_t *nonnull w, zend_llist *nonnull hl); @@ -306,7 +319,11 @@ static zval _convert_json(char *nonnull entity, size_t entity_len) { zval zv; ZVAL_NULL(&zv); -#define MAX_DEPTH 12 + if (!_json_decode_ex) { + return zv; + } + +#define MAX_DEPTH 30 php_json_decode_ex( &zv, entity, entity_len, PHP_JSON_OBJECT_AS_ARRAY, MAX_DEPTH); if (Z_TYPE(zv) == IS_NULL) { @@ -345,6 +362,33 @@ static bool _assume_utf8(const char *ct, size_t ct_len) return true; } +static zend_array *_transform_attr_keys(const zval *orig) +{ + // append @ to keys + zend_array *new_arr = zend_new_array(zend_array_count(Z_ARR_P(orig))); + zend_string *key; + zval *val; + ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARR_P(orig), key, val) + { + if (!key) { + // attribute names can't be only numbers anyway + continue; + } + if (Z_TYPE_P(val) != IS_STRING) { + continue; + } + char *new_key = safe_emalloc(ZSTR_LEN(key), 1, 2); + char *wp = new_key; + *wp++ = '@'; + memcpy(wp, ZSTR_VAL(key), ZSTR_LEN(key) + 1); + zval_addref_p(val); + zend_hash_str_add_new(new_arr, new_key, ZSTR_LEN(key) + 1, val); + efree(new_key); + } + ZEND_HASH_FOREACH_END(); + return new_arr; +} + static zval _convert_xml_impl(const char *nonnull entity, size_t entity_len, const char *content_type, size_t content_type_len) { @@ -434,7 +478,7 @@ static zval _convert_xml_impl(const char *nonnull entity, size_t entity_len, // now transform the the result // each tag is encoded as a singleton map: - // : {content: [...], attributes: {...}) + // : [{@attr1: "...", ...}, "text...", {further tags...}]) // text is encoded as string zend_array *root = zend_new_array(1); zend_array *cur = root; // non-owning @@ -489,33 +533,25 @@ static zval _convert_xml_impl(const char *nonnull entity, size_t entity_len, ZVAL_ARR(&celem_zv, celem); zend_hash_next_index_insert(cur, &celem_zv); - // map with keys content and attributes - zend_array *celem_val = zend_new_array(attr_zv ? 2 : 1); + // array with content and attributes + zend_array *celem_val = + zend_new_array(attr_zv ? 2 : 1 /* estimate only */); { zval celem_val_zv; ZVAL_ARR(&celem_val_zv, celem_val); zend_hash_add_new(celem, Z_STR_P(tag_zv), &celem_val_zv); } - zend_array *content = NULL; - if (type == open || value_zv) { - content = zend_new_array(1); - { - zval content_zv; - ZVAL_ARR(&content_zv, content); - zend_hash_str_add_new(celem_val, "content", - sizeof("content") - 1, &content_zv); - } - if (value_zv) { - zval_addref_p(value_zv); - zend_hash_next_index_insert(content, value_zv); - } + if (attr_zv) { + zend_array *new_attr = _transform_attr_keys(attr_zv); + zval new_attr_zv; + ZVAL_ARR(&new_attr_zv, new_attr); + zend_hash_next_index_insert(celem_val, &new_attr_zv); } - if (attr_zv) { - zval_addref_p(attr_zv); - zend_hash_str_add_new( - celem_val, "attributes", sizeof("attributes") - 1, attr_zv); + if (value_zv) { + zval_addref_p(value_zv); + zend_hash_next_index_insert(celem_val, value_zv); } if (type == open) { @@ -523,8 +559,7 @@ static zval _convert_xml_impl(const char *nonnull entity, size_t entity_len, zval cur_zv; ZVAL_ARR(&cur_zv, cur); zend_hash_next_index_insert(stack, &cur_zv); - assert(content != NULL); - cur = content; + cur = celem_val; } } else if (type == cdata) { zval *value_zv = zend_hash_str_find(val, LSTRARG("value")); @@ -609,6 +644,22 @@ PHP_FUNCTION(datadog_appsec_testing_convert_xml) void dd_request_shutdown_startup() { +#if PHP_VERSION_ID < 80000 + void *handle = dlopen(NULL, RTLD_NOW | RTLD_GLOBAL); + if (handle == NULL) { + // NOLINTNEXTLINE(concurrency-mt-unsafe) + mlog(dd_log_error, "Failed load process symbols: %s", dlerror()); + } else { + _json_decode_ex = (json_decode_ex_t)dlsym(handle, "php_json_decode_ex"); + if (!_json_decode_ex) { + mlog(dd_log_warning, "Failed to load php_json_decode_ex: %s", + dlerror()); // NOLINT(concurrency-mt-unsafe) + } + dlclose(handle); + } +#else + _json_decode_ex = php_json_decode_ex; +#endif if (get_global_DD_APPSEC_TESTING()) { dd_phpobj_reg_funcs(ext_functions); } diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index 4ace24fdd9e..28ec93fb6ca 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -166,6 +166,7 @@ static PHP_GINIT_FUNCTION(ddappsec) static PHP_GSHUTDOWN_FUNCTION(ddappsec) { + dd_entity_body_gshutdown(); dd_helper_gshutdown(); // delay log shutdown until the last possible moment, so that TSRM // destructors can run with logging @@ -231,7 +232,6 @@ static PHP_MSHUTDOWN_FUNCTION(ddappsec) // no other thread is running now. reset config to global config only. runtime_config_first_init = false; - dd_entity_body_shutdown(); dd_tags_shutdown(); dd_user_tracking_shutdown(); dd_trace_shutdown(); diff --git a/appsec/src/extension/entity_body.c b/appsec/src/extension/entity_body.c index 8bf2e7b3939..f2b3128f903 100644 --- a/appsec/src/extension/entity_body.c +++ b/appsec/src/extension/entity_body.c @@ -15,10 +15,6 @@ static typeof(zend_write(NULL, 0)) _dd_save_output_zend_write( ZEND_TLS zend_string *_buffer; ZEND_TLS size_t _buffer_size; -// we need to keep track of all buffers so we can free them on shutdown -// this is in order to avoid having memory leaks reported -static HashTable _all_buffers; - #define DEFAULT_MAX_BUF_SIZE (1024 * 512UL) static typeof(zend_write) orig_zend_write; @@ -27,25 +23,14 @@ void dd_entity_body_startup() { orig_zend_write = zend_write; zend_write = _dd_save_output_zend_write; - zend_hash_init(&_all_buffers, 0, NULL, NULL, 1); } -void dd_entity_body_shutdown() +void dd_entity_body_gshutdown() { - zend_write = orig_zend_write; - zend_string *key_s = NULL; - zend_ulong key_i; - ZEND_HASH_FOREACH_KEY(&_all_buffers, key_i, key_s) - { - UNUSED(key_i); - assert(key_s != NULL); - zend_string *s; - memcpy(&s, ZSTR_VAL(key_s), sizeof(s)); // NOLINT - assert(ZSTR_LEN(key_s) == sizeof(s)); // NOLINT - zend_string_release(s); + if (_buffer) { + zend_string_release(_buffer); + _buffer = NULL; } - ZEND_HASH_FOREACH_END(); - zend_hash_destroy(&_all_buffers); } static typeof(zend_write(NULL, 0)) _dd_save_output_zend_write( @@ -72,13 +57,8 @@ void dd_entity_body_activate() if (desired_bufsize != _buffer_size) { if (_buffer != NULL) { zend_string_release(_buffer); - zend_hash_str_del( - &_all_buffers, (void *)&_buffer, sizeof(_buffer)); // NOLINT } _buffer = zend_string_alloc(desired_bufsize + /* NUL */ 1, 1); - // NOLINTNEXTLINE - zend_hash_str_add(&_all_buffers, (void *)&_buffer, sizeof(_buffer), - &(zval){.u1.type_info = IS_NULL}); // value is irrelevant _buffer_size = desired_bufsize; } diff --git a/appsec/src/extension/entity_body.h b/appsec/src/extension/entity_body.h index ea2ed8cba99..c6a020d88d0 100644 --- a/appsec/src/extension/entity_body.h +++ b/appsec/src/extension/entity_body.h @@ -10,7 +10,7 @@ #include void dd_entity_body_startup(void); -void dd_entity_body_shutdown(void); +void dd_entity_body_gshutdown(void); void dd_entity_body_activate(void); zend_string *nonnull dd_request_body_buffered(size_t limit); zend_string *nonnull dd_response_body_buffered(void); diff --git a/appsec/src/extension/request_lifecycle.c b/appsec/src/extension/request_lifecycle.c index 548a4be6353..1e0e5fdbb47 100644 --- a/appsec/src/extension/request_lifecycle.c +++ b/appsec/src/extension/request_lifecycle.c @@ -483,8 +483,10 @@ static zend_string *nullable _get_entity_as_string(zval *rbe_zv) // TODO: support non-seekable streams. Needs replacing the stream if (stream->flags & PHP_STREAM_FLAG_NO_SEEK) { - mlog(dd_log_info, "Response body entity is a stream, but it is " - "not seekable; ignoring"); + __auto_type lvl = + get_global_DD_APPSEC_TESTING() ? dd_log_info : dd_log_debug; + mlog(lvl, "Response body entity is a stream, but it is " + "not seekable; ignoring"); return NULL; } @@ -525,7 +527,9 @@ static zend_string *nullable _get_entity_as_string(zval *rbe_zv) size_t effective_size = stream_size - (size_t)start_pos; if (effective_size >= (size_t)get_DD_APPSEC_MAX_BODY_BUFF_SIZE()) { - mlog(dd_log_info, + __auto_type lvl = + get_global_DD_APPSEC_TESTING() ? dd_log_info : dd_log_debug; + mlog(lvl, "Response body entity is larger than %zu bytes (got %zu); ignoring", max_size, effective_size); return NULL; diff --git a/appsec/tests/extension/convert_json_max_depth.phpt b/appsec/tests/extension/convert_json_max_depth.phpt new file mode 100644 index 00000000000..e172d5f0ee4 --- /dev/null +++ b/appsec/tests/extension/convert_json_max_depth.phpt @@ -0,0 +1,36 @@ +--TEST-- +_convert_json function (exceed max depth) +--FILE-- + Array ( - [content] => Array + [0] => Array ( - [0] => + [@attr] => bar + ) + + [1] => test - [1] => Array + [2] => Array + ( + [br] => Array ( - [br] => Array - ( - ) - ) - [2] => baz - ) - [attributes] => Array - ( - [attr] => bar - ) + [3] => baz ) diff --git a/appsec/tests/extension/rshutdown_command_body_user_request_stream.phpt b/appsec/tests/extension/rshutdown_command_body_user_request_stream.phpt index 144b5672ef1..9dda5b0128c 100644 --- a/appsec/tests/extension/rshutdown_command_body_user_request_stream.phpt +++ b/appsec/tests/extension/rshutdown_command_body_user_request_stream.phpt @@ -91,26 +91,22 @@ Array ( [foo] => Array ( - [content] => Array + [0] => Array ( - [0] => + [@attr] => bar + ) + + [1] => test - [1] => Array + [2] => Array + ( + [br] => Array ( - [br] => Array - ( - ) - ) - [2] => baz - ) - [attributes] => Array - ( - [attr] => bar - ) + [3] => baz ) diff --git a/appsec/tests/extension/rshutdown_command_body_xml.phpt b/appsec/tests/extension/rshutdown_command_body_xml.phpt index b847810000d..046e2deb034 100644 --- a/appsec/tests/extension/rshutdown_command_body_xml.phpt +++ b/appsec/tests/extension/rshutdown_command_body_xml.phpt @@ -61,26 +61,22 @@ Array ( [foo] => Array ( - [content] => Array + [0] => Array ( - [0] => + [@attr] => bar + ) + + [1] => test - [1] => Array + [2] => Array + ( + [br] => Array ( - [br] => Array - ( - ) - ) - [2] => baz - ) - [attributes] => Array - ( - [attr] => bar - ) + [3] => baz ) diff --git a/appsec/tests/extension/rshutdown_command_body_xml_explicit_utf8.phpt b/appsec/tests/extension/rshutdown_command_body_xml_explicit_utf8.phpt index 2c7ddb53756..ca262742290 100644 --- a/appsec/tests/extension/rshutdown_command_body_xml_explicit_utf8.phpt +++ b/appsec/tests/extension/rshutdown_command_body_xml_explicit_utf8.phpt @@ -61,26 +61,22 @@ Array ( [foo] => Array ( - [content] => Array + [0] => Array ( - [0] => + [@attr] => bar + ) + + [1] => test - [1] => Array + [2] => Array + ( + [br] => Array ( - [br] => Array - ( - ) - ) - [2] => baz - ) - [attributes] => Array - ( - [attr] => bar - ) + [3] => baz ) diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy index eddc166801f..29aa2d606dd 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy @@ -316,8 +316,7 @@ trait CommonTests { ], "key_path" : [ "note", - "content", - "1" + "2" ], "value" : "\\n poison\\n" } diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy index f1b291598ad..de919102e61 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy @@ -296,8 +296,7 @@ class RoadRunnerTests { ], "key_path" : [ "note", - "content", - "1" + "2" ], "value" : "\\n poison\\n" } diff --git a/appsec/tests/integration/src/test/waf/recommended.json b/appsec/tests/integration/src/test/waf/recommended.json index be954478c5f..042e95ea538 100644 --- a/appsec/tests/integration/src/test/waf/recommended.json +++ b/appsec/tests/integration/src/test/waf/recommended.json @@ -6754,8 +6754,7 @@ { "address": "server.response.body", "key_path": [ - "note", - "content" + "note" ] } ],