Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Acks to unencrypted messages broken after exchange closure #10515

Closed
bzbarsky-apple opened this issue Oct 14, 2021 · 3 comments · Fixed by #19398
Closed

Acks to unencrypted messages broken after exchange closure #10515

bzbarsky-apple opened this issue Oct 14, 2021 · 3 comments · Fixed by #19398
Assignees
@bzbarsky-apple
Labels
reliability spec Mismatch between spec and implementation V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

Consider the case of an unencrypted message coming in after the exchange it was targeting has been closed (e.g. it's a duplicate of the last message in a handshake, because the other side never got our ack).

We land in ExchangeManager::OnMessageReceived and don't match any existing exchanges. We create a new exchange to handle the standalone ack. We do this check:

        if (ec->IsEncryptionRequired() != packetHeader.GetFlags().Has(Header::FlagValues::kEncryptedMessage))
            ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_INVALID_MESSAGE_TYPE));
            ec->Close();
            return;

and the condition tests true, because the standalone ack exchange has no delegate, hence uses the mDefaultExchangeDispatch of the exchange manager, which is an ApplicationExchangeDispatch, which requires encryption.

So we never send the ack.

Proposed Solution

Fix things so we send acks properly.

@pan-apple @kghost
@bzbarsky-apple bzbarsky-apple added the spec Mismatch between spec and implementation label Oct 14, 2021
@kghost
Copy link
Contributor

kghost commented Oct 15, 2021 •
edited
Loading

Can we remove exchange dispatcher now ? As you can see, there is almost no difference between application dispatcher and session establishment dispatcher. The major code divergence have moved into session manager, because it is handling both secure and unauthenticated session now.

@kghost
Copy link
Contributor

kghost commented Feb 1, 2022

This should be fixed by #12794

@kghost kghost closed this as completed Feb 1, 2022
@bzbarsky-apple
Copy link
Contributor Author

This is not fixed. The behavior is still exactly the same: null-delegate exchanges default to ApplicationExchangeDispatch::Instance(), which requires encryption.

@bzbarsky-apple bzbarsky-apple reopened this Feb 1, 2022
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 9, 2022
@bzbarsky-apple
Ensure that we send MRP acks to incoming messages as needed.
fde9af8 
There were two ways we could fail to send an ack to an incoming reliable message:

1) If we found no matching handler, and hence created an ephemeral exchange to
handle the message, but the message was unencrypted.  In this case our ephemeral
exchange would return true for IsEncryptionRequired(), because it would default
to an ApplicationExchangeDispatch, and we would never call into
ExchangeContext::HandleMessage.

2) If ExchangeMessageDispatch::MessagePermitted returned false for the message.
In particular, for an ApplicationExchangeDispatch, this would happen for all the
handshake messages except StatusReport.

The fix for issue 1 is to ensure we always call into HandleMEssage if we manage
to allocate an exchange.  If there is an encryption mismatch, which only matters
when the exchange is non-ephemeral, we close the exchange first to prevent event
delivery to the delegate.

The fix for issue 2 is to move the MRP processing out of ExchangeMessageDispatch
and into ExchangeContext, and to move the MessagePermitted check so the only
thing it prevents is delivery of the message to the delegate, not any other
processing by the exchange.

Fixes project-chip#10515
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 9, 2022
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 9, 2022
@bzbarsky-apple
Ensure that we send MRP acks to incoming messages as needed.
4bfbee0 
There were two ways we could fail to send an ack to an incoming reliable message:

1) If we found no matching handler, and hence created an ephemeral exchange to
handle the message, but the message was unencrypted.  In this case our ephemeral
exchange would return true for IsEncryptionRequired(), because it would default
to an ApplicationExchangeDispatch, and we would never call into
ExchangeContext::HandleMessage.

2) If ExchangeMessageDispatch::MessagePermitted returned false for the message.
In particular, for an ApplicationExchangeDispatch, this would happen for all the
handshake messages except StatusReport.

The fix for issue 1 is to ensure we always call into HandleMEssage if we manage
to allocate an exchange.  If there is an encryption mismatch, which only matters
when the exchange is non-ephemeral, we close the exchange first to prevent event
delivery to the delegate.

The fix for issue 2 is to move the MRP processing out of ExchangeMessageDispatch
and into ExchangeContext, and to move the MessagePermitted check so the only
thing it prevents is delivery of the message to the delegate, not any other
processing by the exchange.

Fixes project-chip#10515
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 9, 2022
@bzbarsky-apple
Ensure that we send MRP acks to incoming messages as needed.
e7795a8 
There were two ways we could fail to send an ack to an incoming reliable message:

1) If we found no matching handler, and hence created an ephemeral exchange to
handle the message, but the message was unencrypted.  In this case our ephemeral
exchange would return true for IsEncryptionRequired(), because it would default
to an ApplicationExchangeDispatch, and we would never call into
ExchangeContext::HandleMessage.

2) If ExchangeMessageDispatch::MessagePermitted returned false for the message.
In particular, for an ApplicationExchangeDispatch, this would happen for all the
handshake messages except StatusReport.

The fix for issue 1 is to ensure we always call into HandleMEssage if we manage
to allocate an exchange.  If there is an encryption mismatch, which only matters
when the exchange is non-ephemeral, we close the exchange first to prevent event
delivery to the delegate.

The fix for issue 2 is to move the MRP processing out of ExchangeMessageDispatch
and into ExchangeContext, and to move the MessagePermitted check so the only
thing it prevents is delivery of the message to the delegate, not any other
processing by the exchange.

Fixes project-chip#10515
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 9, 2022
@bzbarsky-apple
Ensure that we send MRP acks to incoming messages as needed.
fa18784 
There were two ways we could fail to send an ack to an incoming reliable message:

1) If we found no matching handler, and hence created an ephemeral exchange to
handle the message, but the message was unencrypted.  In this case our ephemeral
exchange would return true for IsEncryptionRequired(), because it would default
to an ApplicationExchangeDispatch, and we would never call into
ExchangeContext::HandleMessage.

2) If ExchangeMessageDispatch::MessagePermitted returned false for the message.
In particular, for an ApplicationExchangeDispatch, this would happen for all the
handshake messages except StatusReport.

The fix for issue 1 is to ensure we always call into HandleMEssage if we manage
to allocate an exchange.  If there is an encryption mismatch, which only matters
when the exchange is non-ephemeral, we close the exchange first to prevent event
delivery to the delegate.

The fix for issue 2 is to move the MRP processing out of ExchangeMessageDispatch
and into ExchangeContext, and to move the MessagePermitted check so the only
thing it prevents is delivery of the message to the delegate, not any other
processing by the exchange.

Fixes project-chip#10515
bzbarsky-apple added a commit that referenced this issue Jun 10, 2022
@bzbarsky-apple
Ensure that we send MRP acks to incoming messages as needed. (#19398)
10a5389 
There were two ways we could fail to send an ack to an incoming reliable message:

1) If we found no matching handler, and hence created an ephemeral exchange to
handle the message, but the message was unencrypted.  In this case our ephemeral
exchange would return true for IsEncryptionRequired(), because it would default
to an ApplicationExchangeDispatch, and we would never call into
ExchangeContext::HandleMessage.

2) If ExchangeMessageDispatch::MessagePermitted returned false for the message.
In particular, for an ApplicationExchangeDispatch, this would happen for all the
handshake messages except StatusReport.

The fix for issue 1 is to ensure we always call into HandleMEssage if we manage
to allocate an exchange.  If there is an encryption mismatch, which only matters
when the exchange is non-ephemeral, we close the exchange first to prevent event
delivery to the delegate.

The fix for issue 2 is to move the MRP processing out of ExchangeMessageDispatch
and into ExchangeContext, and to move the MessagePermitted check so the only
thing it prevents is delivery of the message to the delegate, not any other
processing by the exchange.

Fixes #10515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bzbarsky-apple bzbarsky-apple

Labels
reliability spec Mismatch between spec and implementation V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Ensure that we send MRP acks to incoming messages as needed. bzbarsky-apple/connectedhomeip
5 participants
@kghost @tcarmelveilleux @andy31415 @woody-apple @bzbarsky-apple