mirror of
https://github.com/MobileGL-Dev/MobileGL
synced 2026-11-07 01:53:24 +09:00
[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)
This commit is contained in:
@@ -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<std::uint8_t> 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 &&
|
||||
|
||||
@@ -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<Transport::StreamLink>();
|
||||
created = link->AttachOwnedDeferred(m_sizes, Transport::TransportRoleTag::ServerConsumer);
|
||||
if (created == MOBILEGL_OK) {
|
||||
auto link = std::make_unique<Transport::StreamLink>();
|
||||
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<Uint8>>();
|
||||
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<unsigned long long>(terms->cmdWindowBytes()),
|
||||
static_cast<unsigned long long>(terms->stageWindowBytes()),
|
||||
static_cast<unsigned long long>(terms->eventWindowBytes()),
|
||||
static_cast<unsigned long long>(terms->maxReplyBytes()),
|
||||
static_cast<unsigned long long>(cmdWindow),
|
||||
static_cast<unsigned long long>(stageWindow),
|
||||
static_cast<unsigned long long>(eventWindow),
|
||||
static_cast<unsigned long long>(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.
|
||||
|
||||
@@ -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<unsigned char>(expected[index]) : 0u;
|
||||
const unsigned char theirs =
|
||||
index < presentedSize ? static_cast<unsigned char>(presented[index]) : 0u;
|
||||
difference |= static_cast<unsigned char>(
|
||||
static_cast<unsigned char>(expected[index]) ^ theirs);
|
||||
difference |= static_cast<unsigned char>(ours ^ theirs);
|
||||
}
|
||||
return difference == 0;
|
||||
}
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user