Fix several bugs and potential bugs in appsec

* Fix free on possibly invalid pointer (the pointer might have been
  advanced in rare circumstances)
* Remove limit enforcing on the helper. If a limit was violated, it
  would invalidate the full message.
* Improve limits on the extension.
* Avoid using malloc on the extension when possible; make the mpack
  library use Zend's emalloc.
* Increase max message size on the helper from 64k to 700k. This should
  be able to accomodate msgpack-serialized json messages of 512k.
  (The limit on the extension is 4 MB and it was not changed)

(cherry picked from commit c826df2)

Author: cataphract

appsec/src/extension/commands/config_sync.c

@@ -12,7 +12,6 @@
 #include "../logging.h"
 #include "../msgpack_helpers.h"
 #include "config_sync.h"
-#include <mpack.h>
 
 static dd_result _request_pack(mpack_writer_t *nonnull w, void *nonnull ctx);
 dd_result dd_command_process_config_sync(

appsec/src/extension/commands/request_exec.c

@@ -50,7 +50,13 @@ static dd_result _pack_command(mpack_writer_t *nonnull w, void *nonnull _ctx)
     struct ctx *ctx = _ctx;
 
     dd_mpack_write_nullable_zstr(w, ctx->rasp_rule);
-    dd_mpack_write_zval(w, ctx->data);
+
+    dd_mpack_limits limits = dd_mpack_def_limits;
+    dd_mpack_write_zval_lim(w, ctx->data, &limits);
+
+    if (dd_mpack_limits_reached(&limits)) {
+        mlog(dd_log_info, "Limits reched when serializing request exec data");
+    }
 
     return dd_success;
 }

appsec/src/extension/commands/request_init.c

@@ -9,13 +9,12 @@
 
 #include "../commands_helpers.h"
 #include "../configuration.h"
-#include "../ddappsec.h"
-#include "../ddtrace.h"
+#include "../ddappsec.h" // NOLINT(unused-includes)
 #include "../entity_body.h"
-#include "../ip_extraction.h"
 #include "../logging.h"
 #include "../msgpack_helpers.h"
-#include "../php_compat.h"
+#include "../php_compat.h" // NOLINT(unused-includes)
+#include "../php_helpers.h"
 #include "../string_helpers.h"
 #include "request_init.h"
 #include <mpack.h>
@@ -31,7 +30,8 @@ static void _pack_files_field_names(
 static void _pack_path_params(
     mpack_writer_t *nonnull w, const zend_string *nullable uri_raw);
 static void _pack_request_body(mpack_writer_t *nonnull w,
-    struct req_info_init *nonnull ctx, const zend_array *nonnull server);
+    struct req_info_init *nonnull ctx, const zend_array *nonnull server,
+    dd_mpack_limits *nonnull limits);
 
 static const dd_command_spec _spec = {
     .name = "request_init",
@@ -64,20 +64,27 @@ static dd_result _request_pack(mpack_writer_t *nonnull w, void *nonnull _ctx)
     // Pack data from SAPI request_info
     sapi_request_info *request_info = &SG(request_info);
 
+    // Limits applied to query, cookies and request body
+    dd_mpack_limits limits = dd_mpack_def_limits;
+
     // 1.
     dd_mpack_write_lstr(w, "server.request.query");
-    dd_mpack_write_array(w, dd_get_superglob_or_equiv(ZEND_STRL("_GET"),
-                                TRACK_VARS_GET, ctx->superglob_equiv));
+    dd_mpack_write_array_lim(w,
+        dd_get_superglob_or_equiv(
+            ZEND_STRL("_GET"), TRACK_VARS_GET, ctx->superglob_equiv),
+        &limits);
 
     // 2.
     const zend_array *nonnull server = dd_get_superglob_or_equiv(
         ZEND_STRL("_SERVER"), TRACK_VARS_SERVER, ctx->superglob_equiv);
     dd_mpack_write_lstr(w, "server.request.method");
     if (ctx->superglob_equiv) {
-        dd_mpack_write_nullable_zstr(w,
-            dd_php_get_string_elem_cstr(server, ZEND_STRL("REQUEST_METHOD")));
+        dd_mpack_write_nullable_zstr_lim(w,
+            dd_php_get_string_elem_cstr(server, ZEND_STRL("REQUEST_METHOD")),
+            limits.max_string_length);
     } else {
-        mpack_write(w, request_info->request_method);
+        dd_mpack_write_nullable_cstr_lim(
+            w, request_info->request_method, limits.max_string_length);
     }
 
     // Pack data from server global
@@ -87,22 +94,24 @@ static dd_result _request_pack(mpack_writer_t *nonnull w, void *nonnull _ctx)
 
     // 3.
     dd_mpack_write_lstr(w, "server.request.cookies");
-    dd_mpack_write_array(w, dd_get_superglob_or_equiv(ZEND_STRL("_COOKIE"),
-                                TRACK_VARS_COOKIE, ctx->superglob_equiv));
+    dd_mpack_write_array_lim(w,
+        dd_get_superglob_or_equiv(
+            ZEND_STRL("_COOKIE"), TRACK_VARS_COOKIE, ctx->superglob_equiv),
+        &limits);
 
     // 4.
     const zend_string *nullable request_uri =
         dd_php_get_string_elem_cstr(server, ZEND_STRL("REQUEST_URI"));
     dd_mpack_write_lstr(w, "server.request.uri.raw");
-    dd_mpack_write_nullable_zstr(w, request_uri);
+    dd_mpack_write_nullable_zstr_lim(w, request_uri, limits.max_string_length);
 
     // 5.
     dd_mpack_write_lstr(w, "server.request.headers.no_cookies");
     _pack_headers(w, server);
 
     // 6.
     dd_mpack_write_lstr(w, "server.request.body");
-    _pack_request_body(w, ctx, server);
+    _pack_request_body(w, ctx, server, &limits);
 
     // 7.
     const zend_array *nonnull files = dd_get_superglob_or_equiv(
@@ -130,6 +139,10 @@ static dd_result _request_pack(mpack_writer_t *nonnull w, void *nonnull _ctx)
 
     mpack_finish_map(w);
 
+    if (dd_mpack_limits_reached(&limits)) {
+        mlog(dd_log_info, "Limits reched when serializing request init");
+    }
+
     return dd_success;
 }
 
@@ -163,6 +176,9 @@ static void _pack_headers(
 {
     mpack_build_map(w);
 
+    // apply limits to part of the headers map
+    dd_mpack_limits limits = dd_mpack_def_limits;
+
     // Pack headers
     zend_string *key;
     zval *val;
@@ -172,12 +188,17 @@ static void _pack_headers(
             continue;
         }
 
+        if (limits.elements_remaining == 0) {
+            mlog(dd_log_warning, "Headers map is full, stopping");
+            break;
+        }
+
         if (zend_string_equals_literal(key, "CONTENT_TYPE")) {
             dd_mpack_write_lstr(w, "content-type");
-            dd_mpack_write_zval(w, val);
+            dd_mpack_write_zval_lim(w, val, &limits);
         } else if (zend_string_equals_literal(key, "CONTENT_LENGTH")) {
             dd_mpack_write_lstr(w, "content-length");
-            dd_mpack_write_zval(w, val);
+            dd_mpack_write_zval_lim(w, val, &limits);
         } else if (zend_string_equals_literal(key, "PHP_AUTH_DIGEST")) {
             if (Z_TYPE_P(val) == IS_STRING && Z_STRLEN_P(val) > 0) {
                 dd_mpack_write_lstr(w, "authorization");
@@ -186,14 +207,16 @@ static void _pack_headers(
                 memcpy(auth_str, "digest ", sizeof("digest ") - 1);
                 memcpy(auth_str + sizeof("digest ") - 1, Z_STRVAL_P(val),
                     Z_STRLEN_P(val));
-                mpack_write_str(w, auth_str, auth_len);
+                dd_mpack_write_nullable_str_lim(
+                    w, auth_str, auth_len, limits.max_string_length);
                 efree(auth_str);
             }
-        } else if (_is_relevant_header(key)) {
+        } else if (_is_relevant_header(key)) { // i.e. not a cookie header
             zend_string *transf_header_name = _transform_header_name(key);
-            dd_mpack_write_zstr(w, transf_header_name);
+            dd_mpack_write_zstr_lim(
+                w, transf_header_name, limits.max_string_length);
             zend_string_efree(transf_header_name);
-            dd_mpack_write_zval(w, val);
+            dd_mpack_write_zval_lim(w, val, &limits);
         }
     }
     ZEND_HASH_FOREACH_END();
@@ -290,29 +313,31 @@ static void _pack_path_params(
 }
 
 static void _pack_request_body(mpack_writer_t *nonnull w,
-    struct req_info_init *nonnull ctx, const zend_array *nonnull server)
+    struct req_info_init *nonnull ctx, const zend_array *nonnull server,
+    dd_mpack_limits *nonnull limits)
 {
     const zend_array *post = dd_get_superglob_or_equiv(
         ZEND_STRL("_POST"), TRACK_VARS_POST, ctx->superglob_equiv);
     if (zend_hash_num_elements(post) != 0) {
         dd_mpack_write_array(w, post);
-    } else {
-        bool written = false;
-        if (ctx->entity) {
-            zend_string *ct =
-                dd_php_get_string_elem_cstr(server, ZEND_STRL("CONTENT_TYPE"));
-            if (ct) {
-                zval body_zv = dd_entity_body_convert(
-                    ZSTR_VAL(ct), ZSTR_LEN(ct), ctx->entity);
-                if (Z_TYPE(body_zv) != IS_NULL) {
-                    dd_mpack_write_zval(w, &body_zv);
-                    zval_ptr_dtor(&body_zv);
-                    written = true;
-                }
+        return;
+    }
+
+    bool written = false;
+    if (ctx->entity) {
+        zend_string *ct =
+            dd_php_get_string_elem_cstr(server, ZEND_STRL("CONTENT_TYPE"));
+        if (ct) {
+            zval body_zv =
+                dd_entity_body_convert(ZSTR_VAL(ct), ZSTR_LEN(ct), ctx->entity);
+            if (Z_TYPE(body_zv) != IS_NULL) {
+                dd_mpack_write_zval_lim(w, &body_zv, limits);
+                zval_ptr_dtor(&body_zv);
+                written = true;
             }
         }
-        if (!written) {
-            dd_mpack_write_array(w, &zend_empty_array);
-        }
+    }
+    if (!written) {
+        dd_mpack_write_array(w, &zend_empty_array);
     }
 }

appsec/src/extension/commands/request_shutdown.c

@@ -82,9 +82,15 @@ static dd_result _request_pack(mpack_writer_t *nonnull w, void *nonnull ctx)
 
     // 1.3.?
     if (Z_TYPE(resp_body) != IS_NULL) {
+        dd_mpack_limits limits = dd_mpack_def_limits;
+
         dd_mpack_write_lstr(w, "server.response.body");
-        dd_mpack_write_zval(w, &resp_body);
+        dd_mpack_write_zval_lim(w, &resp_body, &limits);
         zval_ptr_dtor_nogc(&resp_body);
+
+        if (dd_mpack_limits_reached(&limits)) {
+            mlog(dd_log_info, "Limits reched when serializing response body");
+        }
     }
 
     mpack_finish_map(w);
@@ -157,13 +163,14 @@ static void _pack_headers_no_cookies_llist(
     zend_llist *coll;
     ZEND_HASH_FOREACH_STR_KEY_PTR(&headers_map, key, coll)
     {
-        mpack_write_str(w, ZSTR_VAL(key), ZSTR_LEN(key));
-        mpack_start_array(w, zend_llist_count(coll));
+        dd_mpack_write_zstr(w, key);
 
+        mpack_start_array(w, zend_llist_count(coll));
         zend_llist_position p;
         for (struct _header_val *hv = zend_llist_get_first_ex(coll, &p); hv;
              hv = zend_llist_get_next_ex(coll, &p)) {
-            mpack_write_str(w, hv->val, hv->len);
+            dd_mpack_write_nullable_str_lim(
+                w, hv->val, hv->len, DD_MPACK_DEF_STRING_LIMIT);
         }
         mpack_finish_array(w);
     }
@@ -217,15 +224,16 @@ static void _pack_headers_no_cookies_map(
             zend_string_addref(key);
         }
 
-        mpack_write_str(w, ZSTR_VAL(key), ZSTR_LEN(key));
+        dd_mpack_write_zstr(w, key);
 
         if (Z_TYPE_P(val) == IS_ARRAY) {
             mpack_start_array(w, zend_array_count(Z_ARRVAL_P(val)));
             zval *zv;
             ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(val), zv)
             {
                 if (Z_TYPE_P(zv) == IS_STRING) {
-                    mpack_write_str(w, Z_STRVAL_P(zv), Z_STRLEN_P(zv));
+                    dd_mpack_write_zstr_lim(
+                        w, Z_STR_P(zv), DD_MPACK_DEF_STRING_LIMIT);
                 } else {
                     mpack_write_str(w, ZEND_STRL("(invalid value)"));
                     mlog(dd_log_warning,
@@ -237,7 +245,7 @@ static void _pack_headers_no_cookies_map(
             ZEND_HASH_FOREACH_END();
         } else if (Z_TYPE_P(val) == IS_STRING) {
             mpack_start_array(w, 1);
-            mpack_write_str(w, Z_STRVAL_P(val), Z_STRLEN_P(val));
+            dd_mpack_write_zstr_lim(w, Z_STR_P(val), DD_MPACK_DEF_STRING_LIMIT);
         } else {
             mpack_start_array(w, 1);
             mpack_write_str(w, ZEND_STRL("(invalid value)"));

appsec/src/extension/commands_helpers.c

@@ -99,11 +99,8 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn,
         dd_imsg imsg = {0};
         res = _imsg_recv(&imsg, conn);
         if (res) {
-            if (res != dd_helper_error) {
-                mlog(dd_log_warning,
-                    "Error receiving reply for command %.*s: %s", NAME_L,
-                    dd_result_to_string(res));
-            }
+            mlog(dd_log_warning, "Error receiving reply for command %.*s: %s",
+                NAME_L, dd_result_to_string(res));
             return res;
         }
 
@@ -283,7 +280,7 @@ static ATTR_WARN_UNUSED dd_result _imsg_recv(
 static inline ATTR_WARN_UNUSED mpack_error_t _imsg_destroy(
     dd_imsg *nonnull imsg)
 {
-    free(imsg->_data);
+    efree(imsg->_data);
     imsg->_data = NULL;
     imsg->_size = 0;
     return mpack_tree_destroy(&imsg->_tree);
@@ -825,7 +822,7 @@ void _handle_telemetry_metric(const char *nonnull key_str, size_t key_len,
         if (key_len == LSTRLEN(name) && memcmp(key_str, name, key_len) == 0) { \
             static zend_string *_Atomic key_zstr;                              \
             _init_zstr(&key_zstr, name, LSTRLEN(name));                        \
-            zend_string *tags_zstr = zend_string_init(tags_str, tags_len, 1);  \
+            zend_string *tags_zstr = zend_string_init(tags_str, tags_len, 0);  \
             dd_telemetry_add_metric(key_zstr, value, tags_zstr, type);         \
             zend_string_release(tags_zstr);                                    \
             mlog_g(dd_log_debug,                                               \

appsec/src/extension/mpack/mpack-config.h

@@ -0,0 +1,7 @@
+#pragma once
+
+#include <Zend/zend.h>
+
+#define MPACK_MALLOC emalloc
+#define MPACK_REALLOC erealloc
+#define MPACK_FREE efree