SBN_ProcessSubsFromPeer: peer-supplied SubCnt is unbounded

[nurdymuny]

Summary

SBN_ProcessSubsFromPeer() in fsw/src/sbn_subs.c reads a 16-bit SubCnt from the peer-supplied subscription message and iterates SubCnt times without checking against SBN_MAX_SUBS_PER_PEER. The inner ProcessSubFromPeer() does reject writes past the cap, so this isn't a memory-corruption bug — but the outer loop still spins up to 65535 iterations on each malformed packet, doing one Unpack_Data and one error-path round-trip per iteration, and floods the EVS log.

Location

fsw/src/sbn_subs.c:487 (SBN_ProcessSubsFromPeer):

uint16 SubCnt;
Unpack_UInt16(&Pack, &SubCnt);                  // peer controls this — 0..65535

int SubIdx = 0;
for (SubIdx = 0; SubIdx < SubCnt; SubIdx++)     // ← no upper bound
{
    CFE_SB_MsgId_t MsgID;
    Unpack_MsgID(&Pack, &MsgID);
    CFE_SB_Qos_t QoS;
    Unpack_Data(&Pack, &QoS, sizeof(QoS));

    SBN_Status = ProcessSubFromPeer(Peer, MsgID, QoS);

    if (SBN_Status != SBN_SUCCESS) {
        return SBN_Status;
    }
}

Threat model

SBN bridges cFS Software Bus messages between flight CPUs over UDP/TCP/DTN. If one CPU is compromised (or merely glitching), it can send subscription messages with SubCnt=0xFFFF to its peers. The receiving CPU then spends 65535 iterations in this loop on every such packet, suspending real work and flooding the event log. Repeat the message and the CPU is effectively pinned.

Fix

PR adds an explicit if (SubCnt > SBN_MAX_SUBS_PER_PEER) early-return with an EVS error, mirroring the upper-bound check the same function already does for the version-hash mismatch above.

Notes

• SBN_PACKED_HDR_SZ + sizeof(SBN_SubCnt_t) + (sizeof(CFE_SB_MsgId_t) + sizeof(CFE_SB_Qos_t)) * SBN_MAX_SUBS_PER_PEER is already the bound used for the maximum packet size (fsw/platform_inc/sbn_interfaces.h:43), so applying the same cap on the per-peer SubCnt is consistent with the existing design.
• The Pack_t machinery presumably bounds reads against the source buffer when Unpack_Data would run off the end, so the inner loop reads return zeros / safe values rather than OOB. The bug is wasted CPU + log spam, not memory disclosure.