Use the correct parameters for list_add_tail when enqueueing a message
in the tx_queue. Previously, list_add_tail(list, entry) was used instead
of list_add_tail(entry, list). Due to this, the list would only contain
the latest entry, and previously inserted entries were left dangling.
Reset the tx_queue offset after a message from the tx_queue has been sent
completely.
Signed-off-by: Alexander Van Parys <alexander.vanparys_ext@softathome.com>
A bad client can send a message whose blob_attr len is less than 4,
and ubus_msg_new happily points ->data off the end of the allocated
buffer, leading to invalid reads, writes, and eventually a crash if
ubus monitor is running:
==17683== Invalid write of size 4
==17683== at 0x10A915: client_cb (ubusd_main.c:143)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683== Address 0x4a63200 is 0 bytes after a block of size 32 alloc'd
==17683== at 0x4837B65: calloc (vg_replace_malloc.c:752)
==17683== by 0x10AA87: ubus_msg_new (ubusd.c:47)
==17683== by 0x10A8CE: client_cb (ubusd_main.c:135)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683==
==17683== Invalid read of size 4
==17683== at 0x10A645: blob_len (blob.h:102)
==17683== by 0x10A93D: blob_raw_len (blob.h:111)
==17683== by 0x10A93D: client_cb (ubusd_main.c:149)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683== Address 0x4a63200 is 0 bytes after a block of size 32 alloc'd
==17683== at 0x4837B65: calloc (vg_replace_malloc.c:752)
==17683== by 0x10AA87: ubus_msg_new (ubusd.c:47)
==17683== by 0x10A8CE: client_cb (ubusd_main.c:135)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683==
==17683== Invalid read of size 4
==17683== at 0x10ACE8: blob_len (blob.h:102)
==17683== by 0x10B7E1: blob_raw_len (blob.h:111)
==17683== by 0x10B7E1: ubusd_proto_receive_message (ubusd_proto.c:457)
==17683== by 0x10A9A7: client_cb (ubusd_main.c:169)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683== Address 0x4a63200 is 0 bytes after a block of size 32 alloc'd
==17683== at 0x4837B65: calloc (vg_replace_malloc.c:752)
==17683== by 0x10AA87: ubus_msg_new (ubusd.c:47)
==17683== by 0x10A8CE: client_cb (ubusd_main.c:135)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683==
==17683== Invalid read of size 4
==17683== at 0x10D39B: blob_len (blob.h:102)
==17683== by 0x10D53E: ubusd_monitor_message (ubusd_monitor.c:91)
==17683== by 0x10A99C: client_cb (ubusd_main.c:168)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683== Address 0x4a6b3e0 is 0 bytes after a block of size 32 alloc'd
==17683== at 0x4837B65: calloc (vg_replace_malloc.c:752)
==17683== by 0x10AA87: ubus_msg_new (ubusd.c:47)
==17683== by 0x10A8CE: client_cb (ubusd_main.c:135)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683==
==17683== Invalid read of size 1
==17683== at 0x4848286: blob_put (blob.c:167)
==17683== by 0x10D555: ubusd_monitor_message (ubusd_monitor.c:91)
==17683== by 0x10A99C: client_cb (ubusd_main.c:168)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683== Address 0x4a6b3e4 is 4 bytes after a block of size 32 alloc'd
==17683== at 0x4837B65: calloc (vg_replace_malloc.c:752)
==17683== by 0x10AA87: ubus_msg_new (ubusd.c:47)
==17683== by 0x10A8CE: client_cb (ubusd_main.c:135)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
==17683==
==17683==
==17683== Process terminating with default action of signal 11 (SIGSEGV)
==17683== Bad permissions for mapped region at address 0x4E43000
==17683== at 0x4848286: blob_put (blob.c:167)
==17683== by 0x10D555: ubusd_monitor_message (ubusd_monitor.c:91)
==17683== by 0x10A99C: client_cb (ubusd_main.c:168)
==17683== by 0x48495E3: uloop_run_events (uloop.c:198)
==17683== by 0x48495E3: uloop_run_timeout (uloop.c:555)
==17683== by 0x10A503: uloop_run (uloop.h:111)
==17683== by 0x10A503: main (ubusd_main.c:284)
The following Python program minimally reproduces the issue:
import socket
sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
sock.connect('/tmp/usock')
sock.recv(12)
sock.send(b'\x00\x04\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00')
Signed-off-by: Julian Squires <julian@cipht.net>
No new message can be enqueued if this brings the total queue length of
that client over UBUS_CLIENT_MAX_TXQ_LEN.
Set UBUS_CLIENT_MAX_TXQ_LEN to UBUS_MAX_MSGLEN, i.e. 1MB. This limit
should be plenty for any practical use case.
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
ubusd maintains a per-client tx_queue containing references to message
buffers that have not been sent yet (due to the socket blocking). This
is a fixed-size, 64-element queue.
When more than 64 elements are queued, subsequent elements are simply
dropped. Thus, a client that is waiting for those messages will block
indefinitely. In particular, this happens when more than +- 250 objects
are registered on the bus and either "ubus list" or "ubus wait_for" is
called. The responses to these requests consist of a message buffer per
object. Since in practice, ubusd will not yield between the sends of
these message buffers, the client has no time to process them and
eventually the output socket blocks. After 64 more objects, the rest is
dropped, including the final message that indicates termination. Thus,
the client waits indefinitely for the termination message.
To solve this, turn the tx_queue into a variable-sized linked list
instead of a fixed-size queue.
To maintain the linked list, an additional structure ubus_msg_buf_list
is created. It is not possible to add the linked list to ubus_msg_buf,
because that is shared between clients.
Note that this infinite tx_queue opens the door to a DoS attack. You can
open a client and a server connection, then send messages from the
client to the server without ever reading anything on the server side.
This will eventually lead to an out-of-memory. However, such a DoS
already existed anyway, it just requires opening multiple server
connections and filling up the fixed-size queue on each one. To protect
against such DoS attacks, we'd need to:
- keep a global maximum queue size that applies to all rx and tx queues
together;
- stop reading from any connection when the maximum is reached;
- close any connection when it hasn't become writeable after some
timeout.
Fixes: https://bugs.openwrt.org/index.php?do=details&task_id=1525
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
When ubus is running as root, /var/run/ubus most likely hasn't been
created as well (as that's the homedir of user ubus, and if a user
ubus was found, then ubus would run being that user).
Blindly attempt to create the directory (which won't do any harm if
it does exist and/or ubus is not running as root) to still be able to
start ubus in case of user ubus not existing.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
In commit 5d7ca8309d ("ubusd/libubus-io: fix variable sized struct
position warning") the position of cmsghdr struct has been changed in
order to fix clang-9 compiler warning, but it has introduced regression
in at least `logread` which hanged indefinitely.
So this patch reworks the socket descriptor passing in a way recommended
in the `cmsg(3)` manual page.
Ref: http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020840.html
Fixes: 5d7ca8309d ("ubusd/libubus-io: fix variable sized struct position warning")
Reported-by: Hannu Nyman <hannu.nyman@welho.com>
Signed-off-by: Petr Štetiar <ynezz@true.cz>
