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

FindLocalNodeFromDestionationId uses uninitialized IPKs #17940

Closed
msandstedt opened this issue Apr 30, 2022 · 2 comments · Fixed by #17942
Closed

FindLocalNodeFromDestionationId uses uninitialized IPKs #17940

msandstedt opened this issue Apr 30, 2022 · 2 comments · Fixed by #17942
Assignees
@msandstedt
Labels
secure channel security

Comments

@msandstedt
Copy link
Contributor

msandstedt commented Apr 30, 2022 •
edited
Loading

Problem

After AddNOC, we should have one valid IPK. However, FindLocalNodeFromDestionationId is considering uninitialized keys as candidates, meaning it will accept a match if the initiator uses an all-zero hmac(IPK) because uninitialized keys have this value.

Proposed Solution

Make sure candidateDestinationIdSpan does not accept match against uninitialized IPK epoch keys.
@msandstedt
Copy link
Contributor Author

CC @tcarmelveilleux , @kianooshkarami

@msandstedt msandstedt changed the title candidateDestinationIdSpan uses uninitialized IPKs FindLocalNodeFromDestionationId uses uninitialized IPKs Apr 30, 2022
@msandstedt msandstedt self-assigned this Apr 30, 2022
@msandstedt
Copy link
Contributor Author

Looks to be an off-by-one error with this fix:

// Try every IPK candidate we have for a match
-        for (size_t keyIdx = 0; keyIdx <= ipkKeySet.num_keys_used; ++keyIdx)
+        for (size_t keyIdx = 0; keyIdx < ipkKeySet.num_keys_used; ++keyIdx)

msandstedt added a commit to msandstedt/connectedhomeip that referenced this issue Apr 30, 2022
@msandstedt
Fix off-by-one error in FindLocalNodeFromDestionationId
17d4a1f 
FindLocalNodeFromDestionationId is indexing 1 entry past the initialized
IPK epoch keys, with the result that an all-zero key is accepted when
one or two epoch-keys are installed.  If three epoch keys are installed,
this will reference out of bounds.

This commit corrects the loop bound in this method to fix the problem.

Testing: Manually tested with an initiator using an incorrect, all-zero
key.  Without the fix, CASE establishment succeeds.  With the fix, the
responder now correctly rejects the incoming establishment request.

Fixes project-chip#17940
@msandstedt msandstedt mentioned this issue Apr 30, 2022
Merged
bzbarsky-apple pushed a commit that referenced this issue May 2, 2022
@msandstedt
Fix off-by-one error in FindLocalNodeFromDestionationId (#17942)
cfb160f 
FindLocalNodeFromDestionationId is indexing 1 entry past the initialized
IPK epoch keys, with the result that an all-zero key is accepted when
one or two epoch-keys are installed.  If three epoch keys are installed,
this will reference out of bounds.

This commit corrects the loop bound in this method to fix the problem.

Testing: Manually tested with an initiator using an incorrect, all-zero
key.  Without the fix, CASE establishment succeeds.  With the fix, the
responder now correctly rejects the incoming establishment request.

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

@msandstedt msandstedt

Labels
secure channel security
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix off-by-one error in FindLocalNodeFromDestionationId msandstedt/connectedhomeip
1 participant
@msandstedt