vici: Increase vici message length header from 16 to 32 bits

While we currently have no need for messages larger than 65KB, we should design
the protocol to be future-proof, as we plan to keep at least to lowest protocol
layer stable.

To avoid any allocation issues, we currently keep the message size limit at
512KB.

diff --git a/src/libcharon/plugins/vici/README.md b/src/libcharon/plugins/vici/README.md
index ff5031e6e4831a7068e2dfef9ada93573da5c9cf..46fa5b5285ec966c7c3ec0c6ccb921630f0ecb01 100644 (file)
--- a/src/libcharon/plugins/vici/README.md
+++ b/src/libcharon/plugins/vici/README.md
@@ -14,10 +14,10 @@ A client connects to this service to access functionality. It may send an
 arbitrary number of packets over the connection before closing it.
 
 To exchange data, the transport protocol is segmented into byte sequences.
-Each byte sequence is prefixed by a 16-bit length header in network order,
-followed by the data. The maximum segment length is 65535 bytes, and the length
-field contains the length of the data only, not including the length field
-itself.
+Each byte sequence is prefixed by a 32-bit length header in network order,
+followed by the data. The maximum segment length is currently limited to 512KB
+of data, and the length field contains the length of the data only, not
+including the length field itself.
 
 The order of byte sequences must be strict, byte sequences must arrive in the
 same order as sent.

diff --git a/src/libcharon/plugins/vici/libvici.c b/src/libcharon/plugins/vici/libvici.c
index becd8867101164526cf0383fd02b8c48038ed9b7..211fefd57fc9b2bfeb048d9135b01df95093afc4 100644 (file)
--- a/src/libcharon/plugins/vici/libvici.c
+++ b/src/libcharon/plugins/vici/libvici.c
@@ -16,6 +16,7 @@
 #include "libvici.h"
 #include "vici_builder.h"
 #include "vici_dispatcher.h"
+#include "vici_socket.h"
 
 #include <library.h>
 #include <threading/mutex.h>
@@ -122,7 +123,7 @@ static bool read_error(vici_conn_t *conn, int err)
 /**
  * Handle a command response message
  */
-static bool handle_response(vici_conn_t *conn, u_int16_t len)
+static bool handle_response(vici_conn_t *conn, u_int32_t len)
 {
        chunk_t buf;
 
@@ -139,7 +140,7 @@ static bool handle_response(vici_conn_t *conn, u_int16_t len)
 /**
  * Dispatch received event message
  */
-static bool handle_event(vici_conn_t *conn, u_int16_t len)
+static bool handle_event(vici_conn_t *conn, u_int32_t len)
 {
        vici_message_t *message;
        event_t *event;
@@ -197,7 +198,7 @@ static bool handle_event(vici_conn_t *conn, u_int16_t len)
 CALLBACK(on_read, bool,
        vici_conn_t *conn, stream_t *stream)
 {
-       u_int16_t len;
+       u_int32_t len;
        u_int8_t op;
        ssize_t hlen;
 
@@ -218,7 +219,11 @@ CALLBACK(on_read, bool,
                }
        }
 
-       len = ntohs(len);
+       len = ntohl(len);
+       if (len > VICI_MESSAGE_SIZE_MAX)
+       {
+               return read_error(conn, EBADMSG);
+       }
        if (len-- < sizeof(op))
        {
                return read_error(conn, EBADMSG);
@@ -353,7 +358,7 @@ vici_res_t* vici_submit(vici_req_t *req, vici_conn_t *conn)
        vici_message_t *message;
        vici_res_t *res;
        chunk_t data;
-       u_int16_t len;
+       u_int32_t len;
        u_int8_t namelen, op;
 
        message = req->b->finalize(req->b);
@@ -366,7 +371,7 @@ vici_res_t* vici_submit(vici_req_t *req, vici_conn_t *conn)
        op = VICI_CMD_REQUEST;
        namelen = strlen(req->name);
        data = message->get_encoding(message);
-       len = htons(sizeof(op) + sizeof(namelen) + namelen + data.len);
+       len = htonl(sizeof(op) + sizeof(namelen) + namelen + data.len);
 
        if (!conn->stream->write_all(conn->stream, &len, sizeof(len)) ||
                !conn->stream->write_all(conn->stream, &op, sizeof(op)) ||
@@ -679,13 +684,13 @@ void vici_free_res(vici_res_t *res)
 int vici_register(vici_conn_t *conn, char *name, vici_event_cb_t cb, void *user)
 {
        event_t *event;
-       u_int16_t len;
+       u_int32_t len;
        u_int8_t namelen, op;
        int ret = 1;
 
        op = cb ? VICI_EVENT_REGISTER : VICI_EVENT_UNREGISTER;
        namelen = strlen(name);
-       len = htons(sizeof(op) + sizeof(namelen) + namelen);
+       len = htonl(sizeof(op) + sizeof(namelen) + namelen);
        if (!conn->stream->write_all(conn->stream, &len, sizeof(len)) ||
                !conn->stream->write_all(conn->stream, &op, sizeof(op)) ||
                !conn->stream->write_all(conn->stream, &namelen, sizeof(namelen)) ||

diff --git a/src/libcharon/plugins/vici/suites/test_socket.c b/src/libcharon/plugins/vici/suites/test_socket.c
index a9ebdbfca759b16e82600de6bb12159df284ce68..032445bb08f0c684305e208ba30233eedae3ae22 100644 (file)
--- a/src/libcharon/plugins/vici/suites/test_socket.c
+++ b/src/libcharon/plugins/vici/suites/test_socket.c
@@ -32,7 +32,7 @@ static void echo_inbound(void *user, u_int id, chunk_t buf)
 
        ck_assert_int_eq(data->id, id);
        /* count number of bytes, including the header */
-       data->bytes += buf.len + sizeof(u_int16_t);
+       data->bytes += buf.len + sizeof(u_int32_t);
        /* echo back data chunk */
        data->s->send(data->s, id, chunk_clone(buf));
 }
@@ -73,13 +73,13 @@ START_TEST(test_echo)
        stream_t *c;
        test_data_t data = {};
        chunk_t x, m = chunk_from_chars(
-               0x00,0x00,
-               0x00,0x01,      0x01,
-               0x00,0x05,      0x11,0x12,0x13,0x14,0x15,
-               0x00,0x0A,      0x21,0x22,0x23,0x24,0x25,0x26,0x27,0x28,0x29,0x02A,
+               0x00,0x00,0x00,0x00,
+               0x00,0x00,0x00,0x01,    0x01,
+               0x00,0x00,0x00,0x05,    0x11,0x12,0x13,0x14,0x15,
+               0x00,0x00,0x00,0x0A,    0x21,0x22,0x23,0x24,0x25,0x26,0x27,0x28,0x29,0x02A,
        );
        char buf[m.len];
-       u_int16_t len;
+       u_int32_t len;
 
        lib->processor->set_threads(lib->processor, 4);
 

diff --git a/src/libcharon/plugins/vici/vici_socket.c b/src/libcharon/plugins/vici/vici_socket.c
index 5a0fc81ece097ba162ea6779a7e3bb3611a94036..05c23749767bfd74e893e7cc83ff034f03a4b117 100644 (file)
--- a/src/libcharon/plugins/vici/vici_socket.c
+++ b/src/libcharon/plugins/vici/vici_socket.c
@@ -95,11 +95,11 @@ typedef struct {
        /** bytes of length header sent/received */
        u_char hdrlen;
        /** bytes of length header */
-       char hdr[sizeof(u_int16_t)];
+       char hdr[sizeof(u_int32_t)];
        /** send/receive buffer on heap */
        chunk_t buf;
        /** bytes sent/received in buffer */
-       u_int16_t done;
+       u_int32_t done;
 } msg_buf_t;
 
 /**
@@ -397,6 +397,7 @@ CALLBACK(on_write, bool,
 static bool do_read(private_vici_socket_t *this, entry_t *entry,
                                        stream_t *stream)
 {
+       u_int32_t msglen;
        ssize_t len;
 
        /* assemble the length header first */
@@ -420,8 +421,15 @@ static bool do_read(private_vici_socket_t *this, entry_t *entry,
                entry->in.hdrlen += len;
                if (entry->in.hdrlen == sizeof(entry->in.hdr))
                {
+                       msglen = untoh32(entry->in.hdr);
+                       if (msglen > VICI_MESSAGE_SIZE_MAX)
+                       {
+                               DBG1(DBG_CFG, "vici message length %u exceeds %u bytes limit, "
+                                        "ignored", msglen, VICI_MESSAGE_SIZE_MAX);
+                               return FALSE;
+                       }
                        /* header complete, continue with data */
-                       entry->in.buf = chunk_alloc(untoh16(entry->in.hdr));
+                       entry->in.buf = chunk_alloc(msglen);
                }
        }
 
@@ -584,7 +592,7 @@ CALLBACK(enable_writer, job_requeue_t,
 METHOD(vici_socket_t, send_, void,
        private_vici_socket_t *this, u_int id, chunk_t msg)
 {
-       if (msg.len <= (u_int16_t)~0)
+       if (msg.len <= VICI_MESSAGE_SIZE_MAX)
        {
                entry_selector_t *sel;
                msg_buf_t *out;
@@ -596,7 +604,7 @@ METHOD(vici_socket_t, send_, void,
                        INIT(out,
                                .buf = msg,
                        );
-                       htoun16(out->hdr, msg.len);
+                       htoun32(out->hdr, msg.len);
 
                        array_insert(entry->out, ARRAY_TAIL, out);
                        if (array_count(entry->out) == 1)
@@ -619,7 +627,8 @@ METHOD(vici_socket_t, send_, void,
        }
        else
        {
-               DBG1(DBG_CFG, "vici message exceeds maximum size, discarded");
+               DBG1(DBG_CFG, "vici message size %zu exceeds maximum size of %u, "
+                        "discarded", msg.len, VICI_MESSAGE_SIZE_MAX);
                chunk_clear(&msg);
        }
 }

diff --git a/src/libcharon/plugins/vici/vici_socket.h b/src/libcharon/plugins/vici/vici_socket.h
index f591998554add7a1626f0dbf271b8f4ee4a92995..872783665928617a03a14851ea56cf9577707cfd 100644 (file)
--- a/src/libcharon/plugins/vici/vici_socket.h
+++ b/src/libcharon/plugins/vici/vici_socket.h
@@ -23,6 +23,11 @@
 
 #include <library.h>
 
+/**
+ * Maximum size of a single message exchanged.
+ */
+#define VICI_MESSAGE_SIZE_MAX (512 * 1024)
+
 typedef struct vici_socket_t vici_socket_t;
 
 /**