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

Secure session establishment can fail if local MRP parameters cannot fit within 16 bits #17812

Closed
msandstedt opened this issue Apr 27, 2022 · 4 comments · Fixed by #17978
Closed

Secure session establishment can fail if local MRP parameters cannot fit within 16 bits #17812

msandstedt opened this issue Apr 27, 2022 · 4 comments · Fixed by #17978
Assignees
@msandstedt
Labels
secure channel

Comments

@msandstedt
Copy link
Contributor

msandstedt commented Apr 27, 2022 •
edited
Loading

Problem

Nodes can exchange MRP parameters during CASE/PASE handshake with the mrp-parameter-struct, which encloses active and idle retry times as 16-bit values.

However, maximum bounds for these are 360,000 milliseconds. It appears that when populating these, the pairing session code will simply fail. The implication is that long MRP intervals can likely break PASE and CASE at this location in the code:

https://github.com/project-chip/connectedhomeip/blob/master/src/transport/PairingSession.cpp#L62

Proposed Solution

  • consider saturating values for placement in mrp-parameter-struct if local values don't fit
  • or consider not sending these if they can't fit - sending is optional
  • however, we shouldn't fail secure session establishment as we do now for values that overflow
@msandstedt msandstedt changed the title CASE can fail if local MPR parameters cannot fit within 16 bits. Secure session establishment can fail if local RMP parameters cannot fit within 16 bits Apr 27, 2022
@bzbarsky-apple
Copy link
Contributor

@msandstedt This is a spec-level issue, right?

I do wonder whether it's worth just changing the units on these values from ms to 10s of ms or 100s of ms, depending on the granularity we think we actually need....

@msandstedt msandstedt self-assigned this May 2, 2022
@robszewczyk
Copy link
Contributor

Makes sense to me. I'd prefer to go with the approach that @tcarmelveilleux proposed in the PR and keep the granularity of ms rather than invent new units. Please approve the spec PR, and I will merge it.

@msandstedt
Copy link
Contributor Author

@robszewczyk ,

Thanks. Makes sense. Will do.

@msandstedt msandstedt changed the title Secure session establishment can fail if local RMP parameters cannot fit within 16 bits Secure session establishment can fail if local MRP parameters cannot fit within 16 bits May 2, 2022
msandstedt added a commit to msandstedt/connectedhomeip that referenced this issue May 2, 2022
@msandstedt
Change mrp-parameter-struct to hold 32-bit milliseconds
d1578c4 
Nodes store and advertise MRP parameters as 32-bit values.  However,
the mrp-parameter-struct had been specified to only hold 16-bit values
on the wire.  This would lead to session establishment failures with
nodes attempting to exchange values in excess of 65536 milliseconds,
despite the fact that values up to 360,000 milliseconds are legal.

This corrects the problem to allow up-to 32-bit values per the spec
change here:

https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5173

In most cases, peers will be using smaller MRP values and and will
therefore still exchange 1 or 2-byte fields on the wire, making this
change mostly backward compatible.

Testing: verification of successful exchange of larger MRP values up
to 360,000 has been added to TestCASESession.cpp.  TestTxtFields.cpp
already has coverage for advertisement of large values.

Fixes project-chip#17812
@msandstedt msandstedt mentioned this issue May 2, 2022
Merged
@msandstedt
Copy link
Contributor Author

@robszewczyk , @tcarmelveilleux ,

See the sdk PR here: #17978

msandstedt added a commit to msandstedt/connectedhomeip that referenced this issue May 2, 2022
@msandstedt
Change mrp-parameter-struct to hold 32-bit milliseconds
2a87ab1 
Nodes store and advertise MRP parameters as 32-bit values.  However,
the mrp-parameter-struct had been specified to only hold 16-bit values
on the wire.  This would lead to session establishment failures with
nodes attempting to exchange values in excess of 65536 milliseconds,
despite the fact that values up to 360,000 milliseconds are legal.

This corrects the problem to allow up-to 32-bit values per the spec
change here:

https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5173

In most cases, peers will be using smaller MRP values and and will
therefore still exchange 1 or 2-byte fields on the wire, making this
change mostly backward compatible.

Testing: verification of successful exchange of larger MRP values up
to 360,000 has been added to TestCASESession.cpp.  TestTxtFields.cpp
already has coverage for advertisement of large values.

Fixes project-chip#17812
andy31415 pushed a commit that referenced this issue May 3, 2022
@msandstedt
Change mrp-parameter-struct to hold 32-bit milliseconds (#17978)
2103225 
* Change mrp-parameter-struct to hold 32-bit milliseconds

Nodes store and advertise MRP parameters as 32-bit values.  However,
the mrp-parameter-struct had been specified to only hold 16-bit values
on the wire.  This would lead to session establishment failures with
nodes attempting to exchange values in excess of 65536 milliseconds,
despite the fact that values up to 360,000 milliseconds are legal.

This corrects the problem to allow up-to 32-bit values per the spec
change here:

https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5173

In most cases, peers will be using smaller MRP values and and will
therefore still exchange 1 or 2-byte fields on the wire, making this
change mostly backward compatible.

Testing: verification of successful exchange of larger MRP values up
to 360,000 has been added to TestCASESession.cpp.  TestTxtFields.cpp
already has coverage for advertisement of large values.

Fixes #17812

* per bzbarsky, s/verySleep/verySleepy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@msandstedt msandstedt

Labels
secure channel
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change mrp-parameter-struct to hold 32-bit milliseconds msandstedt/connectedhomeip
3 participants
@robszewczyk @msandstedt @bzbarsky-apple