[PATCH] imap-login: Fix excessive memory growth with pre-login ID command

The pre-login ID parser reset the per-argument line and list limits after
every argument, so nothing bounded the number of key/value pairs. Each
external key grew log_reply and added a distinct event field (a linear
scan, making it O(N^2)), allowing a pre-auth CPU/memory amplification.

Account at most 30 pairs (per RFC 2971) into the logging and client_id
bookkeeping. Internal x-* parameter handlers still run for pairs beyond
the limit, since a trusted proxy may legitimately forward more fields.

Gbp-Pq: Name 0001-imap-login-Fix-excessive-memory-growth-with-pre-logi.patch

diff --git a/src/imap-login/imap-login-client.h b/src/imap-login/imap-login-client.h
index 34ea226c707c4b67e30830b3db07133fc58bd32b..be4f16f2049a5c36629ffa2923ef237849be8d49 100644 (file)
--- a/src/imap-login/imap-login-client.h
+++ b/src/imap-login/imap-login-client.h
@@ -53,6 +53,10 @@ struct imap_client_cmd_id {
        struct event *params_event;
        struct imap_id_params *params;
        string_t *log_reply;
+       /* Number of external (non internal x-*) key/value pairs accounted so
+          far. Used to bound the pre-login accounting (logging, event fields,
+          client_id) done per external pair. */
+       unsigned int processed_pairs_count;
 };
 
 struct imap_client {

diff --git a/src/imap-login/imap-login-cmd-id.c b/src/imap-login/imap-login-cmd-id.c
index be12744e0834d251a815de12b4c1102ec94f61ee..d86817d25bb86a4950238169ae8c36c8b72f42e1 100644 (file)
--- a/src/imap-login/imap-login-cmd-id.c
+++ b/src/imap-login/imap-login-cmd-id.c
@@ -9,6 +9,12 @@
 #include "imap-login-settings.h"
 #include "imap-login-client.h"
 
+/* RFC 2971 says a client SHOULD NOT send more than 30 field-value pairs.
+   Allow that many to be accounted for (logged / kept in client_id); pairs
+   beyond this are still parsed and internal x-* handlers still run, but they
+   are not added to the logging/client_id accounting that grows per pair. */
+#define IMAP_ID_MAX_ACCOUNTED_PAIRS 30
+
 struct imap_id_params_forward {
        const char *key;
        const char *value;
@@ -183,6 +189,16 @@ static bool cmd_id_handle_keyvalue(struct imap_client *client,
                imap_id_param_handler_find(key);
        bool is_login_id_param = handler != NULL;
 
+       /* Only external pairs are logged / kept in client_id, and only those
+          grow per-pair with count, so only they need bounding to avoid a
+          pre-login CPU/memory DoS. Internal x-* handlers must keep running
+          regardless (e.g. a proxy may legitimately forward more than 30
+          fields) and are not counted against the limit. */
+       bool account_pair = !is_login_id_param &&
+               client->cmd_id->processed_pairs_count < IMAP_ID_MAX_ACCOUNTED_PAIRS;
+       if (account_pair)
+               client->cmd_id->processed_pairs_count++;
+
        if (!is_login_id_param) {
                /* not an internal key */
        } else if (client->id_logged) {
@@ -197,7 +213,7 @@ static bool cmd_id_handle_keyvalue(struct imap_client *client,
                }
        }
 
-       if (client->set->imap_id_retain && !is_login_id_param &&
+       if (account_pair && client->set->imap_id_retain &&
            (client->common.client_id == NULL ||
             str_len(client->common.client_id) + kvlen < LOGIN_MAX_CLIENT_ID_LEN)) {
                if (client->common.client_id == NULL) {
@@ -213,7 +229,7 @@ static bool cmd_id_handle_keyvalue(struct imap_client *client,
                        imap_append_quoted(client->common.client_id, value);
        }
 
-       if (!is_login_id_param)
+       if (account_pair)
                imap_id_add_log_entry(log_entry, key, value);
        return TRUE;
 }