[components][sal] Fix SIOCGIFCONF buffer overflow

2 files changed, 116 insertions, 8 deletions

diff --git a/components/net/sal/src/sal_socket.c b/components/net/sal/src/sal_socket.c
index b420eb2957..509da973e2 100644
--- a/components/net/sal/src/sal_socket.c
+++ b/components/net/sal/src/sal_socket.c
@@ -1572,21 +1572,41 @@ int sal_ioctlsocket(int socket, long cmd, void *arg)
 
         case SIOCGIFCONF:
         {
-            struct ifconf *ifconf_tmp;
-            ifconf_tmp = (struct ifconf *)arg;
-            int count_size = 0;
+            const int sal_ifreq_size = (int)sizeof(struct sal_ifreq);
+            struct ifconf *ifconf_tmp = (struct ifconf *)arg;
+            char *ifc_buf = ifconf_tmp->ifc_ifcu.ifcu_buf;
+            int buffer_size = ifconf_tmp->ifc_len;
+            int copied_size = 0;
+
+            if (buffer_size < 0)
+            {
+                LOG_E("ifconfig: network interface device list buffer size error.\n");
+                rt_set_errno(EINVAL);
+                return -1;
+            }
 
             for (node = &(cur_netdev_list->list); node; node = rt_slist_next(node))
             {
                 struct sal_ifreq sal_ifreq_temp;
-                count_size++;
+                if (ifc_buf == RT_NULL)
+                {
+                    copied_size += sal_ifreq_size;
+                    continue;
+                }
+
+                if (buffer_size - copied_size < sal_ifreq_size)
+                {
+                    break;
+                }
+
+                rt_memset(&sal_ifreq_temp, 0, sizeof(struct sal_ifreq));
                 netdev = rt_list_entry(node, struct netdev, list);
                 rt_strcpy(sal_ifreq_temp.ifr_ifrn.ifrn_name, netdev->name);
-                rt_memcpy(ifconf_tmp->ifc_ifcu.ifcu_buf, &sal_ifreq_temp, sizeof(struct sal_ifreq));
-                ifconf_tmp->ifc_ifcu.ifcu_buf += sizeof(struct sal_ifreq);
+                rt_memcpy(ifc_buf, &sal_ifreq_temp, sizeof(struct sal_ifreq));
+                copied_size += sal_ifreq_size;
+                ifc_buf += sizeof(struct sal_ifreq);
             }
-            ifconf_tmp->ifc_len = sizeof(struct sal_ifreq) * count_size;
-            ifconf_tmp->ifc_ifcu.ifcu_buf = ifconf_tmp->ifc_ifcu.ifcu_buf - sizeof(struct sal_ifreq) * count_size;
+            ifconf_tmp->ifc_len = copied_size;
             return 0;
         }
         case SIOCGIFINDEX:
diff --git a/components/net/utest/tc_sal_socket.c b/components/net/utest/tc_sal_socket.c
index db6acd7b72..169272d2ac 100644
--- a/components/net/utest/tc_sal_socket.c
+++ b/components/net/utest/tc_sal_socket.c
@@ -61,6 +61,16 @@
 static char test_send_data[] = "Hello, RT-Thread SAL!";
 static char test_recv_buffer[TEST_BUFFER_SIZE];
 
+struct sal_test_ifconf
+{
+    int ifc_len;
+    union
+    {
+        char *ifcu_buf;
+        struct sal_ifreq *ifcu_req;
+    } ifc_ifcu;
+};
+
 /* Local IP for tests (fallback to loopback) */
 static char local_ip[16] = "127.0.0.1";
 
@@ -164,6 +174,83 @@ static void close_test_socket(int sock)
     }
 }
 
+static void TC_sal_socket_siocgifconf(void)
+{
+    struct sal_test_ifconf ifconf;
+    struct sal_ifreq *ifreq;
+    unsigned char buffer[sizeof(struct sal_ifreq)];
+    char *original_buffer;
+    size_t index;
+    size_t name_length;
+    int sock;
+    int ret;
+
+    sock = create_test_socket(AF_INET, SOCK_DGRAM, 0);
+    uassert_true(sock >= 0);
+    if (sock < 0)
+    {
+        return;
+    }
+
+    /* A zero-sized destination must not be written. */
+    memset(buffer, 0xA5, sizeof(buffer));
+    ifconf.ifc_len = 0;
+    ifconf.ifc_ifcu.ifcu_buf = (char *)buffer;
+    ret = sal_ioctlsocket(sock, SIOCGIFCONF, &ifconf);
+    uassert_int_equal(ret, 0);
+    uassert_int_equal(ifconf.ifc_len, 0);
+    for (index = 0; index < sizeof(buffer); index++)
+    {
+        uassert_int_equal(buffer[index], 0xA5);
+    }
+
+    /* A NULL destination queries the required buffer size. */
+    ifconf.ifc_len = sizeof(buffer);
+    ifconf.ifc_ifcu.ifcu_buf = RT_NULL;
+    ret = sal_ioctlsocket(sock, SIOCGIFCONF, &ifconf);
+    uassert_int_equal(ret, 0);
+    uassert_true(ifconf.ifc_len >= sizeof(struct sal_ifreq));
+    uassert_int_equal(ifconf.ifc_len % sizeof(struct sal_ifreq), 0);
+
+    /* A negative buffer length must be rejected without writing. */
+    memset(buffer, 0xA5, sizeof(buffer));
+    ifconf.ifc_len = -1;
+    ifconf.ifc_ifcu.ifcu_buf = (char *)buffer;
+    ret = sal_ioctlsocket(sock, SIOCGIFCONF, &ifconf);
+    uassert_int_equal(rt_get_errno(), EINVAL);
+    uassert_int_equal(ret, -1);
+    for (index = 0; index < sizeof(buffer); index++)
+    {
+        uassert_int_equal(buffer[index], 0xA5);
+    }
+
+    /* Returned bytes outside the interface name must be initialized. */
+    memset(buffer, 0xA5, sizeof(buffer));
+    original_buffer = (char *)buffer;
+    ifconf.ifc_len = sizeof(buffer);
+    ifconf.ifc_ifcu.ifcu_buf = original_buffer;
+    ret = sal_ioctlsocket(sock, SIOCGIFCONF, &ifconf);
+    uassert_int_equal(ret, 0);
+    uassert_int_equal(ifconf.ifc_len, sizeof(struct sal_ifreq));
+    uassert_true(ifconf.ifc_ifcu.ifcu_buf == original_buffer);
+
+    ifreq = (struct sal_ifreq *)buffer;
+    for (name_length = 0; name_length < IFNAMSIZ; name_length++)
+    {
+        if (ifreq->ifr_ifrn.ifrn_name[name_length] == '\0')
+        {
+            break;
+        }
+    }
+    uassert_true(name_length > 0 && name_length < IFNAMSIZ);
+    for (index = name_length + 1; index < sizeof(*ifreq); index++)
+    {
+        uassert_int_equal(buffer[index], 0);
+    }
+
+    close_test_socket(sock);
+}
+
 /* Server thread function */
 static void server_thread_entry(void *parameter)
 {
@@ -1021,6 +1108,7 @@ static void utest_do_tc(void)
     UTEST_UNIT_RUN(TC_sal_socket_send_recv);
     UTEST_UNIT_RUN(TC_sal_socket_udp_communication);
     UTEST_UNIT_RUN(TC_sal_socket_getpeername_getsockname);
+    UTEST_UNIT_RUN(TC_sal_socket_siocgifconf);
     UTEST_UNIT_RUN(TC_sal_socket_close);
 
     LOG_I("===========================================");