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.

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

[session] [crypto] Sessions should use correct Source Node ID in NONCE. #10161

Closed
turon opened this issue Oct 2, 2021 · 4 comments
Closed

[session] [crypto] Sessions should use correct Source Node ID in NONCE. #10161

turon opened this issue Oct 2, 2021 · 4 comments

Comments

@turon
Copy link
Contributor

turon commented Oct 2, 2021

Problem

Based on current testing of chip-tool with all-clusters-app, it appears all nodes are using NodeID = 0 for the nonce/IV.
This is perhaps not the end of the world security-wise, as sessions will typically have a unique key.
Per spec: "For unicast secure session communication, both the Source Node ID and Destination Node ID SHOULD be omitted, as they are derivable from the session context referenced by the Session ID."
NOTE: utilizing the proper Source Node ID for the nonce will complicate the sniffer dissector and other such diagnostic tools.

Proposed Solution

Nodes need to encrypt with the source NodeID they advertised/negotiated during session establishment.
SessionManager and Session objects need to track the peer's NodeID and use that address for the nonce in decryption.

Specifically, CryptoContext::GetIV should not call header.GetSourceNodeId() unless SessionType = group and Sflag=1.
Instead, the peer node id as authenticated during CASE should be used.

For PASE, this is less of an issue.

@bzbarsky-apple bzbarsky-apple added crypto spec Mismatch between spec and implementation labels Oct 9, 2021
@andy31415 andy31415 added v1_triage_split_9 crypto V1.0 spec Mismatch between spec and implementation and removed crypto V1.0 spec Mismatch between spec and implementation labels Jan 30, 2022
@holbrookt
Copy link
Contributor

@turon is this needed for 1.0?

@woody-apple woody-apple added V1.X and removed v1_secondary_triage V1.0 spec Mismatch between spec and implementation labels Feb 8, 2022
@woody-apple
Copy link
Contributor

Per spec, this is a should, so this can wait until v1.X as long as we don't have a security issue here.

@tcarmelveilleux
Copy link
Contributor

It was intended in the security design for CASE that the source node ID would always be included in the nonce, but from the secure session rather than the header. The SHOULD mentioned above is actually for something else.

@turon
Copy link
Contributor Author

turon commented May 24, 2022

Fixed #16098

@turon turon closed this as completed May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants