BLE commissioning fails after unauthenticated session changes #9738
Comments
|
This also appears to happen on IP commissioning. |
Interesting. I could not reproduce it with IP commissioning on ::1 (against the "linux" all-clusters-app), which makes sense because that configuration is tested in CI... I wonder what the difference between the successful and failing IP commissioning scenarios. |
|
I haven't reproduced the problem, but I found some suspicious code in connectedhomeip/src/ble/BtpEngine.cpp Lines 458 to 460 in dddd5d1
|
|
Oh, I know what's going on most immediately! We are doing MRP over BTP. We are not supposed to be doing that! And when we try to do it, it's broken as it has been all along (because that code path never got taken, because it's not supposed to be taken). So the real question is why we are doing MRP over BTP. |
|
I'll copy my finding from slack. I found that in https://github.com/project-chip/connectedhomeip/blob/master/src/messaging/ExchangeContext.cpp#L115 |
|
Yep, that's what I just found. Before the state came back as non-null, which is how it worked. And that meshes with this being broken by #9269 somehow. |
|
In other places in #9269 where |
|
Ah, I see how this might need to work. Patch coming up, I hope. But more generally, it seems like we need to audit all |
We were incorrectly doing MRP over BTP, because there was no peer connection state for our insecure session when doing a SPAKE handshake. The various header include additions were needed to fix header files that used types without declaring them and hence depended on other headers being included before them. Fixes project-chip#9738
We were incorrectly doing MRP over BTP, because there was no peer connection state for our insecure session when doing a SPAKE handshake. The various header include additions were needed to fix header files that used types without declaring them and hence depended on other headers being included before them. Fixes #9738
We were incorrectly doing MRP over BTP, because there was no peer connection state for our insecure session when doing a SPAKE handshake. The various header include additions were needed to fix header files that used types without declaring them and hence depended on other headers being included before them. Fixes project-chip#9738
Problem
Using chip-tool to do BLE commissioning of an m5stack fails. On-network commissioning works, which is why CI is not affected.
Commissioner log:
On the commissionee side, it receives the PBKDF param request, sends PBKDF param response, then nothing good happens after that.
This was broken by #9269.
I stepped through what happens on the commissioner when it gets the PBKDF param response, on the merge changeset for #9269 (102c6e2). We find the right exchange, call
ExchangeContext::HandleMessage, land inExchangeMessageDispatch::OnMessageReceived, see there is a piggybacked ack, land inReliableMessageMgr::CheckAndRemRetransTableand return false. That causes the caller to error out, and then everything errors out up the callstack.The reason
CheckAndRemRetransTablereturns false is that inEncryptedPacketBufferHandle::GetMsgIdwe fail to parse the stored data to extract the message counter. And that happens because there place where the message data actually starts for*thisis 5 bytes into the packet buffer (based on where the actual bytes for the message counter appear).I tried modifying
SecureSessionMgr::SendPreparedMessagelike so:and that makes the problem go away. So something after that point is adding bytes to the front of the buffer that then confuse us when we later try to parse the buffer. In the case I was looking at just now those bytes are:
0x0d 0x00 0x00 0x3d 0x00.What confuses me is that at first glance the old code had the same basic structure as now:
SessionEstablishmentExchangeDispatch::SendPreparedMessagejust passed on the message to the transport. Now it callsmSessionMgr->SendPreparedMessage(), which does the same thing....I can dig at this some more on Friday, but I am off tomorrow, so if someone else wants to pick this up in the meantime...
@pan-apple @kghost @tcarmelveilleux @turon @carol-apple @sagar-apple
Proposed Solution
Figure out what's going on.
The text was updated successfully, but these errors were encountered: