[CritFix] http: fix shared body storage lifecycle

The proxy cleared RSPAMD_HTTP_FLAG_SHMEM by hand before installing a
new body. The following rspamd_http_message_set_body() then cleaned up
the storage as if it were an fstring and free(3)'d the refcounted
rspamd_storage_shmem the union aliases, whilst the segment descriptor
and its mapping were leaked. Add rspamd_http_message_drop_shared_body()
which releases the shared storage whilst the flags still describe it,
and use it in all three places.

storage_cleanup() skipped a segment sitting on descriptor 0, leaking
both the descriptor and the mapping whenever the kernel handed us that
one. Releasing it uncovered the reason it looked harmless: a message
can carry the shared flag before a segment exists for it (a body-less
copy inherits the flag from its origin), and the zero then read as a
valid descriptor and closed whatever occupied slot 0 - in a proxy
worker that is the accepted client socket. Initialise shm_fd to -1 in
rspamd_http_new_message() so it is only ever valid for a real segment,
then release it properly.

Whilst here: reset `name` after releasing it so the next cleanup cannot
release it twice, drop the fstat(2) hidden inside g_assert(), reject
non-regular and empty descriptors in set_body_from_fd(), guard the
length overflow in grow_body() and validate that the body of a copied
message really fits the segment it is mapped from.

diff --git a/src/libserver/http/http_connection.c b/src/libserver/http/http_connection.c
index a3971b175c1357a75e29221a5b27c7913e013827..347a7f5486dad0a3777440ea02043307d8c9c743 100644 (file)
--- a/src/libserver/http/http_connection.c
+++ b/src/libserver/http/http_connection.c
@@ -1501,11 +1501,15 @@ rspamd_http_connection_copy_msg(struct rspamd_http_message *msg, GError **err)
        if (msg->body_buf.len > 0) {
 
                if (msg->flags & RSPAMD_HTTP_FLAG_SHMEM) {
+                       gsize body_off = 0;
+
                        /* Avoid copying by just mapping a shared segment */
                        new_msg->flags |= RSPAMD_HTTP_FLAG_SHMEM_IMMUTABLE;
 
                        storage = &new_msg->body_buf.c;
+                       storage->shared.name = NULL;
                        storage->shared.shm_fd = dup(msg->body_buf.c.shared.shm_fd);
+                       new_msg->body_buf.str = MAP_FAILED;
 
                        if (storage->shared.shm_fd == -1) {
                                rspamd_http_message_unref(new_msg);
@@ -1525,6 +1529,24 @@ rspamd_http_connection_copy_msg(struct rspamd_http_message *msg, GError **err)
                                return NULL;
                        }
 
+                       if (RSPAMD_HTTP_BODY_IS_MAPPED(msg) &&
+                               msg->body_buf.begin >= msg->body_buf.str) {
+                               body_off = msg->body_buf.begin - msg->body_buf.str;
+                       }
+
+                       if (st.st_size <= 0 ||
+                               (gsize) st.st_size < body_off ||
+                               msg->body_buf.len > (gsize) st.st_size - body_off) {
+                               g_set_error(err, http_error_quark(), EINVAL,
+                                                       "shmem fd %d does not fit the body: %zu bytes at "
+                                                       "offset %zu of %lld",
+                                                       storage->shared.shm_fd, msg->body_buf.len,
+                                                       body_off, (long long) st.st_size);
+                               rspamd_http_message_unref(new_msg);
+
+                               return NULL;
+                       }
+
                        /* We don't own segment, so do not try to touch it */
 
                        if (msg->body_buf.c.shared.name) {
@@ -1545,10 +1567,9 @@ rspamd_http_connection_copy_msg(struct rspamd_http_message *msg, GError **err)
                                return NULL;
                        }
 
-                       new_msg->body_buf.begin = new_msg->body_buf.str;
+                       new_msg->body_buf.begin = new_msg->body_buf.str + body_off;
                        new_msg->body_buf.len = msg->body_buf.len;
-                       new_msg->body_buf.begin = new_msg->body_buf.str +
-                                                                         (msg->body_buf.begin - msg->body_buf.str);
+                       new_msg->body_buf.allocated_len = st.st_size;
                }
                else {
                        old_body = rspamd_http_message_get_body(msg, &old_len);
@@ -1874,7 +1895,14 @@ rspamd_http_detach_shared(struct rspamd_http_message *msg)
 {
        rspamd_fstring_t *cpy_str;
 
-       cpy_str = rspamd_fstring_new_init(msg->body_buf.begin, msg->body_buf.len);
+       if (msg->body_buf.begin != NULL && msg->body_buf.len > 0) {
+               cpy_str = rspamd_fstring_new_init(msg->body_buf.begin,
+                                                                                 msg->body_buf.len);
+       }
+       else {
+               cpy_str = rspamd_fstring_new();
+       }
+
        rspamd_http_message_set_body_from_fstring_steal(msg, cpy_str);
 }
 
@@ -2316,14 +2344,20 @@ rspamd_http_connection_write_message_common(struct rspamd_http_connection *conn,
                        allow_shared = FALSE;
                }
                else {
+                       gsize shm_offset = 0;
+
+                       if (RSPAMD_HTTP_BODY_IS_MAPPED(msg) &&
+                               msg->body_buf.begin >= msg->body_buf.str) {
+                               shm_offset = msg->body_buf.begin - msg->body_buf.str;
+                       }
+
                        /* Insert new headers */
                        rspamd_http_message_add_header(msg, "Shm",
                                                                                   msg->body_buf.c.shared.name->shm_name);
-                       rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%d",
-                                                       (int) (msg->body_buf.begin - msg->body_buf.str));
+                       rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%uz", shm_offset);
                        rspamd_http_message_add_header(msg, "Shm-Offset",
                                                                                   tmpbuf);
-                       rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%z",
+                       rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%uz",
                                                        msg->body_buf.len);
                        rspamd_http_message_add_header(msg, "Shm-Length",
                                                                                   tmpbuf);

diff --git a/src/libserver/http/http_message.c b/src/libserver/http/http_message.c
index b27b64caaea95feb3dd175698b1ae35451337a50..3d2fa393a05a4691889a2c7a6731742b7f657920 100644 (file)
--- a/src/libserver/http/http_message.c
+++ b/src/libserver/http/http_message.c
@@ -40,6 +40,14 @@ rspamd_http_new_message(enum rspamd_http_message_type type)
        new->type = type;
        new->method = HTTP_INVALID;
        new->headers = kh_init(rspamd_http_headers_hash);
+       /*
+        * `shm_fd` lives past the pointer that `normal` shares the union with, so
+        * initialising it here is safe for both storage kinds. It must never be
+        * left as a valid looking zero: a message can be flagged as shared before
+        * a segment is actually created for it, and the cleanup would then close a
+        * completely unrelated descriptor 0.
+        */
+       new->body_buf.c.shared.shm_fd = -1;
 
        REF_INIT_RETAIN(new, rspamd_http_message_free);
 
@@ -204,6 +212,11 @@ rspamd_http_message_set_body(struct rspamd_http_message *msg,
        rspamd_http_message_storage_cleanup(msg);
 
        if (msg->flags & RSPAMD_HTTP_FLAG_SHMEM) {
+               if (len == G_MAXSIZE) {
+                       /* Unknown length is treated as an empty body */
+                       len = 0;
+               }
+
                storage->shared.name = g_malloc(sizeof(*storage->shared.name));
                REF_INIT_RETAIN(storage->shared.name, rspamd_http_shname_dtor);
 #ifdef HAVE_SANE_SHMEM
@@ -225,7 +238,7 @@ rspamd_http_message_set_body(struct rspamd_http_message *msg,
                        return FALSE;
                }
 
-               if (len != 0 && len != G_MAXSIZE) {
+               if (len != 0) {
                        if (ftruncate(storage->shared.shm_fd, len) == -1) {
                                return FALSE;
                        }
@@ -303,6 +316,8 @@ rspamd_http_message_set_body_from_fd(struct rspamd_http_message *msg,
        storage = &msg->body_buf.c;
        msg->flags |= RSPAMD_HTTP_FLAG_SHMEM | RSPAMD_HTTP_FLAG_SHMEM_IMMUTABLE;
 
+       /* We do not own the segment, so there is no name to unlink afterwards */
+       storage->shared.name = NULL;
        storage->shared.shm_fd = dup(fd);
        msg->body_buf.str = MAP_FAILED;
 
@@ -314,6 +329,11 @@ rspamd_http_message_set_body_from_fd(struct rspamd_http_message *msg,
                return FALSE;
        }
 
+       if (!S_ISREG(st.st_mode) || st.st_size <= 0) {
+               /* Nothing that can be mapped: not a regular file or an empty one */
+               return FALSE;
+       }
+
        msg->body_buf.str = mmap(NULL, st.st_size,
                                                         PROT_READ, MAP_SHARED,
                                                         storage->shared.shm_fd, 0);
@@ -384,6 +404,11 @@ rspamd_http_message_grow_body(struct rspamd_http_message *msg, gsize len)
                        return FALSE;
                }
 
+               if (len > G_MAXSIZE - msg->body_buf.len) {
+                       /* Integer overflow in the requested size */
+                       return FALSE;
+               }
+
                if (fstat(storage->shared.shm_fd, &st) == -1) {
                        return FALSE;
                }
@@ -394,8 +419,9 @@ rspamd_http_message_grow_body(struct rspamd_http_message *msg, gsize len)
                        newlen = rspamd_fstring_suggest_size(msg->body_buf.len, st.st_size,
                                                                                                 len);
                        /* Unmap as we need another size of segment */
-                       if (msg->body_buf.str != MAP_FAILED) {
+                       if (RSPAMD_HTTP_BODY_IS_MAPPED(msg)) {
                                munmap(msg->body_buf.str, st.st_size);
+                               msg->body_buf.str = MAP_FAILED;
                        }
 
                        if (ftruncate(storage->shared.shm_fd, newlen) == -1) {
@@ -485,11 +511,21 @@ void rspamd_http_message_storage_cleanup(struct rspamd_http_message *msg)
        if (msg->flags & RSPAMD_HTTP_FLAG_SHMEM) {
                storage = &msg->body_buf.c;
 
-               if (storage->shared.shm_fd > 0) {
-                       g_assert(fstat(storage->shared.shm_fd, &st) != -1);
-
-                       if (msg->body_buf.str != MAP_FAILED) {
-                               munmap(msg->body_buf.str, st.st_size);
+               if (storage->shared.shm_fd >= 0) {
+                       if (RSPAMD_HTTP_BODY_IS_MAPPED(msg)) {
+                               /*
+                                * We map the whole segment, hence its current size is the
+                                * mapping length. If fstat fails somehow, we have no reliable
+                                * length to unmap, so we have to leak the mapping instead of
+                                * unmapping a wrong range.
+                                */
+                               if (fstat(storage->shared.shm_fd, &st) != -1) {
+                                       munmap(msg->body_buf.str, st.st_size);
+                               }
+                               else {
+                                       msg_err("cannot fstat shmem fd %d: %s; mapping is leaked",
+                                                       storage->shared.shm_fd, strerror(errno));
+                               }
                        }
 
                        close(storage->shared.shm_fd);
@@ -499,6 +535,12 @@ void rspamd_http_message_storage_cleanup(struct rspamd_http_message *msg)
                        REF_RELEASE(storage->shared.name);
                }
 
+               /*
+                * `name` shares the storage with `normal` (it is a union), so it must
+                * be reset unconditionally: leaving a dangling pointer here would make
+                * the next cleanup release it for the second time
+                */
+               storage->shared.name = NULL;
                storage->shared.shm_fd = -1;
                msg->body_buf.str = MAP_FAILED;
        }
@@ -513,6 +555,23 @@ void rspamd_http_message_storage_cleanup(struct rspamd_http_message *msg)
        msg->body_buf.len = 0;
 }
 
+void rspamd_http_message_drop_shared_body(struct rspamd_http_message *msg)
+{
+       if (!(msg->flags & RSPAMD_HTTP_FLAG_SHMEM)) {
+               return;
+       }
+
+       /* Cleanup whilst the flags still describe the storage we actually have */
+       rspamd_http_message_storage_cleanup(msg);
+
+       msg->flags &= ~(RSPAMD_HTTP_FLAG_SHMEM | RSPAMD_HTTP_FLAG_SHMEM_IMMUTABLE);
+       msg->body_buf.c.normal = NULL;
+       msg->body_buf.begin = NULL;
+       msg->body_buf.str = NULL;
+       msg->body_buf.len = 0;
+       msg->body_buf.allocated_len = 0;
+}
+
 void rspamd_http_message_free(struct rspamd_http_message *msg)
 {
        struct rspamd_http_header *hdr, *hcur, *hcurtmp;

diff --git a/src/libserver/http/http_message.h b/src/libserver/http/http_message.h
index 0b1c9441c6f2a11a1f155e82b56c8c7bc3bee0ed..6d58e6571fb1b6a6c0cbcdbf4bd151c436626ae4 100644 (file)
--- a/src/libserver/http/http_message.h
+++ b/src/libserver/http/http_message.h
@@ -237,6 +237,18 @@ struct rspamd_storage_shmem *rspamd_http_message_shmem_ref(struct rspamd_http_me
  */
 void rspamd_http_message_shmem_unref(struct rspamd_storage_shmem *p);
 
+/**
+ * Release shared memory storage of a message (if any) and switch it back to the
+ * ordinary heap storage.
+ *
+ * The body content is NOT preserved, so this is merely a way to say "forget the
+ * shared body, I'm going to set a new one". Clearing RSPAMD_HTTP_FLAG_SHMEM
+ * manually instead of calling this function leaks the mapping and the segment
+ * descriptor, and makes the next cleanup treat the shmem storage as an fstring.
+ * @param msg
+ */
+void rspamd_http_message_drop_shared_body(struct rspamd_http_message *msg);
+
 /**
  * Returns message's flags
  * @param msg

diff --git a/src/libserver/http/http_private.h b/src/libserver/http/http_private.h
index 8613a4366b71a42b176892eeb37c090acb1d6b7e..3d6b545367d88089cbc15583b20abf0e5978051b 100644 (file)
--- a/src/libserver/http/http_private.h
+++ b/src/libserver/http/http_private.h
@@ -25,6 +25,8 @@
 #include "upstream.h"
 #include "khash.h"
 
+#include <sys/mman.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -94,6 +96,14 @@ struct rspamd_http_message {
        ref_entry_t ref;
 };
 
+/*
+ * True if the message body currently lives in a shared mapping that we own and
+ * hence have to unmap. `str` is NULL for a message with no body yet and
+ * MAP_FAILED once the storage has been cleaned up or a mapping attempt failed.
+ */
+#define RSPAMD_HTTP_BODY_IS_MAPPED(msg) \
+       ((msg)->body_buf.str != NULL && (msg)->body_buf.str != (char *) MAP_FAILED)
+
 struct rspamd_keepalive_hash_key {
        rspamd_inet_addr_t *addr;
        char *host;

diff --git a/src/rspamd_proxy.c b/src/rspamd_proxy.c
index b8d1c60e7b01ffd4073b9dc65c5e5089eb7e5fe9..45c33262d637aafa3a4df2dbf4af2f7444d66b98 100644 (file)
--- a/src/rspamd_proxy.c
+++ b/src/rspamd_proxy.c
@@ -119,7 +119,7 @@ struct rspamd_http_mirror {
        gboolean local;
        gboolean compress;
        gboolean ssl;
-       gboolean keepalive; /* Whether to use keepalive for this mirror */
+       gboolean keepalive;     /* Whether to use keepalive for this mirror */
        gboolean follow_master; /* Tie mirror lifetime to master upstream */
        enum rspamd_proxy_log_tag_type log_tag_type;
        ucl_object_t *extra_headers;
@@ -2070,7 +2070,7 @@ proxy_open_mirror_connections(struct rspamd_proxy_session *session)
                                                }
                                                else {
                                                        if (session->fname) {
-                                                               msg->flags &= ~RSPAMD_HTTP_FLAG_SHMEM;
+                                                               rspamd_http_message_drop_shared_body(msg);
                                                                rspamd_http_message_set_body(msg, session->map, session->map_len);
                                                        }
 
@@ -2232,7 +2232,7 @@ proxy_open_mirror_connections(struct rspamd_proxy_session *session)
                }
                else {
                        if (session->fname) {
-                               msg->flags &= ~RSPAMD_HTTP_FLAG_SHMEM;
+                               rspamd_http_message_drop_shared_body(msg);
                                rspamd_http_message_set_body(msg, session->map, session->map_len);
                        }
 
@@ -3021,7 +3021,7 @@ proxy_send_master_message(struct rspamd_proxy_session *session)
                }
                else {
                        if (session->fname) {
-                               msg->flags &= ~RSPAMD_HTTP_FLAG_SHMEM;
+                               rspamd_http_message_drop_shared_body(msg);
                                rspamd_http_message_set_body(msg,
                                                                                         session->map, session->map_len);
                        }