Bugs in iscsiuio

[Qualys Security Advisory <qsa () qualys com>]

Hi all,

We took a quick look at iscsiuio (from the iscsi-initiator-utils),
discovered several bugs, and sent a brief report of our findings to
linux-distros (on Monday, December 11). It was then decided that we
should send this report to oss-security on Wednesday, December 13:
please see below.

Notes: we did not have the time to draft a proper advisory, so this is
rather raw material, but hopefully it will be useful and detailed
enough; also, we did not fully investigate the exploitability (or
unexploitability) of these issues; and this was by no means an
exhaustive audit, so there may be more bugs to be found.

Thank you very much! With best regards,

-- the Qualys Security Advisory team

========================================================================

Source: iscsi-initiator-utils-6.2.0.874-6.git86e8892.fc27.src.rpm

------------------------------------------------------------------------

In iscsid_loop(), new connections are accepted on the abstract socket
@ISCSID_UIP_ABSTRACT_NAMESPACE, but the source of the connection is not
checked: any local user can connect and send messages, thus triggering
the bugs below; this may or may not be the intended behavior. (iscsid
for example processes only root's messages, in mgmt_ipc_handle()).

------------------------------------------------------------------------

In process_iscsid_broadcast(), payload_len is read directly from the
user's message, without checks, and then used in an fread() to a fixed-
size buffer: a straight heap overflow (arbitrary size, arbitrary bytes).
This would probably be exploitable, but every distribution we checked
compiles iscsiuio with FORTIFY, and __fread_chk() catches the heap
overflow before it happens, and aborts:

$ echo -e '\1\0\0\0\x41\x41\x41\x41' | socat - ABSTRACT-CONNECT:ISCSID_UIP_ABSTRACT_NAMESPACE

Process 32442 (iscsiuio) of user 0 dumped core.

Stack trace of thread 32445:
#0  0x00007f15redacted raise (libc.so.6)
#1  0x00007f15redacted abort (libc.so.6)
#2  0x00007f15redacted __libc_message (libc.so.6)
#3  0x00007f15redacted __fortify_fail (libc.so.6)
#4  0x00007f15redacted __chk_fail (libc.so.6)
#5  0x00007f15redacted __fread_chk (libc.so.6)
#6  0x0000555dredacted process_iscsid_broadcast (iscsiuio)
#7  0x0000555dredacted iscsid_loop (iscsiuio)
#8  0x00007f15redacted start_thread (libpthread.so.0)
#9  0x00007f15redacted __clone (libc.so.6)

Stack trace of thread 32443:
#0  0x00007f15redacted sigwait (libpthread.so.0)
#1  0x0000555dredacted signal_handle_thread (iscsiuio)
#2  0x00007f15redacted start_thread (libpthread.so.0)
#3  0x00007f15redacted __clone (libc.so.6)

Stack trace of thread 32442:
#0  0x00007f15redacted socket (libc.so.6)
#1  0x0000555dredacted nic_nl_open (iscsiuio)
#2  0x0000555dredacted main (iscsiuio)
#3  0x00007f15redacted __libc_start_main (libc.so.6)
#4  0x0000555dredacted _start (iscsiuio)

------------------------------------------------------------------------

In process_iscsid_broadcast(), rsp is not initialized, and the
ISCSID_UIP_IPC_GET_IFACE and default cases do not initialize its ping_sc
member: because rsp is then sent back to the connected user, this may
leak useful information to an attacker.

------------------------------------------------------------------------

At the end of process_iscsid_broadcast() there is an fclose(fd) where fd
is fdopen(s2); but after the return, iscsid_loop() calls close(s2): this
double-closes s2, which may not always be a problem, but here, iscsiuio
is multi-threaded and it is theoretically possible that another thread
recycles the file descriptor s2 after the fclose(fd) but before the
close(s2), which would therefore close the wrong file.

Note: at the beginning of process_iscsid_broadcast() there is a return
that does not close s2; care must be taken with the double-close patch,
or it may leak a file descriptor instead.

------------------------------------------------------------------------

At the beginning of decode_cidr(), where in_ipaddr_str is from the
message sent by the user/attacker:

        char                    ipaddress[NI_MAXHOST];

the following code has two problems:

        char ipaddr_str[NI_MAXHOST];
        ...
        if (strlen(in_ipaddr_str) > NI_MAXHOST)
                strncpy(ipaddr_str, in_ipaddr_str, NI_MAXHOST);
        else
                strcpy(ipaddr_str, in_ipaddr_str);

1/ strlen(in_ipaddr_str) > NI_MAXHOST is possible, because the user's
ipaddress is not necessarily null-terminated, but in that case strncpy()
does not null-terminate ipaddr_str;

Note: other "strings" in the message sent by the user/attacker also
suffer from this (non) null-termination problem.

2/ if strlen(in_ipaddr_str) == NI_MAXHOST, the strcpy() triggers an
off-by-one overflow.

Note: this strncpy()/strcpy() pattern of bugs is repeated many times
throughout the codebase.

------------------------------------------------------------------------

In decode_cidr(), the strcpy(ipaddr_str, tok) is invalid, because tok
points to ipaddr_str, and "strings may not overlap" (man strcpy).

------------------------------------------------------------------------

In decode_cidr(), int keepbits = atoi(tmp) may be negative (it comes
from the message sent by the user/attacker), which may bypass checks and
have other consequences in other parts of the code.

------------------------------------------------------------------------

In perform_ping(), int datalen comes from the message sent by the
user/attacker, but it is not checked at all: it looks like this could
lead to a heap overflow in fill_payload_data() (through
do_ping_from_nic_iface() -> prepare_icmpv6_req_pkt()).

(It seems that packet buffers are malloc()ated by alloc_packet(), called
only with alloc_packet(1500, 1500), and uip_slen in fill_payload_data()
is an u16_t, originally from the attacker-controlled int datalen, so it
may very well overflow this 1500-byte buffer -- but we did not double-
check this conclusion, and the lack of datalen checks may have other
consequences as well, but we did not explore them fully.)

========================================================================