mmo-ref/tinymux
1
0
Fork 0
mirror of https://github.com/brazilofmux/tinymux synced 2026-08-13 00:23:11 -04:00
Code Issues Projects Releases Packages Wiki Activity

fix(ganl): close #950 cleartext gap; polish review suggestions

Browse source 
Unify TLS/plain egress in ConnectionBase::egressToWire so
processProtocolData and startTelnetNegotiation never cleartext-emit on
a TLS socket that is not established (only sendDataToClient had the
#950 guard). Count connections_accepted_ only after successful
initialize; print NET/STAT with full-width counters; set socketClosed_
on associateContext-failure close; align interactive CPU HALT exemption
with cque via drv_Wizard and log the exemption; warn once when
SSL_OP_NO_RENEGOTIATION is unavailable; note json TC001 split_token lock.
This commit is contained in:
Stephen Dennis 2026-07-23 20:49:21 -06:00
parent c140c59435
commit 8c971724cf
6 changed files with 128 additions and 94 deletions
Download patch file Download diff file Expand all files Collapse all files

8
mux/ganl/include/connection.h
View file

@ -174,6 +174,14 @@ protected:
bool processProtocolData();
void closeNetworkAfterDrain();
// Single egress path for protocol/application bytes (#950). TLS +
// established → encrypt into encryptedOutput_; non-TLS → plain append;
// TLS but not established → never cleartext (retain source if
// retainIfTlsNotReady, else drop). Returns false if a TLS error closed
// the connection.
bool egressToWire(IoBuffer& source, bool retainIfTlsNotReady,
const char* what);
// Core properties needed by derived classes
ConnectionHandle handle_;
NetworkEngine& networkEngine_;

157
mux/ganl/src/connection.cpp
View file

@ -151,6 +151,7 @@ bool ConnectionBase::initialize(bool useTls) {
// this one used to return without closing, leaking the fd (the adapter's
// initialize()-failed path only erases its maps, it does not close).
networkEngine_.closeConnection(handle_);
socketClosed_ = true;
transitionToState(ConnectionState::Closed); // Mark as unusable
return false;
}
@ -272,6 +273,62 @@ void ConnectionBase::handleNetworkEvent(const IoEvent& event) {
// --- Data Flow ---
// Single egress path for protocol/application bytes (#950).
//
// TLS + established → processOutgoing into encryptedOutput_
// non-TLS → plain append into encryptedOutput_
// TLS + !established → never cleartext. If retainIfTlsNotReady, leave source
// unconsumed so a member buffer can flush later; else drop.
//
bool ConnectionBase::egressToWire(IoBuffer& source, bool retainIfTlsNotReady,
const char* what)
{
if (source.readableBytes() == 0) {
return true;
}
if (useTls_ && secureTransport_ != nullptr) {
if (secureTransport_->isEstablished(handle_)) {
GANL_CONN_DEBUG(handle_, "Encrypting " << source.readableBytes()
<< " bytes of " << what << " through TLS.");
TlsResult result = secureTransport_->processOutgoing(
handle_, source, encryptedOutput_, true);
GANL_CONN_DEBUG(handle_, "TLS processOutgoing (" << what
<< ") result: " << static_cast<int>(result)
<< ". encryptedOutput_ size now: "
<< encryptedOutput_.readableBytes());
if (result == TlsResult::Error || result == TlsResult::Closed) {
GANL_CONN_DEBUG(handle_, "TLS error sending " << what << ": "
<< secureTransport_->getLastTlsErrorString(handle_)
<< ". Closing.");
close(DisconnectReason::TlsError);
return false;
}
return true;
}
// Mid/failed (re)negotiation window: NEVER emit cleartext on a TLS
// socket. That is a confidentiality downgrade (#950).
if (retainIfTlsNotReady) {
GANL_CONN_DEBUG(handle_, "TLS not established; retaining "
<< source.readableBytes() << " bytes of " << what
<< " (no cleartext egress).");
} else {
GANL_CONN_DEBUG(handle_, "TLS not established; dropping "
<< source.readableBytes() << " bytes of " << what
<< " (no cleartext egress).");
source.clear();
}
return true;
}
GANL_CONN_DEBUG(handle_, "TLS not used. Copying " << source.readableBytes()
<< " bytes of " << what << " directly to encryptedOutput_.");
encryptedOutput_.append(source.readPtr(), source.readableBytes());
source.clear();
return true;
}
void ConnectionBase::sendDataToClient(const std::string& data) {
GANL_CONN_DEBUG(handle_, "Received " << data.length() << " bytes from application to send.");
@ -302,7 +359,7 @@ void ConnectionBase::sendDataToClient(const std::string& data) {
// leftover bytes are preserved in order and flushed on the next send.
// (#948 closes the only current trigger — client-initiated renegotiation
// — but retaining the bytes is correct regardless. The non-TLS path
// below clears formattedOutput_ explicitly after copying it out.)
// clears formattedOutput_ inside egressToWire after copying it out.)
if (!protocolHandler_.formatOutput(handle_, applicationOutput_, formattedOutput_, true)) {
GANL_CONN_DEBUG(handle_, "Error formatting output data: " << protocolHandler_.getLastProtocolErrorString(handle_) << ". Closing.");
close(DisconnectReason::ProtocolError);
@ -311,45 +368,12 @@ void ConnectionBase::sendDataToClient(const std::string& data) {
// Explicitly passing consumeInput=true ensures applicationOutput_ is consumed by the method
GANL_CONN_DEBUG(handle_, "Formatted data. formattedOutput_ size: " << formattedOutput_.readableBytes());
// 3. Encrypt the data (TLS) if needed
// Note: processOutgoing reads from formattedOutput_ and writes to encryptedOutput_
IoBuffer* sourceBuffer = &formattedOutput_; // Start with formatted data
if (useTls_ && secureTransport_ != nullptr) {
if (secureTransport_->isEstablished(handle_)) {
GANL_CONN_DEBUG(handle_, "Processing " << sourceBuffer->readableBytes() << " bytes through TLS.");
// Ensure encryptedOutput_ has space? Or assume processOutgoing handles it. Let's assume.
TlsResult result = secureTransport_->processOutgoing(handle_, *sourceBuffer, encryptedOutput_, true);
// Explicitly specifying consumeInput=true ensures buffer is consumed by the method
GANL_CONN_DEBUG(handle_, "TLS processOutgoing result: " << static_cast<int>(result)
<< ". encryptedOutput_ size now: " << encryptedOutput_.readableBytes());
if (result == TlsResult::Error || result == TlsResult::Closed) {
GANL_CONN_DEBUG(handle_, "TLS error during processOutgoing: " << secureTransport_->getLastTlsErrorString(handle_) << ". Closing.");
close(DisconnectReason::TlsError);
return;
}
// If result is WantRead/WantWrite, it implies handshake messages need I/O,
// but the application data might still be buffered in encryptedOutput_.
// The subsequent postWrite() call will handle sending.
} else {
// TLS connection, but the session is not currently established
// (mid/failed (re)negotiation window). NEVER emit application data
// as plaintext on a TLS socket — that is a confidentiality
// downgrade (#950). Retain the formatted bytes in formattedOutput_;
// formatOutput() appends and processOutgoing() consumes only what it
// encrypts, so the retained bytes flush in order once TLS is
// (re)established, or are dropped if the connection closes.
GANL_CONN_DEBUG(handle_, "TLS not established; retaining " << sourceBuffer->readableBytes()
<< " formatted bytes (no cleartext egress).");
}
} else {
GANL_CONN_DEBUG(handle_, "TLS not used. Copying " << sourceBuffer->readableBytes() << " bytes directly to encryptedOutput_.");
// If TLS is not enabled, copy formatted data directly to encrypted output
encryptedOutput_.append(sourceBuffer->readPtr(), sourceBuffer->readableBytes());
sourceBuffer->clear(); // Consume the source to be consistent with TLS path
GANL_CONN_DEBUG(handle_, "encryptedOutput_ size now: " << encryptedOutput_.readableBytes());
// 3. Encrypt or plain-copy via the single #950-safe egress helper.
// Retain formattedOutput_ if TLS is not yet established so a later
// send can flush once the session is ready.
if (!egressToWire(formattedOutput_, /*retainIfTlsNotReady=*/true,
"application data")) {
return;
}
// 4. Post write operation if not already pending
@ -449,29 +473,16 @@ bool ConnectionBase::processProtocolData() {
return false; // Stop processing
}
// Send any generated Telnet responses
// Send any generated Telnet responses via the #950-safe egress path.
// Local buffer: drop (do not retain) if TLS is not established — there
// is no member buffer to hold them across the call.
if (telnetResponses.readableBytes() > 0) {
GANL_CONN_DEBUG(handle_, "Sending " << telnetResponses.readableBytes() << " bytes of Telnet responses.");
// Treat telnet responses as application data going out
IoBuffer* sourceBuf = &telnetResponses;
if (isTlsEnabled() && secureTransport_ != nullptr && secureTransport_->isEstablished(handle_)) {
// Encrypt the telnet responses
GANL_CONN_DEBUG(handle_, "Encrypting telnet responses.");
TlsResult tlsRes = secureTransport_->processOutgoing(handle_, *sourceBuf, encryptedOutput, true);
// Explicitly passing consumeInput=true ensures source buffer is consumed
if (tlsRes == TlsResult::Error || tlsRes == TlsResult::Closed) {
GANL_CONN_DEBUG(handle_, "TLS Error sending telnet responses: " << secureTransport_->getLastTlsErrorString(handle_) << ". Closing.");
close(DisconnectReason::TlsError);
return false;
}
} else {
// Send directly if no TLS
GANL_CONN_DEBUG(handle_, "Sending telnet responses without TLS.");
encryptedOutput.append(sourceBuf->readPtr(), sourceBuf->readableBytes());
sourceBuf->clear(); // Consume all data to be consistent with TLS path
GANL_CONN_DEBUG(handle_, "Sending " << telnetResponses.readableBytes()
<< " bytes of Telnet responses.");
if (!egressToWire(telnetResponses, /*retainIfTlsNotReady=*/false,
"telnet responses")) {
return false;
}
// Ensure write is posted
if (encryptedOutput.readableBytes() > 0 && !pendingWriteFlag()) {
postWrite();
}
@ -836,26 +847,14 @@ void ConnectionBase::startTelnetNegotiation() {
protocolHandler_.startNegotiation(handle_, telnetOptions);
GANL_CONN_DEBUG(handle_, "protocolHandler_.startNegotiation generated " << telnetOptions.readableBytes() << " bytes of options.");
// Send the initial options
// Send the initial options via the #950-safe egress path. TLS callers
// only reach here after isEstablished(); if that invariant breaks, drop
// rather than cleartext-egress IAC negotiation bytes.
if (telnetOptions.readableBytes() > 0) {
IoBuffer* sourceBuf = &telnetOptions;
if (useTls_ && secureTransport_ != nullptr && secureTransport_->isEstablished(handle_)) {
GANL_CONN_DEBUG(handle_, "Encrypting initial Telnet options.");
TlsResult tlsRes = secureTransport_->processOutgoing(handle_, *sourceBuf, encryptedOutput_, true);
// Explicitly passing consumeInput=true ensures source buffer is consumed
if (tlsRes == TlsResult::Error || tlsRes == TlsResult::Closed) {
GANL_CONN_DEBUG(handle_, "TLS Error sending initial Telnet options: " << secureTransport_->getLastTlsErrorString(handle_) << ". Closing.");
close(DisconnectReason::TlsError);
return; // Stop negotiation start
}
} else {
GANL_CONN_DEBUG(handle_, "Sending initial Telnet options without TLS.");
encryptedOutput_.append(sourceBuf->readPtr(), sourceBuf->readableBytes());
sourceBuf->clear(); // Consume all data to be consistent with TLS path
if (!egressToWire(telnetOptions, /*retainIfTlsNotReady=*/false,
"initial Telnet options")) {
return; // TLS error closed the connection
}
// Ensure write is posted
if (encryptedOutput_.readableBytes() > 0 && !pendingWrite_) {
postWrite();
}

13
mux/ganl/src/openssl_transport.cpp
View file

@ -84,6 +84,19 @@ bool OpenSSLTransport::initialize(const TlsConfig& config) {
// retry safely (see #949). TLS 1.3 has no renegotiation. Guarded because
// the macro only exists on OpenSSL >= 1.1.0h / LibreSSL. (#948)
options |= SSL_OP_NO_RENEGOTIATION;
#else
// Residual risk on older OpenSSL/LibreSSL without the option: TLS 1.2
// client-initiated renegotiation remains possible (CPU DoS + BAD_WRITE_RETRY
// surface). We still require TLS 1.2+ above and enable MOVING_WRITE_BUFFER
// below, but cannot refuse renegotiation. Log once so operators know.
static bool s_loggedRenegoGap = false;
if (!s_loggedRenegoGap) {
s_loggedRenegoGap = true;
std::cerr << "[OpenSSL:Global] WARNING: SSL_OP_NO_RENEGOTIATION is not "
"available on this OpenSSL build; TLS 1.2 client-initiated "
"renegotiation cannot be disabled (#948)."
<< std::endl;
}
#endif
SSL_CTX_set_options(ctx_, options);

23
mux/src/ganl_adapter.cpp
View file

@ -2136,13 +2136,18 @@ void GanlAdapter::run_main_loop() {
pending_remote_addresses_[connHandle] = events[i].remoteAddress;
pending_tls_flags_[connHandle] = useTls;
handle_to_conn_[connHandle] = conn;
connections_accepted_++;
// Count only connections that fully initialize. Failed
// init closes the fd itself and never reaches
// onConnectionClose, so accepting first would permanently
// inflate NET/STAT live = accepted - closed.
if (!conn->initialize(useTls)) {
handle_to_conn_.erase(connHandle);
connection_listener_map_.erase(connHandle);
pending_remote_addresses_.erase(connHandle);
pending_tls_flags_.erase(connHandle);
} else {
connections_accepted_++;
}
}
continue;
@ -2373,14 +2378,14 @@ void GanlAdapter::log_socket_stats(bool force)
STARTLOG(LOG_NET, "NET", "STAT");
g_pILog->WriteString(tprintf(
T("sockets descs=%u conn_map=%u desc_map=%u accepted=%u closed=%u live=%d osfds=%d"),
static_cast<unsigned>(g_descriptors_list.size()),
static_cast<unsigned>(handle_to_conn_.size()),
static_cast<unsigned>(handle_to_desc_.size()),
static_cast<unsigned>(connections_accepted_),
static_cast<unsigned>(connections_closed_),
static_cast<int>(live),
static_cast<int>(osfds)));
T("sockets descs=%zu conn_map=%zu desc_map=%zu accepted=%llu closed=%llu live=%lld osfds=%ld"),
g_descriptors_list.size(),
handle_to_conn_.size(),
handle_to_desc_.size(),
static_cast<unsigned long long>(connections_accepted_),
static_cast<unsigned long long>(connections_closed_),
live,
osfds));
ENDLOG;
}

19
mux/src/net.cpp
View file

@ -2626,16 +2626,23 @@ void do_command(DESC *d, UTF8 *command)
{
notify(d->player, T("GAME: Expensive activity abbreviated."));
// Same exemption as the queue-side guard in cque.cpp: the
// command was already abbreviated mid-run, so the HALT only
// quarantines future work. Don't dark a wizard whose next
// typed command is often the fix.
if ( GOD != d->player
&& !(drv_Flags(d->player, FLAG_WORD1) & WIZARD))
// Same exemption as the queue-side guard in cque.cpp: use
// Wizard() (via drv_Wizard — owner inheritance), not the raw
// WIZARD bit. The command was already abbreviated mid-run, so
// the HALT only quarantines future work. Don't dark a wizard
// whose next typed command is often the fix.
if (!drv_Wizard(d->player))
{
drv_HaltQueue(d->player, NOTHING);
drv_s_Flags(d->player, FLAG_WORD1, drv_Flags(d->player, FLAG_WORD1) | HALT);
}
else
{
STARTLOG(LOG_PROBLEMS, "CMD", "CPU");
g_pILog->log_name_and_loc(d->player);
g_pILog->log_text(T(" expensive activity abbreviated; wizard player exempted from HALT"));
ENDLOG;
}
}
alarm_clock.clear();

2
testcases/json_fn.mux
View file

@ -13,6 +13,8 @@
-
#
# Test Case #1 - json() construction.
# L5/L6/L8 lock the split_token whole-tail fix: next_token left the list
# tail intact so json(array,1 2 3) used to re-emit as [1 2 3,2 3,3].
#
&tr.tc001 test_json_fn=
@if cand(