oss-security - Bugs in iscsiuio

Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20171213192155.GA21805@localhost.localdomain>
Date: Wed, 13 Dec 2017 11:21:55 -0800
From: Qualys Security Advisory <qsa@...lys.com>
To: oss-security@...ts.openwall.com
Subject: Bugs in iscsiuio

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.)

========================================================================
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.