From 8c8fc31952a70507870ba6e0d168b818c2604597 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Tue, 22 Sep 2026 21:48:22 -0400 Subject: [PATCH] [MG_Remote, MG_Test, ci] (P7): the token compare runs a fixed 64-byte width, an ignored window ask is logged once, a nonce that could not be minted is refused by name, and a garbage first frame while busy is MalformedHello rather than Busy - AuthToken.h: ConstantTimeTokenMatch loops kAuthTokenCompareBytes (64) iterations with both operands read as zero past their ends - its trip count no longer tells a peer the configured token's length; a token longer than 64 is still compared to its last byte (SocketTransportTest pins the 70-byte case) - ServerSession.cpp: an ask in Hello.linkTerms above the granted terms logs one MGLOG_W naming both sides ("the ask is ignored and the session is server-sized"); the production client asks for nothing, so a session that was never mis-configured never logs it; SessionHandshakeTest's 64 GiB case requires the line (red before: absent) - ServerSession.cpp: a MintNonce failure sends Refuse{LinkTerms} "stream data plane unavailable: the server could not mint a data nonce" before returning the fault it is (still exit 67, still counted) instead of a bare close - ServerMain.cpp: a first frame with the wrong magic or over 1 MiB while a session is live is Refuse{MalformedHello} "first frame is not a control frame", drained before the close so the refusal is not RST away; Busy stays the answer for a well-formed frame that is not a DataBind - tcp_supervisor_smoke.py: garbage_first_frame_while_busy (magic XXXX on a live session -> code 8; red before: code 7) - fatal census unchanged: 79 abort sites, 3 refusal words (the new refusals go through the enum, not a literal) --- MobileGL/MG_Remote/Server/ServerMain.cpp | 19 ++++++++ MobileGL/MG_Remote/Server/ServerSession.cpp | 46 +++++++++++++++---- MobileGL/MG_Remote/Transport/AuthToken.h | 31 +++++++++---- .../MG_Test/Wire/SessionHandshakeTest.cpp | 11 +++++ MobileGL/MG_Test/Wire/SocketTransportTest.cpp | 9 ++++ scripts/ci/tcp_supervisor_smoke.py | 22 +++++++++ 6 files changed, 120 insertions(+), 18 deletions(-) diff --git a/MobileGL/MG_Remote/Server/ServerMain.cpp b/MobileGL/MG_Remote/Server/ServerMain.cpp index ed5e5b43..0041ffce 100644 --- a/MobileGL/MG_Remote/Server/ServerMain.cpp +++ b/MobileGL/MG_Remote/Server/ServerMain.cpp @@ -89,10 +89,29 @@ void RefuseBusy(SocketTransport& transport, const char* detail) { constexpr std::uint32_t kFirstFrameWaitMs = 2000; constexpr std::uint64_t kFirstFrameMaxBytes = 1024 * 1024; +// Whatever the peer has already sent, read and dropped (non-blocking, at most 1 MiB), so a close +// with bytes still unread does not become an RST that discards the refusal in flight - the +// reason RefuseBusy reads the Hello before refusing. +void DrainUnread(int fd) { + char sink[4096]; + for (int rounds = 0; rounds < 256; ++rounds) { + if (::recv(fd, sink, sizeof(sink), MSG_DONTWAIT) <= 0) return; + } +} + void RouteWhileBusy(SocketTransport& connection, int handoff) { std::vector frame; const int fd = connection.StreamFd(); const auto read = SocketTransport::ReceiveOneFrame(fd, kFirstFrameWaitMs, kFirstFrameMaxBytes, &frame); + if (read == MOBILEGL_ERR_PROTOCOL_MISMATCH) { + // (F fix round) A first frame with the wrong magic or a length over 1 MiB is not a second + // client waiting its turn; it is not a control frame at all, and Busy would name a + // condition this connection does not have. The word is the one ServerSession::Accept uses + // for a first frame that is not a verifiable Hello. + DrainUnread(fd); + Refuse(connection, Protocol::RefuseCode::MalformedHello, "first frame is not a control frame"); + return; + } std::uint8_t nonce[kDataNonceBytes] = {}; if (read == MOBILEGL_OK && DecodeDataBind(frame, nonce)) { if (handoff >= 0 && diff --git a/MobileGL/MG_Remote/Server/ServerSession.cpp b/MobileGL/MG_Remote/Server/ServerSession.cpp index b5f3c444..c860d166 100644 --- a/MobileGL/MG_Remote/Server/ServerSession.cpp +++ b/MobileGL/MG_Remote/Server/ServerSession.cpp @@ -571,13 +571,21 @@ namespace MobileGL::MG_Remote::Server { return RefuseHandshake(transport, ::MobileGL::Wire::RefuseCode::LinkTerms, "stream needs a TCP control connection and a data-connection source"); created = Transport::SocketTransport::MintNonce(dataNonce, sizeof(dataNonce)); + if (created != MOBILEGL_OK) { + // (F fix round) The peer is told by name instead of seeing a bare close. The + // return value stays the fault it is - exit 67 in ServerMain, counted in + // sessionsFaulted - because a CSPRNG that gave nothing is this server's failure + // and not the peer's terms; the code is LinkTerms because the terms asked for a + // stream data plane and that is what could not be granted. + (void)RefuseHandshake(transport, ::MobileGL::Wire::RefuseCode::LinkTerms, + "stream data plane unavailable: the server could not mint a data nonce"); + return created; + } + auto link = std::make_unique(); + created = link->AttachOwnedDeferred(m_sizes, Transport::TransportRoleTag::ServerConsumer); if (created == MOBILEGL_OK) { - auto link = std::make_unique(); - created = link->AttachOwnedDeferred(m_sizes, Transport::TransportRoleTag::ServerConsumer); - if (created == MOBILEGL_OK) { - pendingStream = link.get(); - AttachDataLink(std::move(link)); - } + pendingStream = link.get(); + AttachDataLink(std::move(link)); } } else { created = m_link->Memory().Create(m_sizes, Transport::MemoryRole::Server); @@ -619,9 +627,31 @@ namespace MobileGL::MG_Remote::Server { ? builder.CreateVector(dataNonce, sizeof(dataNonce)) : ::flatbuffers::Offset<::flatbuffers::Vector>(); const Uint64 maxReply = m_sizes.ReplyBytes / m_sizes.ReplySlotCount - sizeof(Transport::ReplySlotHeader); + const Uint64 cmdWindow = m_link->Memory().CmdRingCapacity(); + const Uint64 stageWindow = m_link->Memory().StageBytes(); + const Uint64 eventWindow = m_link->Memory().EventRingCapacity(); + // PH-8 (F fix round). The Hello's four counts are IGNORED, not clamped: nothing above + // read them for sizing, and the session is the size this server chose. Said once when + // a peer asked for more than it is granted, so an operator comparing a client's + // configured windows with the server's sees which side sized the session (rule I: a + // silent difference is the kind this phase removes). The production client asks for + // nothing, so a session that was never mis-configured never logs this. + if (terms->cmdWindowBytes() > cmdWindow || terms->stageWindowBytes() > stageWindow || + terms->eventWindowBytes() > eventWindow || terms->maxReplyBytes() > maxReply) { + MGLOG_W("MG_Remote server: Hello asked for windows the server does not grant " + "(cmd=%llu stage=%llu event=%llu maxReply=%llu); the ask is ignored and the " + "session is server-sized (cmd=%llu stage=%llu event=%llu maxReply=%llu) - PH-8", + static_cast(terms->cmdWindowBytes()), + static_cast(terms->stageWindowBytes()), + static_cast(terms->eventWindowBytes()), + static_cast(terms->maxReplyBytes()), + static_cast(cmdWindow), + static_cast(stageWindow), + static_cast(eventWindow), + static_cast(maxReply)); + } auto negotiated = ::MobileGL::Wire::CreateLinkTerms(builder, terms->dataPlane(), - ::MobileGL::Wire::WireForm::StructImage, maxReply, m_link->Memory().CmdRingCapacity(), - m_link->Memory().StageBytes(), m_link->Memory().EventRingCapacity()); + ::MobileGL::Wire::WireForm::StructImage, maxReply, cmdWindow, stageWindow, eventWindow); auto welcome = ::MobileGL::Wire::CreateWelcome( builder, MOBILEGL_PROTOCOL_ABI_MAJOR, MOBILEGL_PROTOCOL_ABI_MINOR, // CONTRACT-P6 4.3: THE SERVER'S OWN PID, not an echo of the client's. diff --git a/MobileGL/MG_Remote/Transport/AuthToken.h b/MobileGL/MG_Remote/Transport/AuthToken.h index cb594f9e..9edc364c 100644 --- a/MobileGL/MG_Remote/Transport/AuthToken.h +++ b/MobileGL/MG_Remote/Transport/AuthToken.h @@ -65,24 +65,35 @@ namespace MobileGL::MG_Remote::Transport { return token != nullptr && std::strlen(token) >= kMinimumAuthTokenBytes; } - // Constant time in the CONTENTS of both operands. + // The comparison's width. Up to this many bytes the loop's trip count is a constant; a + // configured token longer than it is compared in full (the loop extends to its length). + inline constexpr std::size_t kAuthTokenCompareBytes = 64; + + // Constant time in the CONTENTS of both operands, and in their LENGTHS up to + // kAuthTokenCompareBytes. // - // The loop runs over the expected token's length and never leaves early, so the time depends - // on that length - which is this process's own configuration, fixed before any peer connects - // and not attacker-controlled - and on nothing the peer sent. The two lengths are folded into - // the accumulator rather than branched on, so a peer cannot learn the token's length from a - // faster answer either. Past the presented token's end the comparison reads a zero, which is - // a value, not a short-circuit: `presented` is never indexed out of its own bounds. + // The first shape of this loop ran over the configured token's length. That length is not the + // peer's to vary, but it is part of the operator's secret, and an answer whose time was + // proportional to it told a peer how long a token to guess (F fix round, review of slices + // 1-3). The loop now runs a fixed 64 iterations - or the configured token's length past that, + // so a longer token is still compared to its last byte and reveals only that it is longer than + // 64 - and reads BOTH operands as zero past their own ends: a zero is a value, not a + // short-circuit, and neither operand is ever indexed out of its own bounds. The two lengths + // are folded into the accumulator rather than branched on, and nothing leaves early. The + // presented length does not extend the loop: a peer's own bytes are the one thing it already + // knows, and letting it choose the trip count would let it choose our work. inline bool ConstantTimeTokenMatch(const char* expected, const char* presented, std::size_t presentedSize) { if (expected == nullptr) return false; const std::size_t expectedSize = std::strlen(expected); + const std::size_t width = expectedSize > kAuthTokenCompareBytes ? expectedSize : kAuthTokenCompareBytes; std::size_t difference = expectedSize ^ presentedSize; - for (std::size_t index = 0; index < expectedSize; ++index) { + for (std::size_t index = 0; index < width; ++index) { + const unsigned char ours = + index < expectedSize ? static_cast(expected[index]) : 0u; const unsigned char theirs = index < presentedSize ? static_cast(presented[index]) : 0u; - difference |= static_cast( - static_cast(expected[index]) ^ theirs); + difference |= static_cast(ours ^ theirs); } return difference == 0; } diff --git a/MobileGL/MG_Test/Wire/SessionHandshakeTest.cpp b/MobileGL/MG_Test/Wire/SessionHandshakeTest.cpp index f51c4d60..e4ee8020 100644 --- a/MobileGL/MG_Test/Wire/SessionHandshakeTest.cpp +++ b/MobileGL/MG_Test/Wire/SessionHandshakeTest.cpp @@ -273,6 +273,12 @@ TEST(SessionHandshakeTest, ForkRefusesDifferentBuildWithIdenticalWire) { // Accept below then fails to create a 64 GiB private segment, or the EXPECT_EQs name the window // that was echoed. The two-process half, with the child's own VmPeak, is // TcpLane.SupervisorProtocolControls' `hello_asks_64_gib`. +// +// "Clamped" in the name is the plan's word; what Accept does is IGNORE the ask - it never reads +// the four counts for sizing - and the session is server-sized. Since the F fix round that is +// said out loud: an ask above the granted terms logs one MGLOG_W naming both sides (rule I: a +// silent difference between what a client configured and what it got is the kind this phase +// removes), and this case requires the line. RED before that fix: the line is absent. TEST(SessionHandshakeTest, AHelloAskingFor64GiBIsClampedToTheServersTermsAndAllocatesNothingOfIt) { using namespace ::MobileGL::Wire; constexpr Uint64 kAsk = 64ull << 30; @@ -291,7 +297,12 @@ TEST(SessionHandshakeTest, AHelloAskingFor64GiBIsClampedToTheServersTermsAndAllo sizes.CmdRingBytes = 4096; sizes.StageBytes = 4096; sizes.ReplyBytes = 4096; sizes.EventRingBytes = 4096; session.SetSegmentSizes(sizes); + const std::string before = ReadLog(); ASSERT_EQ(session.Accept(*server, &first), MOBILEGL_OK); + const std::string delta = ReadLog().substr(before.size()); + EXPECT_TRUE(Contains(delta, "MG_Remote server: Hello asked for windows the server does not grant")) + << "the 64 GiB ask was ignored silently; an operator comparing the client's configured " + "windows with the server's would see no line. Log delta:\n" << delta; const auto reply = ReadHandshakeFrame(*client); ASSERT_FALSE(reply.empty()); ::flatbuffers::Verifier verifier(reply.data(), reply.size()); diff --git a/MobileGL/MG_Test/Wire/SocketTransportTest.cpp b/MobileGL/MG_Test/Wire/SocketTransportTest.cpp index fe110796..b0c34078 100644 --- a/MobileGL/MG_Test/Wire/SocketTransportTest.cpp +++ b/MobileGL/MG_Test/Wire/SocketTransportTest.cpp @@ -180,6 +180,15 @@ TEST(SocketTransportTest, TheTokenComparisonAnswersLengthAndPrefixTheSameWay) { '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'}; EXPECT_FALSE(ConstantTimeTokenMatch(expected, embedded, sizeof(embedded))); EXPECT_FALSE(ConstantTimeTokenMatch(nullptr, "anything", 8)); + // F fix round: the loop runs a fixed kAuthTokenCompareBytes (64) so its trip count does not + // tell a peer the configured token's length; a token LONGER than that is still compared in + // full - the last byte of a 70-byte token decides, it is not past the width. + std::string longToken(70, 'k'); + std::string longPresented = longToken; + EXPECT_TRUE(ConstantTimeTokenMatch(longToken.c_str(), longPresented.c_str(), longPresented.size())); + longPresented[69] = 'K'; + EXPECT_FALSE(ConstantTimeTokenMatch(longToken.c_str(), longPresented.c_str(), longPresented.size())); + EXPECT_FALSE(ConstantTimeTokenMatch(longToken.c_str(), longToken.c_str(), 64)); } // PH-7 (4), ID-P7-3. THE DATA CONNECTION'S FIRST FRAME IS READ EXACTLY, AND ONLY A DataBind diff --git a/scripts/ci/tcp_supervisor_smoke.py b/scripts/ci/tcp_supervisor_smoke.py index 214f9471..8f993915 100644 --- a/scripts/ci/tcp_supervisor_smoke.py +++ b/scripts/ci/tcp_supervisor_smoke.py @@ -591,6 +591,28 @@ def main(): 'fresh_control': still_busy} assert process.poll() is None, 'the refused DataBinds killed the supervisor' + # ---- F fix round: A GARBAGE FIRST FRAME WHILE BUSY IS MalformedHello, NOT Busy ------ + # + # While a session is live the supervisor reads a new connection's first frame to route + # it. A frame with the wrong magic (or a length over 1 MiB) was refused Busy, naming a + # condition the connection does not have: it is not a control frame at all, and it now + # gets the word ServerSession::Accept uses for a first frame that is not a verifiable + # Hello. Busy stays the answer for a well-formed frame that is not a DataBind (the + # `busy` control above). RED before the fix: code 7. + with held_session(port, schema, flatbuffers, + hello(schema, flatbuffers, fingerprint=fingerprint)) as (control, welcome): + assert welcome.get('welcome', 0), welcome + garbage = connect_control(port) + try: + garbage.sendall(b'XXXX' + bytes(60)) + malformed = receive(garbage, schema) + finally: + garbage.close() + assert malformed.get('code') == 8 and malformed.get('detail') == ( + 'first frame is not a control frame'), malformed + evidence['garbage_first_frame_while_busy'] = malformed + assert process.poll() is None, 'a garbage first frame killed the supervisor' + with supervisor(args.server, args.out / 'same-build-supervisor.log', same_build=True) as (port, _, _log): strict = exchange(port, schema, hello(schema, flatbuffers, fingerprint=fingerprint)) assert strict.get('code') == 3, strict