[BUG] [crash] [mrp] Receiving a mid-exchange message without ACK bit set will crash

[turon]

Reproduction steps

1. Start chip-all-clusters server app
2. Initiate PASE as client
3. client sends PBKDFRequest
4. server sends PBKDFResponse
5. client sends PAKE1 message with  A flag (ACK) = 0 (and sends no standalone acks)
6. chip-all-clusters will crash

An erroneous client should not crash the remote server device.

[1664028216.560999][1354655:1354655] CHIP:SC: Received spake2p msg1
[1664028216.561619][1354655:1354655] CHIP:SPT: VerifyOrDie failure at ../../src/messaging/ReliableMessageMgr.cpp:195: !rc->IsMessageNotAcked()

Thread 1 "chip-all-cluste" received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
49	../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
#1  0x00007ffff7106546 in __GI_abort () at abort.c:79
#2  0x000055555557f536 in chipAbort() () at ../../src/lib/support/CodeUtils.h:506
#3  0x000055555576944b in chip::Messaging::ReliableMessageMgr::AddToRetransTable(chip::Messaging::ReliableMessageContext*, chip::Messaging::ReliableMessageMgr::RetransTableEntry**) (this=0x55555584f1e8 <chip::Server::sServer+1448>, rc=0x5555558da848, rEntry=0x7fffffffbb98)
    at ../../src/messaging/ReliableMessageMgr.cpp:195
#4  0x0000555555765fd6 in chip::Messaging::ExchangeMessageDispatch::SendMessage(chip::SessionManager*, chip::SessionHandle const&, unsigned short, bool, chip::Messaging::ReliableMessageContext*, bool, chip::Protocols::Id, unsigned char, chip::System::PacketBufferHandle&&)
    (this=0x55555584ec20 <chip::SessionEstablishmentExchangeDispatch::Instance()::instance>, sessionManager=0x55555584ecf8 <chip::Server::sServer+184>, session=..., exchangeId=0, isInitiator=false, reliableMessageContext=0x5555558da848, isReliableTransmission=true, protocol=..., type=35 '#', message=...) at ../../src/messaging/ExchangeMessageDispatch.cpp:70
#5  0x00005555557642d2 in chip::Messaging::ExchangeContext::SendMessage(chip::Protocols::Id, unsigned char, chip::System::PacketBufferHandle&&, chip::BitFlags<chip::Messaging::SendMessageFlags, unsigned short> const&) (this=0x5555558da840, protocolId=..., msgType=35 '#', msgBuf=..., sendFlags=...)
    at ../../src/messaging/ExchangeContext.cpp:214
#6  0x0000555555769047 in chip::Messaging::ExchangeContext::SendMessage<chip::Protocols::SecureChannel::MsgType, void>(chip::Protocols::SecureChannel::MsgType, chip::System::PacketBufferHandle&&, chip::BitFlags<chip::Messaging::SendMessageFlags, unsigned short> const&)
    (this=0x5555558da840, msgType=chip::Protocols::SecureChannel::MsgType::PASE_Pake2, msgPayload=..., sendFlags=...)
    at ../../src/messaging/ExchangeContext.h:125
#7  0x00005555557e8e26 in chip::PASESession::HandleMsg1_and_SendMsg2(chip::System::PacketBufferHandle&&)
     (this=0x5555558510f0 <chip::Server::sServer+9392>, msg1=...) at ../../src/protocols/secure_channel/PASESession.cpp:610
#8  0x00005555557ea577 in chip::PASESession::OnMessageReceived(chip::Messaging::ExchangeContext*, chip::PayloadHeader const&, chip::System::PacketBufferHandle&&) (this=0x5555558510f0 <chip::Server::sServer+9392>, exchange=0x5555558da840, payloadHeader=..., msg=...)
    at ../../src/protocols/secure_channel/PASESession.cpp:830
#9  0x0000555555765266 in chip::Messaging::ExchangeContext::HandleMessage(unsigned int, chip::PayloadHeader const&, chip::BitFlags<chip::Messaging::MessageFlagValues, unsigned int>, chip::System::PacketBufferHandle&&)
    (this=0x5555558da840, messageCounter=2, payloadHeader=..., msgFlags=..., msgBuf=...) at ../../src/messaging/ExchangeContext.cpp:587
#10 0x000055555576804d in operator()<chip::Messaging::ExchangeContext>(chip::Messaging::ExchangeContext*) const
    (__closure=0x7fffffffc510, ec=0x5555558da840) at ../../src/messaging/ExchangeMgr.cpp:249
#11 0x00005555557684d5 in chip::internal::LambdaProxy<chip::Messaging::ExchangeContext, chip::Messaging::ExchangeManager::OnMessageReceived(const chip::PacketHeader&, const chip::PayloadHeader&, const chip::SessionHandle&, chip::SessionMessageDelegate::DuplicateMessage, chip::System::PacketBufferHandle&&)::<lambda(auto:6*)> >::Call(void *, void *) (context=0x7fffffffc510, target=0x5555558da840) at ../../src/lib/support/Pool.h:126
#12 0x00005555556b00c1 in chip::internal::HeapObjectList::ForEachNode(void*, chip::Loop (*)(void*, void*))
    (this=0x55555584f1b8 <chip::Server::sServer+1400>, context=0x7fffffffc510, lambda=0x5555557684b2 <chip::internal::LambdaProxy<chip::Messaging::ExchangeContext, chip::Messaging::ExchangeManager::OnMessageReceived(const chip::PacketHeader&, const chip::PayloadHeader&, const chip::SessionHandle&, chip::SessionMessageDelegate::DuplicateMessage, chip::System::PacketBufferHandle&&)::<lambda(auto:6*)> >::Call(void *, void *)>)
    at ../../src/lib/support/Pool.cpp:126
#13 0x00005555557680be in chip::HeapObjectPool<chip::Messaging::ExchangeContext>::ForEachActiveObject<chip::Messaging::ExchangeManager::OnMessageReceived(const chip::PacketHeader&, const chip::PayloadHeader&, const chip::SessionHandle&, chip::SessionMessageDelegate::DuplicateMessage, chip::System::PacketBufferHandle&&)::<lambda(auto:6*)> >(struct {...} &&) (this=0x55555584f1a8 <chip::Server::sServer+1384>, function=...)
    at ../../src/lib/support/Pool.h:402
#14 0x00005555557674b4 in chip::Messaging::ExchangeManager::OnMessageReceived(chip::PacketHeader const&, chip::PayloadHeader const&, chip::SessionHandle const&, chip::SessionMessageDelegate::DuplicateMessage, chip::System::PacketBufferHandle&&)
    (this=0x55555584f190 <chip::Server::sServer+1360>, packetHeader=..., payloadHeader=..., session=..., isDuplicate=chip::SessionMessageDelegate::DuplicateMessage::No, msgBuf=...) at ../../src/messaging/ExchangeMgr.cpp:242
#15 0x00005555557711cf in chip::SessionManager::UnauthenticatedMessageDispatch(chip::PacketHeader const&, chip::Transport::PeerAddress const&, chip::System::PacketBufferHandle&&) (this=0x55555584ecf8 <chip::Server::sServer+184>, packetHeader=..., peerAddress=..., msg=...)
    at ../../src/transport/SessionManager.cpp:638
--Type <RET> for more, q to quit, c to continue without paging--q
Quit
(gdb) fr 3
#3  0x000055555576944b in chip::Messaging::ReliableMessageMgr::AddToRetransTable (this=0x55555584f1e8 <chip::Server::sServer+1448>, 
    rc=0x5555558da848, rEntry=0x7fffffffbb98) at ../../src/messaging/ReliableMessageMgr.cpp:195
195	    VerifyOrDie(!rc->IsMessageNotAcked());
(gdb) 

Bug prevalence

Repeatable, but requires a controller that sends invalid mrp

GitHub hash of the SDK that was being used

bd6fa79

Platform

core

Platform Version(s)

No response

Anything else?

Zipped pcapng file enclosed (github doesn't accept raw pcap).
crash-pcap.zip

[bzbarsky-apple]

So the situation here on the server is:

1. We send PBKDFResponse that expects an ack.
2. We got an app-level response to that message (PAKE1) without an ack (spec violation here).
3. We are trying to send an app-level response to the PAKE1 (PAKE2) and hitting this code:

   CHIP_ERROR ReliableMessageMgr::AddToRetransTable(ReliableMessageContext * rc, RetransTableEntry ** rEntry)
   {
       VerifyOrDie(!rc->IsMessageNotAcked());
   

I think this code is trying to protect against the mis-behavior of sending two messages in a row both of which expect an ack.... But crashing does seem a bit over the top, especially given that this can happen due to a bug on the other side of the exchange.

So what should be the behavior here? Fail to send the PAKE2? Replace the message waiting for an ack for this exchange (the PBKDFResponse) with the PAKE2? @turon

[turon]

Two reasonable behaviors could be:

1. Enforce spec-compliance by dropping the otherwise valid PAKE1 (from a SPAKE2 standpoint) due to invalid state and resend the PbkdfResponse
2. Attempt to process the incoming PAKE1 message and allow the state machine to proceed forward despite otherwise missing acks (use app level message as implicit ack and make servers more robust and flexible to slightly invalid commissioner implementations).

I tend to lean toward (1) as that would force commissioners implementations to be spec compliant in order to succeed at commissioning.

Regardless of the choice, I agree that crashing/rebooting the server device based on an erroneous commissioner implementation seems wrong.

[bzbarsky-apple]

This is a problem for commissioners too, if a buggy device sends PBKDFResponse without an ack included... Just bad all around.

Option 1 would be the exchange itself dropping incoming messages that are not acknowledging anything if it has a pending message-to-be-acknowledged, right?