mirror of
https://github.com/RGBCube/serenity
synced 2025-05-16 18:35:07 +00:00
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.
This commit is contained in:
parent
d419a780ae
commit
e682967d7e
4 changed files with 25 additions and 5 deletions
|
@ -121,7 +121,12 @@ bool LocalServer::listen(const String& address)
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
auto socket_address = SocketAddress::local(address);
|
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));
|
rc = ::bind(m_fd, (const sockaddr*)&un, sizeof(un));
|
||||||
if (rc < 0) {
|
if (rc < 0) {
|
||||||
perror("bind");
|
perror("bind");
|
||||||
|
|
|
@ -111,6 +111,12 @@ bool Socket::connect(const SocketAddress& address)
|
||||||
|
|
||||||
sockaddr_un saddr;
|
sockaddr_un saddr;
|
||||||
saddr.sun_family = AF_LOCAL;
|
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());
|
strcpy(saddr.sun_path, address.to_string().characters());
|
||||||
|
|
||||||
m_destination_address = address;
|
m_destination_address = address;
|
||||||
|
|
|
@ -43,7 +43,7 @@ public:
|
||||||
Local
|
Local
|
||||||
};
|
};
|
||||||
|
|
||||||
SocketAddress() {}
|
SocketAddress() { }
|
||||||
SocketAddress(const IPv4Address& address)
|
SocketAddress(const IPv4Address& address)
|
||||||
: m_type(Type::IPv4)
|
: m_type(Type::IPv4)
|
||||||
, m_ipv4_address(address)
|
, 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);
|
ASSERT(type() == Type::Local);
|
||||||
sockaddr_un address;
|
sockaddr_un address;
|
||||||
address.sun_family = AF_LOCAL;
|
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());
|
strcpy(address.sun_path, m_local_address.characters());
|
||||||
return address;
|
return address;
|
||||||
}
|
}
|
||||||
|
|
|
@ -135,7 +135,12 @@ void Service::setup_socket()
|
||||||
}
|
}
|
||||||
|
|
||||||
auto socket_address = Core::SocketAddress::local(m_socket_path);
|
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));
|
int rc = bind(m_socket_fd, (const sockaddr*)&un, sizeof(un));
|
||||||
if (rc < 0) {
|
if (rc < 0) {
|
||||||
perror("bind");
|
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));
|
ASSERT(!m_accept_socket_connections || (!m_socket_path.is_null() && m_lazy && m_multi_instance));
|
||||||
// MultiInstance doesn't work with KeepAlive.
|
// MultiInstance doesn't work with KeepAlive.
|
||||||
ASSERT(!m_multi_instance || !m_keep_alive);
|
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()) {
|
if (!m_socket_path.is_null() && is_enabled()) {
|
||||||
auto socket_permissions_string = config.read_entry(name, "SocketPermissions", "0600");
|
auto socket_permissions_string = config.read_entry(name, "SocketPermissions", "0600");
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue