From e682967d7eb4bff978b011b03a6bf4b939745d1c Mon Sep 17 00:00:00 2001
From: Ben Wiederhake <BenWiederhake.GitHub@gmx.de>
Date: Sun, 23 Aug 2020 13:47:52 +0200
Subject: [PATCH] LibCore: Prefer strlcpy over strncpy, fix overflow

A malicious caller can create a SocketAddress for a local unix socket with an
over-long name that does not fit into struct sock_addr_un.
- Socket::connet: This caused the 'sun_path' field to
  overflow, probably overwriting the return pointer of the call frame, and thus
  crashing the process (in the best case).
- SocketAddress::to_sockaddr_un: This triggered a RELEASE_ASSERT, and thus
  crashing the process.

Both have been fixed to return a nice error code instead of crashing.
---
 Libraries/LibCore/LocalServer.cpp | 7 ++++++-
 Libraries/LibCore/Socket.cpp      | 6 ++++++
 Libraries/LibCore/SocketAddress.h | 8 +++++---
 Services/SystemServer/Service.cpp | 9 ++++++++-
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/Libraries/LibCore/LocalServer.cpp b/Libraries/LibCore/LocalServer.cpp
index ceadadfdb3..8b41ddcf21 100644
--- a/Libraries/LibCore/LocalServer.cpp
+++ b/Libraries/LibCore/LocalServer.cpp
@@ -121,7 +121,12 @@ bool LocalServer::listen(const String& address)
 #endif
 
     auto socket_address = SocketAddress::local(address);
-    auto un = socket_address.to_sockaddr_un();
+    auto un_optional = socket_address.to_sockaddr_un();
+    if (!un_optional.has_value()) {
+        perror("bind");
+        return false;
+    }
+    auto un = un_optional.value();
     rc = ::bind(m_fd, (const sockaddr*)&un, sizeof(un));
     if (rc < 0) {
         perror("bind");
diff --git a/Libraries/LibCore/Socket.cpp b/Libraries/LibCore/Socket.cpp
index a12c57993a..6685839a27 100644
--- a/Libraries/LibCore/Socket.cpp
+++ b/Libraries/LibCore/Socket.cpp
@@ -111,6 +111,12 @@ bool Socket::connect(const SocketAddress& address)
 
     sockaddr_un saddr;
     saddr.sun_family = AF_LOCAL;
+    auto dest_address = address.to_string();
+    if (dest_address.length() >= sizeof(saddr.sun_path)) {
+        fprintf(stderr, "Core::Socket: Failed to connect() to %s: Path is too long!\n", dest_address.characters());
+        errno = EINVAL;
+        return false;
+    }
     strcpy(saddr.sun_path, address.to_string().characters());
 
     m_destination_address = address;
diff --git a/Libraries/LibCore/SocketAddress.h b/Libraries/LibCore/SocketAddress.h
index 621687fe13..1ffa3da24f 100644
--- a/Libraries/LibCore/SocketAddress.h
+++ b/Libraries/LibCore/SocketAddress.h
@@ -43,7 +43,7 @@ public:
         Local
     };
 
-    SocketAddress() {}
+    SocketAddress() { }
     SocketAddress(const IPv4Address& address)
         : m_type(Type::IPv4)
         , m_ipv4_address(address)
@@ -82,12 +82,14 @@ public:
         }
     }
 
-    sockaddr_un to_sockaddr_un() const
+    Optional<sockaddr_un> to_sockaddr_un() const
     {
         ASSERT(type() == Type::Local);
         sockaddr_un address;
         address.sun_family = AF_LOCAL;
-        RELEASE_ASSERT(m_local_address.length() < (int)sizeof(address.sun_path));
+        if (m_local_address.length() >= sizeof(address.sun_path)) {
+            return {};
+        }
         strcpy(address.sun_path, m_local_address.characters());
         return address;
     }
diff --git a/Services/SystemServer/Service.cpp b/Services/SystemServer/Service.cpp
index cb89033db2..f581c0c83d 100644
--- a/Services/SystemServer/Service.cpp
+++ b/Services/SystemServer/Service.cpp
@@ -135,7 +135,12 @@ void Service::setup_socket()
     }
 
     auto socket_address = Core::SocketAddress::local(m_socket_path);
-    auto un = socket_address.to_sockaddr_un();
+    auto un_optional = socket_address.to_sockaddr_un();
+    if (!un_optional.has_value()) {
+        dbg() << "Socket name " << m_socket_path << " is too long. BUG! This should have failed earlier!";
+        ASSERT_NOT_REACHED();
+    }
+    auto un = un_optional.value();
     int rc = bind(m_socket_fd, (const sockaddr*)&un, sizeof(un));
     if (rc < 0) {
         perror("bind");
@@ -358,6 +363,8 @@ Service::Service(const Core::ConfigFile& config, const StringView& name)
     ASSERT(!m_accept_socket_connections || (!m_socket_path.is_null() && m_lazy && m_multi_instance));
     // MultiInstance doesn't work with KeepAlive.
     ASSERT(!m_multi_instance || !m_keep_alive);
+    // Socket path (plus NUL) must fit into the structs sent to the Kernel.
+    ASSERT(m_socket_path.length() < UNIX_PATH_MAX);
 
     if (!m_socket_path.is_null() && is_enabled()) {
         auto socket_permissions_string = config.read_entry(name, "SocketPermissions", "0600");