From 47404b2a0402cf359e1afdb32210ce366a2a805d Mon Sep 17 00:00:00 2001 From: Jason Gauci Date: Wed, 29 Jul 2026 16:57:06 -0500 Subject: [PATCH] Fix four pre-auth and privilege-escalation issues from security notices. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address ANT-2026-5PETM5BV, ANT-2026-VAMER5RC, ANT-2026-AVTT7HQH, and ANT-2026-A3WQS3AG without bumping PROTOCOL_VERSION. ANT-2026-5PETM5BV (pre-auth DoS): The fixed 8-thread client handler pool read ConnectRequest via readProto, which trusted a client length up to 128 MiB and allocated up front. readAll() also reset its 30s idle timer on every byte, so a slow trickle could hold a worker forever. Cap handshake protos at 4 KiB, and enforce both idle and absolute (60s) deadlines in readAll—checking the absolute deadline every loop iteration so a steady trickle cannot bypass it. ANT-2026-VAMER5RC (pre-auth reconnect crash / disconnect): Returning clients were accepted on cleartext clientId alone. recoverClient() closed the live victim socket before reading a plaintext SequenceHeader, and BackedWriter::recover() used STFATAL/LOG(FATAL) when the attacker-supplied sequence was ahead of the server—aborting the root daemon. Treat a bad sequence as a caught runtime_error, and only close the old socket after recover succeeds so a failed reconnect leaves the session intact. ANT-2026-AVTT7HQH / ANT-2026-A3WQS3AG (unix-socket LPE as root): etserver never drops privileges, so reverse-tunnel sources ran root unlink/bind/ chmod/chown on client-chosen paths (arbitrary file delete + chown TOCTOU), and forward destinations connected as root to arbitrary AF_UNIX paths (e.g. docker.sock). Add UserSocketOps: fork, setgroups/setgid/setuid to the session user, perform listen/connect, and return the fd via SCM_RIGHTS. PortForwardHandler uses this for unix source listen and destination connect; path-based chown after bind is removed. Unsolved without a PROTOCOL_VERSION bump: reconnect still does not prove passkey knowledge before recover. ConnectRequest carries only clientId and version; adding challenge-response (or encrypting the recover handshake) would break old clients that must match PROTOCOL_VERSION exactly. Residual risk: an on-path observer who sniffs a live clientId and supplies an acceptable sequence number can still displace that session's TCP connection and force-disconnect the victim. They cannot speak the encrypted session without the passkey, and they can no longer crash the daemon with a crafted sequence. Full reconnect authentication remains a coordinated future protocol change. Add SecurityNoticesTest and UserSocketOps coverage for each fixed case. Co-authored-by: Cursor --- CMakeLists.txt | 2 + src/base/BackedWriter.cpp | 4 +- src/base/Connection.cpp | 3 +- src/base/PipeSocketHandler.cpp | 43 ++ src/base/PipeSocketHandler.hpp | 12 + src/base/ServerClientConnection.cpp | 33 +- src/base/ServerClientConnection.hpp | 4 +- src/base/ServerConnection.cpp | 4 +- src/base/SocketHandler.cpp | 31 +- src/base/SocketHandler.hpp | 41 +- src/base/UserSocketOps.cpp | 239 ++++++++++ src/base/UserSocketOps.hpp | 41 ++ src/terminal/TerminalServer.cpp | 4 +- .../forwarding/ForwardSourceHandler.cpp | 6 +- .../forwarding/ForwardSourceHandler.hpp | 8 +- .../forwarding/PortForwardHandler.cpp | 41 +- .../forwarding/PortForwardHandler.hpp | 15 +- test/unit_tests/BackedIOTest.cpp | 34 ++ test/unit_tests/ClientConnectionTest.cpp | 33 ++ test/unit_tests/SecurityNoticesTest.cpp | 446 ++++++++++++++++++ test/unit_tests/UserSocketOpsTest.cpp | 58 +++ 21 files changed, 1067 insertions(+), 35 deletions(-) create mode 100644 src/base/UserSocketOps.cpp create mode 100644 src/base/UserSocketOps.hpp create mode 100644 test/unit_tests/SecurityNoticesTest.cpp create mode 100644 test/unit_tests/UserSocketOpsTest.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index a0e29a0f8..0dc5b0f1b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -430,6 +430,8 @@ add_library( src/base/SocketHandler.cpp src/base/PipeSocketHandler.hpp src/base/PipeSocketHandler.cpp + src/base/UserSocketOps.hpp + src/base/UserSocketOps.cpp src/base/TcpSocketHandler.hpp src/base/TcpSocketHandler.cpp src/base/UnixSocketHandler.hpp diff --git a/src/base/BackedWriter.cpp b/src/base/BackedWriter.cpp index dbbace1e5..1de25d5f1 100644 --- a/src/base/BackedWriter.cpp +++ b/src/base/BackedWriter.cpp @@ -91,7 +91,9 @@ vector BackedWriter::recover(int64_t lastValidSequenceNumber) { int64_t messagesToRecover = sequenceNumber - lastValidSequenceNumber; if (messagesToRecover < 0) { - STFATAL << "Something went really wrong, client is ahead of server"; + // Attacker-controlled sequence numbers must not abort the process. + throw std::runtime_error( + "Invalid recovery sequence: client is ahead of server"); } if (messagesToRecover == 0) { return vector(); diff --git a/src/base/Connection.cpp b/src/base/Connection.cpp index 3b7a23ff6..eaec4408d 100644 --- a/src/base/Connection.cpp +++ b/src/base/Connection.cpp @@ -118,7 +118,8 @@ bool Connection::recover(int newSocketFd) { // Read the remote sequence number et::SequenceHeader remoteHeader = - socketHandler->readProto(newSocketFd, true); + socketHandler->readProto( + newSocketFd, true, SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH); { // Fetch the catchup bytes and send diff --git a/src/base/PipeSocketHandler.cpp b/src/base/PipeSocketHandler.cpp index 509a05d65..d856bd59e 100644 --- a/src/base/PipeSocketHandler.cpp +++ b/src/base/PipeSocketHandler.cpp @@ -1,5 +1,9 @@ #include "PipeSocketHandler.hpp" +#ifndef WIN32 +#include "UserSocketOps.hpp" +#endif + namespace et { PipeSocketHandler::PipeSocketHandler() {} @@ -90,6 +94,24 @@ int PipeSocketHandler::connect(const SocketEndpoint& endpoint) { return sockFd; } +#ifndef WIN32 +int PipeSocketHandler::connectAsUser(const SocketEndpoint& endpoint, uid_t uid, + gid_t gid) { + lock_guard mutexGuard(globalMutex); + + string pipePath = endpoint.name(); + VLOG(3) << "Connecting to " << endpoint << " as uid " << uid; + int sockFd = UserSocketOps::connectUnixAsUser(pipePath, uid, gid); + if (sockFd < 0) { + return -1; + } + initSocket(sockFd); + addToActiveSockets(sockFd); + LOG(INFO) << "Connected to endpoint " << endpoint << " as uid " << uid; + return sockFd; +} +#endif + set PipeSocketHandler::listen(const SocketEndpoint& endpoint) { lock_guard guard(globalMutex); @@ -117,6 +139,27 @@ set PipeSocketHandler::listen(const SocketEndpoint& endpoint) { return pipeServerSockets[pipePath]; } +#ifndef WIN32 +set PipeSocketHandler::listenAsUser(const SocketEndpoint& endpoint, + uid_t uid, gid_t gid) { + lock_guard guard(globalMutex); + + string pipePath = endpoint.name(); + if (pipeServerSockets.find(pipePath) != pipeServerSockets.end()) { + throw runtime_error("Tried to listen twice on the same path"); + } + + int fd = UserSocketOps::listenUnixAsUser(pipePath, uid, gid); + if (fd < 0) { + throw runtime_error(string("Failed to listen as user on ") + pipePath + + ": " + strerror(GetErrno())); + } + initServerSocket(fd); + pipeServerSockets[pipePath] = set({fd}); + return pipeServerSockets[pipePath]; +} +#endif + set PipeSocketHandler::getEndpointFds(const SocketEndpoint& endpoint) { lock_guard guard(globalMutex); diff --git a/src/base/PipeSocketHandler.hpp b/src/base/PipeSocketHandler.hpp index 2afb99e55..8fe547944 100644 --- a/src/base/PipeSocketHandler.hpp +++ b/src/base/PipeSocketHandler.hpp @@ -17,10 +17,22 @@ class PipeSocketHandler : public UnixSocketHandler { * @brief Connects to a pipe identified by the endpoint name. */ virtual int connect(const SocketEndpoint& endpoint); +#ifndef WIN32 + /** + * @brief Connects to a UNIX socket after dropping to @p uid/@p gid. + */ + int connectAsUser(const SocketEndpoint& endpoint, uid_t uid, gid_t gid); +#endif /** * @brief Creates a listening UNIX socket and stores it internally. */ virtual set listen(const SocketEndpoint& endpoint); +#ifndef WIN32 + /** + * @brief Creates a listening UNIX socket after dropping to @p uid/@p gid. + */ + set listenAsUser(const SocketEndpoint& endpoint, uid_t uid, gid_t gid); +#endif /** * @brief Returns the listening fds for a previously registered pipe. */ diff --git a/src/base/ServerClientConnection.cpp b/src/base/ServerClientConnection.cpp index 32883eabc..3005f6491 100644 --- a/src/base/ServerClientConnection.cpp +++ b/src/base/ServerClientConnection.cpp @@ -25,13 +25,40 @@ ServerClientConnection::~ServerClientConnection() { } bool ServerClientConnection::recoverClient(int newSocketFd) { + // Detach the live session without closing it until recover succeeds, so a + // failed/malicious reconnect cannot force-disconnect the victim. + int oldSocketFd = -1; { lock_guard guard(connectionMutex); - if (socketFd != -1) { - closeSocket(); + oldSocketFd = socketFd; + if (reader) { + reader->invalidateSocket(); + } + if (writer) { + writer->invalidateSocket(); + } + socketFd = -1; + } + + bool success = recover(newSocketFd); + if (success) { + if (oldSocketFd != -1) { + socketHandler->close(oldSocketFd); + } + return true; + } + + if (oldSocketFd != -1) { + lock_guard guard(connectionMutex); + socketFd = oldSocketFd; + if (reader) { + reader->revive(oldSocketFd, vector()); + } + if (writer) { + writer->revive(oldSocketFd); } } - return recover(newSocketFd); + return false; } bool ServerClientConnection::verifyPasskey(const string& targetKey) { diff --git a/src/base/ServerClientConnection.hpp b/src/base/ServerClientConnection.hpp index 201aacd26..72a7eb693 100644 --- a/src/base/ServerClientConnection.hpp +++ b/src/base/ServerClientConnection.hpp @@ -20,8 +20,8 @@ class ServerClientConnection : public Connection { virtual ~ServerClientConnection(); /** - * @brief Tears down the old socket (if any) and attempts recovery on the new - * fd. + * @brief Attempts recovery on the new fd; closes the old socket only after + * recover succeeds. */ bool recoverClient(int newSocketFd); diff --git a/src/base/ServerConnection.cpp b/src/base/ServerConnection.cpp index 95aeee4a2..e91d4bb13 100644 --- a/src/base/ServerConnection.cpp +++ b/src/base/ServerConnection.cpp @@ -41,8 +41,8 @@ void ServerConnection::clientHandler(int clientSocketFd) { string clientId; bool createdClientConnection = false; try { - et::ConnectRequest request = - socketHandler->readProto(clientSocketFd, true); + et::ConnectRequest request = socketHandler->readProto( + clientSocketFd, true, SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH); { int version = request.version(); if (version != PROTOCOL_VERSION) { diff --git a/src/base/SocketHandler.cpp b/src/base/SocketHandler.cpp index 7075a0242..2520ab068 100644 --- a/src/base/SocketHandler.cpp +++ b/src/base/SocketHandler.cpp @@ -3,17 +3,33 @@ #include "base64.h" namespace et { -#define SOCKET_DATA_TRANSFER_TIMEOUT (30) +#define SOCKET_DATA_TRANSFER_TIMEOUT (SocketHandler::SOCKET_IDLE_TIMEOUT_SEC) void SocketHandler::readAll(int fd, void* buf, size_t count, bool timeout) { - time_t startTime = time(NULL); + if (timeout) { + readAll(fd, buf, count, SOCKET_IDLE_TIMEOUT_SEC, + SOCKET_ABSOLUTE_TIMEOUT_SEC); + } else { + readAll(fd, buf, count, 0, 0); + } +} + +void SocketHandler::readAll(int fd, void* buf, size_t count, int idleTimeoutSec, + int absoluteTimeoutSec) { + time_t absoluteStartTime = time(NULL); + time_t idleStartTime = absoluteStartTime; size_t pos = 0; while (pos < count) { + // Enforce deadlines on every iteration so a slow trickle that keeps the + // idle timer reset cannot hold the read open past the absolute limit. + time_t currentTime = time(NULL); + if ((idleTimeoutSec > 0 && currentTime > idleStartTime + idleTimeoutSec) || + (absoluteTimeoutSec > 0 && + currentTime > absoluteStartTime + absoluteTimeoutSec)) { + throw std::runtime_error("Socket Timeout"); + } + if (!waitOnSocketData(fd)) { - time_t currentTime = time(NULL); - if (timeout && currentTime > startTime + SOCKET_DATA_TRANSFER_TIMEOUT) { - throw std::runtime_error("Socket Timeout"); - } continue; } @@ -36,7 +52,8 @@ void SocketHandler::readAll(int fd, void* buf, size_t count, bool timeout) { } } else { pos += bytesRead; - startTime = time(NULL); + // Only the idle timer resets on progress; absolute deadline does not. + idleStartTime = time(NULL); } } } diff --git a/src/base/SocketHandler.hpp b/src/base/SocketHandler.hpp index 232c32969..ed0857635 100644 --- a/src/base/SocketHandler.hpp +++ b/src/base/SocketHandler.hpp @@ -29,13 +29,27 @@ class SocketHandler { */ virtual ssize_t write(int fd, const void* buf, size_t count) = 0; + /** @brief Idle-gap timeout (seconds) used when `readAll(..., true)`. */ + static constexpr int SOCKET_IDLE_TIMEOUT_SEC = 30; + /** + * @brief Absolute read deadline (seconds) used when `readAll(..., true)`. + * Unlike the idle timeout, this is not reset when bytes arrive. + */ + static constexpr int SOCKET_ABSOLUTE_TIMEOUT_SEC = 60; + /** * @brief Reads exactly `count` bytes, retrying on EAGAIN until the buffer * fills. - * @param timeout Whether to enforce the internal transfer timeout while - * waiting. + * @param timeout Whether to enforce idle + absolute transfer timeouts. */ void readAll(int fd, void* buf, size_t count, bool timeout); + /** + * @brief Reads exactly `count` bytes with explicit deadlines. + * @param idleTimeoutSec Max seconds without any progress; 0 disables. + * @param absoluteTimeoutSec Max seconds for the whole read; 0 disables. + */ + void readAll(int fd, void* buf, size_t count, int idleTimeoutSec, + int absoluteTimeoutSec); /** * @brief Attempts to write the full buffer and returns -1 on timeout/failure. * @return Total bytes written or -1 when the socket deadlocks. @@ -47,6 +61,16 @@ class SocketHandler { */ void writeAllOrThrow(int fd, const void* buf, size_t count, bool timeout); + /** @brief Default max length for length-prefixed protobuf reads (128 MiB). */ + static constexpr int64_t DEFAULT_MAX_PROTO_LENGTH = 128 * 1024 * 1024; + /** + * @brief Max length for pre-authentication / handshake protos (4 KiB). + * + * ConnectRequest and SequenceHeader are tiny; a large declared length would + * pin memory on a handler thread before any auth. + */ + static constexpr int64_t MAX_HANDSHAKE_PROTO_LENGTH = 4 * 1024; + /** * @brief Reads a length-prefixed protobuf from the socket. * @tparam T Protobuf message type. @@ -54,13 +78,22 @@ class SocketHandler { */ template inline T readProto(int fd, bool timeout) { + return readProto(fd, timeout, DEFAULT_MAX_PROTO_LENGTH); + } + + /** + * @brief Reads a length-prefixed protobuf, rejecting lengths above @p + * maxLength. + */ + template + inline T readProto(int fd, bool timeout, int64_t maxLength) { T t; int64_t length; readAll(fd, &length, sizeof(int64_t), timeout); - if (length < 0 || length > 128 * 1024 * 1024) { + if (length < 0 || length > maxLength) { // If the message is <= 0 or too big, assume this is a bad packet and // throw - string s = string("Invalid size (<0 or >128 MB): ") + to_string(length); + string s = string("Invalid size (<0 or >max): ") + to_string(length); throw std::runtime_error(s.c_str()); } if (length == 0) { diff --git a/src/base/UserSocketOps.cpp b/src/base/UserSocketOps.cpp new file mode 100644 index 000000000..6937f0a78 --- /dev/null +++ b/src/base/UserSocketOps.cpp @@ -0,0 +1,239 @@ +#include "UserSocketOps.hpp" + +#ifndef WIN32 +#include +#include +#include +#include + +namespace et { +namespace { +struct ResultHeader { + int status; // 0 ok, -1 error + int err; +}; + +void fatalClose(int fd) { + if (fd >= 0) { + ::close(fd); + } +} +} // namespace + +void UserSocketOps::sendFd(int channel, int fdToSend, int status, int err) { + ResultHeader header{status, err}; + struct iovec iov; + iov.iov_base = &header; + iov.iov_len = sizeof(header); + + char control[CMSG_SPACE(sizeof(int))]; + memset(control, 0, sizeof(control)); + + struct msghdr msg; + memset(&msg, 0, sizeof(msg)); + msg.msg_iov = &iov; + msg.msg_iovlen = 1; + + if (status == 0 && fdToSend >= 0) { + msg.msg_control = control; + msg.msg_controllen = sizeof(control); + struct cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + cmsg->cmsg_len = CMSG_LEN(sizeof(int)); + memcpy(CMSG_DATA(cmsg), &fdToSend, sizeof(int)); + } + + // Best-effort; child exits immediately after. + ::sendmsg(channel, &msg, 0); +} + +int UserSocketOps::recvFd(int channel, int* errOut) { + ResultHeader header; + struct iovec iov; + iov.iov_base = &header; + iov.iov_len = sizeof(header); + + char control[CMSG_SPACE(sizeof(int))]; + memset(control, 0, sizeof(control)); + + struct msghdr msg; + memset(&msg, 0, sizeof(msg)); + msg.msg_iov = &iov; + msg.msg_iovlen = 1; + msg.msg_control = control; + msg.msg_controllen = sizeof(control); + + ssize_t n = ::recvmsg(channel, &msg, 0); + if (n != (ssize_t)sizeof(header)) { + if (errOut) { + *errOut = EIO; + } + return -1; + } + if (header.status != 0) { + if (errOut) { + *errOut = header.err ? header.err : EIO; + } + return -1; + } + + struct cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); + if (cmsg == nullptr || cmsg->cmsg_level != SOL_SOCKET || + cmsg->cmsg_type != SCM_RIGHTS || cmsg->cmsg_len < CMSG_LEN(sizeof(int))) { + if (errOut) { + *errOut = EIO; + } + return -1; + } + int fd = -1; + memcpy(&fd, CMSG_DATA(cmsg), sizeof(int)); + if (errOut) { + *errOut = 0; + } + return fd; +} + +void UserSocketOps::childListen(int resultFd, const string& path) { + if (path.size() >= sizeof(sockaddr_un::sun_path)) { + sendFd(resultFd, -1, -1, ENAMETOOLONG); + _exit(1); + } + + int fd = ::socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) { + sendFd(resultFd, -1, -1, errno); + _exit(1); + } + + sockaddr_un local; + memset(&local, 0, sizeof(local)); + local.sun_family = AF_UNIX; + strncpy(local.sun_path, path.c_str(), sizeof(local.sun_path) - 1); + + // Only removes a path the dropped-privilege user can unlink. + ::unlink(local.sun_path); + + if (::bind(fd, (struct sockaddr*)&local, sizeof(local)) < 0) { + int err = errno; + fatalClose(fd); + sendFd(resultFd, -1, -1, err); + _exit(1); + } + if (::listen(fd, 5) < 0) { + int err = errno; + fatalClose(fd); + sendFd(resultFd, -1, -1, err); + _exit(1); + } + if (::fchmod(fd, S_IRUSR | S_IWUSR | S_IXUSR) < 0) { + // fchmod on unix sockets is unsupported on some platforms; fall back to + // path chmod. Still running as the session user. + ::chmod(local.sun_path, S_IRUSR | S_IWUSR | S_IXUSR); + } + + sendFd(resultFd, fd, 0, 0); + fatalClose(fd); + _exit(0); +} + +void UserSocketOps::childConnect(int resultFd, const string& path) { + if (path.size() >= sizeof(sockaddr_un::sun_path)) { + sendFd(resultFd, -1, -1, ENAMETOOLONG); + _exit(1); + } + + int fd = ::socket(AF_UNIX, SOCK_STREAM, 0); + if (fd < 0) { + sendFd(resultFd, -1, -1, errno); + _exit(1); + } + + sockaddr_un remote; + memset(&remote, 0, sizeof(remote)); + remote.sun_family = AF_UNIX; + strncpy(remote.sun_path, path.c_str(), sizeof(remote.sun_path) - 1); + + if (::connect(fd, (struct sockaddr*)&remote, sizeof(remote)) < 0) { + int err = errno; + fatalClose(fd); + sendFd(resultFd, -1, -1, err); + _exit(1); + } + + sendFd(resultFd, fd, 0, 0); + fatalClose(fd); + _exit(0); +} + +int UserSocketOps::runAsUser(Op op, const string& path, uid_t uid, gid_t gid) { + int sv[2]; + if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sv) < 0) { + return -1; + } + + pid_t pid = ::fork(); + if (pid < 0) { + int err = errno; + fatalClose(sv[0]); + fatalClose(sv[1]); + SetErrno(err); + return -1; + } + + if (pid == 0) { + fatalClose(sv[0]); + // Drop privileges before any path operation. Do not use logging here. + // Clear supplemental groups when running as root so we do not retain the + // parent's group set. setgroups(2) requires privilege and is skipped + // otherwise (e.g. already-unprivileged test processes). + if (::geteuid() == 0) { + if (::setgroups(1, &gid) != 0) { + sendFd(sv[1], -1, -1, errno); + _exit(1); + } + } + if (::setgid(gid) != 0) { + sendFd(sv[1], -1, -1, errno); + _exit(1); + } + if (::setuid(uid) != 0) { + sendFd(sv[1], -1, -1, errno); + _exit(1); + } + if (op == Op::LISTEN) { + childListen(sv[1], path); + } else { + childConnect(sv[1], path); + } + _exit(1); + } + + fatalClose(sv[1]); + int err = 0; + int fd = recvFd(sv[0], &err); + fatalClose(sv[0]); + + int status = 0; + while (::waitpid(pid, &status, 0) < 0) { + if (errno != EINTR) { + break; + } + } + + if (fd < 0) { + SetErrno(err ? err : EIO); + return -1; + } + return fd; +} + +int UserSocketOps::listenUnixAsUser(const string& path, uid_t uid, gid_t gid) { + return runAsUser(Op::LISTEN, path, uid, gid); +} + +int UserSocketOps::connectUnixAsUser(const string& path, uid_t uid, gid_t gid) { + return runAsUser(Op::CONNECT, path, uid, gid); +} +} // namespace et +#endif diff --git a/src/base/UserSocketOps.hpp b/src/base/UserSocketOps.hpp new file mode 100644 index 000000000..941332847 --- /dev/null +++ b/src/base/UserSocketOps.hpp @@ -0,0 +1,41 @@ +#ifndef __ET_USER_SOCKET_OPS__ +#define __ET_USER_SOCKET_OPS__ + +#include "Headers.hpp" + +namespace et { +#ifndef WIN32 +/** + * @brief Create or connect UNIX sockets after dropping to a session uid/gid. + * + * etserver is multithreaded and cannot safely seteuid in-process. These helpers + * fork a child, drop privileges, perform the socket operation, and return the + * resulting fd to the parent via SCM_RIGHTS. + */ +class UserSocketOps { + public: + /** + * @brief unlink/bind/listen/fchmod a UNIX socket path as @p uid/@p gid. + * @return Listening fd owned by the caller, or -1 on failure (errno set). + */ + static int listenUnixAsUser(const string& path, uid_t uid, gid_t gid); + + /** + * @brief connect() to a UNIX socket path as @p uid/@p gid. + * @return Connected fd owned by the caller, or -1 on failure (errno set). + */ + static int connectUnixAsUser(const string& path, uid_t uid, gid_t gid); + + private: + enum class Op : int { LISTEN = 1, CONNECT = 2 }; + + static int runAsUser(Op op, const string& path, uid_t uid, gid_t gid); + static void childListen(int resultFd, const string& path); + static void childConnect(int resultFd, const string& path); + static void sendFd(int channel, int fdToSend, int status, int err); + static int recvFd(int channel, int* errOut); +}; +#endif +} // namespace et + +#endif // __ET_USER_SOCKET_OPS__ diff --git a/src/terminal/TerminalServer.cpp b/src/terminal/TerminalServer.cpp index c1cb678e4..22c844fef 100644 --- a/src/terminal/TerminalServer.cpp +++ b/src/terminal/TerminalServer.cpp @@ -218,8 +218,8 @@ void TerminalServer::runTerminal( InitialResponse response; shared_ptr serverSocketHandler = getSocketHandler(); shared_ptr pipeSocketHandler(new PipeSocketHandler()); - shared_ptr portForwardHandler( - new PortForwardHandler(serverSocketHandler, pipeSocketHandler)); + shared_ptr portForwardHandler(new PortForwardHandler( + serverSocketHandler, pipeSocketHandler, userInfo.uid(), userInfo.gid())); map environmentVariables; for (const auto& envVar : payload.environmentvariables()) { diff --git a/src/terminal/forwarding/ForwardSourceHandler.cpp b/src/terminal/forwarding/ForwardSourceHandler.cpp index 308013e3f..68535c6cc 100644 --- a/src/terminal/forwarding/ForwardSourceHandler.cpp +++ b/src/terminal/forwarding/ForwardSourceHandler.cpp @@ -3,11 +3,13 @@ namespace et { ForwardSourceHandler::ForwardSourceHandler( shared_ptr _socketHandler, const SocketEndpoint& _source, - const SocketEndpoint& _destination) + const SocketEndpoint& _destination, bool alreadyListening) : socketHandler(_socketHandler), source(_source), destination(_destination) { - socketHandler->listen(source); + if (!alreadyListening) { + socketHandler->listen(source); + } } ForwardSourceHandler::~ForwardSourceHandler() { diff --git a/src/terminal/forwarding/ForwardSourceHandler.hpp b/src/terminal/forwarding/ForwardSourceHandler.hpp index 7d7905aeb..87f7d1b6a 100644 --- a/src/terminal/forwarding/ForwardSourceHandler.hpp +++ b/src/terminal/forwarding/ForwardSourceHandler.hpp @@ -11,11 +11,15 @@ namespace et { */ class ForwardSourceHandler { public: - /** @brief Creates source/destination handlers used for local port forwarding. + /** + * @brief Creates source/destination handlers used for local port forwarding. + * @param alreadyListening If true, skip listen(); caller already registered + * the source endpoint (e.g. via listenAsUser). */ ForwardSourceHandler(shared_ptr _socketHandler, const SocketEndpoint& _source, - const SocketEndpoint& _destination); + const SocketEndpoint& _destination, + bool alreadyListening = false); ~ForwardSourceHandler(); diff --git a/src/terminal/forwarding/PortForwardHandler.cpp b/src/terminal/forwarding/PortForwardHandler.cpp index 7de16ba99..b2e7c6fce 100644 --- a/src/terminal/forwarding/PortForwardHandler.cpp +++ b/src/terminal/forwarding/PortForwardHandler.cpp @@ -2,12 +2,16 @@ #include +#include "PipeSocketHandler.hpp" + namespace et { PortForwardHandler::PortForwardHandler( shared_ptr _networkSocketHandler, - shared_ptr _pipeSocketHandler) + shared_ptr _pipeSocketHandler, uid_t userid, gid_t groupid) : networkSocketHandler(_networkSocketHandler), - pipeSocketHandler(_pipeSocketHandler) {} + pipeSocketHandler(_pipeSocketHandler), + sessionUid(userid), + sessionGid(groupid) {} void PortForwardHandler::update(vector* requests, vector* dataToSend) { @@ -77,14 +81,23 @@ PortForwardSourceResponse PortForwardHandler::createSource( sourceHandlers.push_back(handler); return PortForwardSourceResponse(); } else { - auto handler = shared_ptr(new ForwardSourceHandler( - pipeSocketHandler, source, pfsr.destination())); #ifndef WIN32 - if (userid >= 0 && groupid >= 0) { - FATAL_FAIL(::chmod(source.name().c_str(), S_IRUSR | S_IWUSR | S_IXUSR)); - FATAL_FAIL(::chown(source.name().c_str(), userid, groupid)); + // Perform unlink/bind/listen as the session user so a client-chosen path + // cannot delete or chown root-owned files. + auto concretePipe = + dynamic_pointer_cast(pipeSocketHandler); + if (concretePipe && userid != static_cast(-1) && + groupid != static_cast(-1)) { + concretePipe->listenAsUser(source, userid, groupid); + auto handler = + shared_ptr(new ForwardSourceHandler( + pipeSocketHandler, source, pfsr.destination(), true)); + sourceHandlers.push_back(handler); + return PortForwardSourceResponse(); } #endif + auto handler = shared_ptr(new ForwardSourceHandler( + pipeSocketHandler, source, pfsr.destination())); sourceHandlers.push_back(handler); return PortForwardSourceResponse(); } @@ -114,7 +127,21 @@ PortForwardDestinationResponse PortForwardHandler::createDestination( fd = networkSocketHandler->connect(ipv4Localhost); } } else { +#ifndef WIN32 + // Connect as the session user so root etserver cannot open privileged + // sockets (e.g. docker.sock) on behalf of an unprivileged client. + auto concretePipe = + dynamic_pointer_cast(pipeSocketHandler); + if (concretePipe && sessionUid != static_cast(-1) && + sessionGid != static_cast(-1)) { + fd = concretePipe->connectAsUser(pfdr.destination(), sessionUid, + sessionGid); + } else { + fd = pipeSocketHandler->connect(pfdr.destination()); + } +#else fd = pipeSocketHandler->connect(pfdr.destination()); +#endif } PortForwardDestinationResponse pfdresponse; pfdresponse.set_clientfd(pfdr.fd()); diff --git a/src/terminal/forwarding/PortForwardHandler.hpp b/src/terminal/forwarding/PortForwardHandler.hpp index b338bd5f5..fc7af75d0 100644 --- a/src/terminal/forwarding/PortForwardHandler.hpp +++ b/src/terminal/forwarding/PortForwardHandler.hpp @@ -14,9 +14,16 @@ namespace et { */ class PortForwardHandler { public: - /** @brief Constructs forwarding helpers for network and router sockets. */ + /** + * @brief Constructs forwarding helpers for network and router sockets. + * @param userid Session uid used for privilege-dropped UNIX socket ops + * ((uid_t)-1 to disable). + * @param groupid Session gid used with @p userid. + */ explicit PortForwardHandler(shared_ptr _networkSocketHandler, - shared_ptr _pipeSocketHandler); + shared_ptr _pipeSocketHandler, + uid_t userid = static_cast(-1), + gid_t groupid = static_cast(-1)); /** @brief Polls all handlers for new destination/data and sends * `PortForwardData`. */ void update(vector* requests, @@ -48,6 +55,10 @@ class PortForwardHandler { shared_ptr networkSocketHandler; /** @brief Handler used for the router/pipe-facing sockets. */ shared_ptr pipeSocketHandler; + /** @brief Session uid for UNIX connect/listen; (uid_t)-1 disables drop. */ + uid_t sessionUid; + /** @brief Session gid for UNIX connect/listen; (gid_t)-1 disables drop. */ + gid_t sessionGid; /** @brief Active destination handlers keyed by socket id. */ unordered_map> destinationHandlers; diff --git a/test/unit_tests/BackedIOTest.cpp b/test/unit_tests/BackedIOTest.cpp index 9f21bfc30..465fd59f7 100644 --- a/test/unit_tests/BackedIOTest.cpp +++ b/test/unit_tests/BackedIOTest.cpp @@ -424,3 +424,37 @@ TEST_CASE("BackedWriter trims old data when connected and buffer exceeds 64MB", handler->close(fd); } + +TEST_CASE("BackedWriter recover rejects client-ahead sequence without aborting", + "[BackedIO]") { + auto handler = make_shared(); + auto encryptCrypto = make_shared( + "12345678901234567890123456789012", 0 /*verbosity*/); + const int fd = handler->createChannel(); + + BackedWriter writer(handler, encryptCrypto, fd); + REQUIRE(writer.write(Packet(1, "one")) == BackedWriterWriteState::SUCCESS); + writer.invalidateSocket(); + + REQUIRE_THROWS_AS(writer.recover(writer.getSequenceNumber() + 1), + std::runtime_error); +} + +TEST_CASE("SocketHandler readProto enforces max length before allocating", + "[SocketHandler]") { + FdSocketHandler handler; + int fds[2]; + REQUIRE(::pipe(fds) == 0); + + int64_t oversize = SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH + 1; + REQUIRE(handler.writeAllOrReturn(fds[1], &oversize, sizeof(oversize)) == + (int)sizeof(oversize)); + + REQUIRE_THROWS_AS( + handler.readProto( + fds[0], true, SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH), + std::runtime_error); + + handler.close(fds[0]); + handler.close(fds[1]); +} diff --git a/test/unit_tests/ClientConnectionTest.cpp b/test/unit_tests/ClientConnectionTest.cpp index 424de6a5a..1cee6a7da 100644 --- a/test/unit_tests/ClientConnectionTest.cpp +++ b/test/unit_tests/ClientConnectionTest.cpp @@ -198,6 +198,39 @@ TEST_CASE("ServerClientConnection verifies passkeys", handler->close(fds[1]); } +TEST_CASE("ServerClientConnection recoverClient keeps old socket on failure", + "[ServerClientConnection]") { + auto handler = make_shared(); + int live[2]; + REQUIRE(::socketpair(AF_UNIX, SOCK_STREAM, 0, live) == 0); + + const string key = "zyxwvutsrqponmlkjihgfedcba987654"; + ServerClientConnection connection(handler, "client-recover", live[0], key); + REQUIRE(connection.getSocketFd() == live[0]); + + int attack[2]; + REQUIRE(::socketpair(AF_UNIX, SOCK_STREAM, 0, attack) == 0); + + std::thread attacker([&]() { + // Read server SequenceHeader, then claim to be far ahead. + handler->readProto( + attack[1], true, SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH); + SequenceHeader bad; + bad.set_sequencenumber(999999); + handler->writeProto(attack[1], bad, true); + }); + + REQUIRE_FALSE(connection.recoverClient(attack[0])); + REQUIRE(connection.getSocketFd() == live[0]); + + attacker.join(); + connection.shutdown(); + handler->close(live[0]); + handler->close(live[1]); + // attack[0] closed inside recover on failure; attack[1] may still be open. + handler->close(attack[1]); +} + TEST_CASE("Connection recover exchanges sequence and catchup", "[Connection]") { auto handler = make_shared(); int live[2]; diff --git a/test/unit_tests/SecurityNoticesTest.cpp b/test/unit_tests/SecurityNoticesTest.cpp new file mode 100644 index 000000000..7d53cddf0 --- /dev/null +++ b/test/unit_tests/SecurityNoticesTest.cpp @@ -0,0 +1,446 @@ +/** + * Regression tests for public security notices under security_notices/. + * + * ANT-2026-VAMER5RC reconnect passkey proof is intentionally not covered: that + * requires a PROTOCOL_VERSION bump / wire-format change. + */ + +#include +#include + +#include +#include +#include +#include +#include +#include + +#include "BackedWriter.hpp" +#include "PipeSocketHandler.hpp" +#include "PortForwardHandler.hpp" +#include "ServerClientConnection.hpp" +#include "ServerConnection.hpp" +#include "TestHeaders.hpp" +#include "UserSocketOps.hpp" + +using namespace et; + +namespace { +// Minimal socket handler that works with socketpairs for handshake tests. +class SocketPairHandler : public SocketHandler { + public: + void queueConnectFd(int fd) { connectQueue.push(fd); } + + bool hasData(int fd) override { return waitOnSocketData(fd); } + + ssize_t read(int fd, void* buf, size_t count) override { + return ::read(fd, buf, count); + } + + ssize_t write(int fd, const void* buf, size_t count) override { + return ::write(fd, buf, count); + } + + int connect(const SocketEndpoint&) override { + if (connectQueue.empty()) { + return -1; + } + int fd = connectQueue.front(); + connectQueue.pop(); + return fd; + } + + set listen(const SocketEndpoint&) override { return {}; } + set getEndpointFds(const SocketEndpoint&) override { return {}; } + int accept(int fd) override { return fd; } + void stopListening(const SocketEndpoint&) override {} + void close(int fd) override { ::close(fd); } + vector getActiveSockets() override { return {}; } + + private: + std::queue connectQueue; +}; + +class RecordingServerConnection : public ServerConnection { + public: + RecordingServerConnection(std::shared_ptr socketHandler, + const SocketEndpoint& endpoint) + : ServerConnection(std::move(socketHandler), endpoint) {} + + bool newClient( + shared_ptr serverClientState) override { + return true; + } +}; + +class FdSocketHandler : public SocketHandler { + public: + bool hasData(int fd) override { return waitOnSocketData(fd); } + ssize_t read(int fd, void* buf, size_t count) override { + return ::read(fd, buf, count); + } + ssize_t write(int fd, const void* buf, size_t count) override { + return ::write(fd, buf, count); + } + int connect(const SocketEndpoint&) override { return -1; } + set listen(const SocketEndpoint&) override { return {}; } + set getEndpointFds(const SocketEndpoint&) override { return {}; } + int accept(int) override { return -1; } + void stopListening(const SocketEndpoint&) override {} + void close(int fd) override { ::close(fd); } + vector getActiveSockets() override { return {}; } +}; + +string makeTempDir() { + string pattern = GetTempDirectory() + "et_secnotice_XXXXXX"; + string dir = string(mkdtemp(&pattern[0])); + REQUIRE_FALSE(dir.empty()); + return dir; +} +} // namespace + +// --------------------------------------------------------------------------- +// ANT-2026-5PETM5BV — pre-auth slowloris / oversized ConnectRequest +// --------------------------------------------------------------------------- + +TEST_CASE( + "ANT-2026-5PETM5BV handshake readProto rejects oversized length before " + "allocating", + "[SecurityNotice][ANT-2026-5PETM5BV]") { + FdSocketHandler handler; + int fds[2]; + REQUIRE(::pipe(fds) == 0); + + // Exactly the old 128 MiB cap must also be rejected for handshake reads. + int64_t oversize = 128 * 1024 * 1024; + REQUIRE(oversize > SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH); + REQUIRE(handler.writeAllOrReturn(fds[1], &oversize, sizeof(oversize)) == + (int)sizeof(oversize)); + + REQUIRE_THROWS_AS( + handler.readProto( + fds[0], true, SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH), + std::runtime_error); + + handler.close(fds[0]); + handler.close(fds[1]); +} + +TEST_CASE( + "ANT-2026-5PETM5BV readAll absolute timeout fires under per-byte trickle", + "[SecurityNotice][ANT-2026-5PETM5BV]") { + FdSocketHandler handler; + int fds[2]; + REQUIRE(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0); + + std::atomic stop{false}; + std::thread trickle([&]() { + // Keep resetting the idle timer with 1 byte ~every 400ms; without an + // absolute deadline this would never time out. + char b = 'x'; + while (!stop.load()) { + if (::write(fds[1], &b, 1) < 0) { + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds(400)); + } + }); + + char buf[64]; + auto start = std::chrono::steady_clock::now(); + // Idle allowance is long; absolute deadline is short. + REQUIRE_THROWS_AS(handler.readAll(fds[0], buf, sizeof(buf), /*idle*/ 30, + /*absolute*/ 2), + std::runtime_error); + auto elapsed = std::chrono::steady_clock::now() - start; + auto elapsedSec = + std::chrono::duration_cast(elapsed).count(); + // Must not wait anywhere near the 30s idle timeout. + REQUIRE(elapsedSec < 10); + + stop.store(true); + trickle.join(); + handler.close(fds[0]); + handler.close(fds[1]); +} + +TEST_CASE( + "ANT-2026-5PETM5BV ServerConnection rejects oversized ConnectRequest " + "length", + "[SecurityNotice][ANT-2026-5PETM5BV][ServerConnection]") { + auto handler = make_shared(); + SocketEndpoint endpoint; + endpoint.set_name("server"); + RecordingServerConnection server(handler, endpoint); + + int fds[2]; + REQUIRE(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds) == 0); + + int64_t oversize = SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH + 1; + REQUIRE(handler->writeAllOrReturn(fds[0], &oversize, sizeof(oversize)) == + (int)sizeof(oversize)); + + // Must return (catch runtime_error), not abort the process. + REQUIRE_NOTHROW(server.clientHandler(fds[1])); + + handler->close(fds[0]); + server.shutdown(); +} + +// --------------------------------------------------------------------------- +// ANT-2026-VAMER5RC — pre-auth recover crash / force-disconnect +// (passkey-before-recover omitted: needs PROTOCOL_VERSION bump) +// --------------------------------------------------------------------------- + +TEST_CASE( + "ANT-2026-VAMER5RC BackedWriter recover throws instead of aborting on " + "client-ahead sequence", + "[SecurityNotice][ANT-2026-VAMER5RC][BackedIO]") { + class InMemorySocketHandler : public SocketHandler { + public: + int createChannel() { + int fd = nextFd++; + buffers[fd] = {}; + return fd; + } + bool hasData(int fd) override { return !buffers[fd].empty(); } + ssize_t read(int fd, void* buf, size_t count) override { + auto& q = buffers[fd]; + if (q.empty()) { + SetErrno(EPIPE); + return 0; + } + size_t n = std::min(count, q.size()); + for (size_t i = 0; i < n; ++i) { + static_cast(buf)[i] = q.front(); + q.pop_front(); + } + return n; + } + ssize_t write(int fd, const void* buf, size_t count) override { + auto* c = static_cast(buf); + for (size_t i = 0; i < count; ++i) { + buffers[fd].push_back(c[i]); + } + return count; + } + int connect(const SocketEndpoint&) override { return -1; } + set listen(const SocketEndpoint&) override { return {}; } + set getEndpointFds(const SocketEndpoint&) override { return {}; } + int accept(int) override { return -1; } + void stopListening(const SocketEndpoint&) override {} + void close(int) override {} + vector getActiveSockets() override { return {}; } + + private: + std::atomic nextFd{1}; + std::map> buffers; + }; + + auto handler = make_shared(); + auto crypto = make_shared("12345678901234567890123456789012", + 0 /*verbosity*/); + const int fd = handler->createChannel(); + BackedWriter writer(handler, crypto, fd); + REQUIRE(writer.write(Packet(1, "one")) == BackedWriterWriteState::SUCCESS); + writer.invalidateSocket(); + + REQUIRE_THROWS_AS(writer.recover(writer.getSequenceNumber() + 1), + std::runtime_error); +} + +TEST_CASE( + "ANT-2026-VAMER5RC recoverClient leaves victim socket open on bad sequence", + "[SecurityNotice][ANT-2026-VAMER5RC][ServerClientConnection]") { + auto handler = make_shared(); + int live[2]; + REQUIRE(::socketpair(AF_UNIX, SOCK_STREAM, 0, live) == 0); + + const string key = "zyxwvutsrqponmlkjihgfedcba987654"; + ServerClientConnection connection(handler, "client-recover", live[0], key); + REQUIRE(connection.getSocketFd() == live[0]); + REQUIRE(::fcntl(live[0], F_GETFD) != -1); + + int attack[2]; + REQUIRE(::socketpair(AF_UNIX, SOCK_STREAM, 0, attack) == 0); + + std::thread attacker([&]() { + handler->readProto( + attack[1], true, SocketHandler::MAX_HANDSHAKE_PROTO_LENGTH); + SequenceHeader bad; + bad.set_sequencenumber(999999); + handler->writeProto(attack[1], bad, true); + }); + + REQUIRE_FALSE(connection.recoverClient(attack[0])); + // Victim session must remain on the original fd, which must still be open. + REQUIRE(connection.getSocketFd() == live[0]); + REQUIRE(::fcntl(live[0], F_GETFD) != -1); + + attacker.join(); + connection.shutdown(); + handler->close(live[0]); + handler->close(live[1]); + handler->close(attack[1]); +} + +// --------------------------------------------------------------------------- +// ANT-2026-AVTT7HQH — reverse-tunnel source root unlink/chown +// --------------------------------------------------------------------------- + +#ifndef WIN32 +TEST_CASE( + "ANT-2026-AVTT7HQH createSource as user does not destroy undeletable file", + "[SecurityNotice][ANT-2026-AVTT7HQH][PortForwardHandler]") { + string dir = makeTempDir(); + string victim = dir + "/victim_file"; + { + int fd = ::open(victim.c_str(), O_CREAT | O_WRONLY | O_TRUNC, 0644); + REQUIRE(fd >= 0); + REQUIRE(::write(fd, "keepme", 6) == 6); + ::close(fd); + } + // Remove directory write permission so unlink(victim) fails for this user. + REQUIRE(::chmod(dir.c_str(), 0555) == 0); + + auto networkHandler = make_shared(); + auto pipeHandler = make_shared(); + PortForwardHandler handler(networkHandler, pipeHandler, getuid(), getgid()); + + PortForwardSourceRequest request; + SocketEndpoint source; + source.set_name(victim); + *request.mutable_source() = source; + SocketEndpoint destination; + destination.set_port(9); + *request.mutable_destination() = destination; + + PortForwardSourceResponse response = + handler.createSource(request, nullptr, getuid(), getgid()); + + REQUIRE(response.has_error()); + + REQUIRE(::chmod(dir.c_str(), 0755) == 0); + struct stat st; + REQUIRE(::stat(victim.c_str(), &st) == 0); + REQUIRE(S_ISREG(st.st_mode)); + + ::unlink(victim.c_str()); + ::rmdir(dir.c_str()); +} + +TEST_CASE( + "ANT-2026-AVTT7HQH createSource as user creates socket on writable path", + "[SecurityNotice][ANT-2026-AVTT7HQH][PortForwardHandler]") { + string dir = makeTempDir(); + string sockPath = dir + "/ok.sock"; + + { + auto networkHandler = make_shared(); + auto pipeHandler = make_shared(); + PortForwardHandler handler(networkHandler, pipeHandler, getuid(), getgid()); + + PortForwardSourceRequest request; + SocketEndpoint source; + source.set_name(sockPath); + *request.mutable_source() = source; + SocketEndpoint destination; + destination.set_port(9); + *request.mutable_destination() = destination; + + PortForwardSourceResponse response = + handler.createSource(request, nullptr, getuid(), getgid()); + REQUIRE_FALSE(response.has_error()); + + struct stat st; + REQUIRE(::stat(sockPath.c_str(), &st) == 0); + REQUIRE(S_ISSOCK(st.st_mode)); + } + + ::unlink(sockPath.c_str()); + ::rmdir(dir.c_str()); +} + +TEST_CASE( + "ANT-2026-AVTT7HQH UserSocketOps listen fails on root-only path without " + "deleting it", + "[SecurityNotice][ANT-2026-AVTT7HQH][UserSocketOps]") { + if (getuid() == 0) { + SKIP("Test requires a non-root process"); + } + // /dev/null is a privileged node; listen/unlink as an unprivileged user must + // fail and must not remove it. + REQUIRE(::access("/dev/null", F_OK) == 0); + int fd = UserSocketOps::listenUnixAsUser("/dev/null", getuid(), getgid()); + REQUIRE(fd < 0); + REQUIRE(::access("/dev/null", F_OK) == 0); +} + +// --------------------------------------------------------------------------- +// ANT-2026-A3WQS3AG — forward destination root connect to arbitrary unix path +// --------------------------------------------------------------------------- + +TEST_CASE( + "ANT-2026-A3WQS3AG createDestination as session user can reach own socket", + "[SecurityNotice][ANT-2026-A3WQS3AG][PortForwardHandler]") { + string dir = makeTempDir(); + string path = dir + "/dest.sock"; + + int listenFd = UserSocketOps::listenUnixAsUser(path, getuid(), getgid()); + REQUIRE(listenFd >= 0); + + auto networkHandler = make_shared(); + auto pipeHandler = make_shared(); + PortForwardHandler handler(networkHandler, pipeHandler, getuid(), getgid()); + + PortForwardDestinationRequest request; + SocketEndpoint destination; + destination.set_name(path); + *request.mutable_destination() = destination; + request.set_fd(7); + + PortForwardDestinationResponse response = handler.createDestination(request); + REQUIRE_FALSE(response.has_error()); + REQUIRE(response.has_socketid()); + + int accepted = ::accept(listenFd, nullptr, nullptr); + REQUIRE(accepted >= 0); + ::close(accepted); + ::close(listenFd); + ::unlink(path.c_str()); + ::rmdir(dir.c_str()); +} + +TEST_CASE( + "ANT-2026-A3WQS3AG createDestination as session user cannot open " + "mode-000 socket", + "[SecurityNotice][ANT-2026-A3WQS3AG][PortForwardHandler]") { + string dir = makeTempDir(); + string path = dir + "/denied.sock"; + + int listenFd = UserSocketOps::listenUnixAsUser(path, getuid(), getgid()); + REQUIRE(listenFd >= 0); + // Strip all access. A root connect would often still succeed; connecting as + // the session user must fail. + REQUIRE(::chmod(path.c_str(), 0) == 0); + + auto networkHandler = make_shared(); + auto pipeHandler = make_shared(); + PortForwardHandler handler(networkHandler, pipeHandler, getuid(), getgid()); + + PortForwardDestinationRequest request; + SocketEndpoint destination; + destination.set_name(path); + *request.mutable_destination() = destination; + request.set_fd(8); + + PortForwardDestinationResponse response = handler.createDestination(request); + REQUIRE(response.has_error()); + REQUIRE_FALSE(response.has_socketid()); + + ::close(listenFd); + ::chmod(path.c_str(), 0700); + ::unlink(path.c_str()); + ::rmdir(dir.c_str()); +} +#endif diff --git a/test/unit_tests/UserSocketOpsTest.cpp b/test/unit_tests/UserSocketOpsTest.cpp new file mode 100644 index 000000000..780ce448a --- /dev/null +++ b/test/unit_tests/UserSocketOpsTest.cpp @@ -0,0 +1,58 @@ +#include "TestHeaders.hpp" +#include "UserSocketOps.hpp" + +#ifndef WIN32 +#include +#include + +using namespace et; + +TEST_CASE("UserSocketOps listen and connect as current user", + "[UserSocketOps]") { + string dirTemplate = GetTempDirectory() + "et_user_sock_XXXXXX"; + string dir = string(mkdtemp(&dirTemplate[0])); + string path = dir + "/sock"; + + uid_t uid = getuid(); + gid_t gid = getgid(); + + int listenFd = UserSocketOps::listenUnixAsUser(path, uid, gid); + REQUIRE(listenFd >= 0); + + struct stat st; + REQUIRE(::stat(path.c_str(), &st) == 0); + REQUIRE(S_ISSOCK(st.st_mode)); + + int connFd = UserSocketOps::connectUnixAsUser(path, uid, gid); + REQUIRE(connFd >= 0); + + int client = ::accept(listenFd, nullptr, nullptr); + REQUIRE(client >= 0); + + REQUIRE(::write(connFd, "ping", 4) == 4); + char buf[4]; + REQUIRE(::read(client, buf, 4) == 4); + REQUIRE(string(buf, 4) == "ping"); + REQUIRE(::write(client, "pong", 4) == 4); + REQUIRE(::read(connFd, buf, 4) == 4); + REQUIRE(string(buf, 4) == "pong"); + + ::close(client); + ::close(connFd); + ::close(listenFd); + ::unlink(path.c_str()); + ::rmdir(dir.c_str()); +} + +TEST_CASE("UserSocketOps listen as user cannot unlink root-only path", + "[UserSocketOps]") { + if (getuid() == 0) { + SKIP("Test requires a non-root process"); + } + + // A path under /dev that a normal user cannot replace. + string path = "/dev/null_et_should_not_bind"; + int fd = UserSocketOps::listenUnixAsUser(path, getuid(), getgid()); + REQUIRE(fd < 0); +} +#endif