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

Validate entropy init and random number generator on all platforms #10454

Closed
tcarmelveilleux opened this issue Oct 13, 2021 · 1 comment · Fixed by #11600
Closed

Validate entropy init and random number generator on all platforms #10454

tcarmelveilleux opened this issue Oct 13, 2021 · 1 comment · Fixed by #11600
Assignees
@tcarmelveilleux
Labels
crypto platform

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

Entropy initialization for DRBG_get_bytes() and random number generation (rand() from C++ standard library, GetRandU*()) is done per platform, often in *PlatformManagerImpl<ImplClass>::_InitChipStack().

It was found that the methods are varied and there are no tests (and no easy test, given the methods used) to ensure entropy sources are initialized properly and across the board.

Proposed Solution

  1. Add an entropy init and random number generator audit method in a core component that uses random numbers, during init, to allow showcasing whether the RNGs are initialized or whether each reset/boot we get the same random numbers in the same order.
  2. Have one owner for each platform run the audit so that the platform is validated for doing this platform-level init properly.
@tcarmelveilleux tcarmelveilleux self-assigned this Oct 13, 2021
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Oct 13, 2021
@tcarmelveilleux
Introduce a RNG determinism audit
2eaaca0 
- Add a temporary audit function in ExchangeMgr.cpp.
- ExchangeMgr.cpp is used because it is used by both
  `DeviceController` and `Server` (i.e. by all modalities)
  and it already relies on random numbers.
- Audit must be run manually since it needs to be done on targets
  and comparisons across reboot.
- Audit validates `GetRandU*` and `Crypto::DRBG_get_bytes()`,
  which may both use different underlying impl.

Testing done:

- Unit tests pass
- Manually tested on Linux by commenting-out `srand(seed);` in
  Entropy.cpp, which forces `GetRandU*` to give the same values
  across calls.
- If you are testing this, search for lines starting with `AUDIT:`
  in the log.

Issue project-chip#10454
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Oct 13, 2021
@tcarmelveilleux
Introduce a RNG determinism audit
e160290 
- Add a temporary audit function in ExchangeMgr.cpp.
- ExchangeMgr.cpp is used because it is used by both
  `DeviceController` and `Server` (i.e. by all modalities)
  and it already relies on random numbers.
- Audit must be run manually since it needs to be done on targets
  and comparisons across reboot.
- Audit validates `GetRandU*` and `Crypto::DRBG_get_bytes()`,
  which may both use different underlying impl.

Testing done:

- Unit tests pass
- Manually tested on Linux by commenting-out `srand(seed);` in
  Entropy.cpp, which forces `GetRandU*` to give the same values
  across calls.
- If you are testing this, search for lines starting with `AUDIT:`
  in the log.

Issue project-chip#10454
@tcarmelveilleux tcarmelveilleux mentioned this issue Oct 13, 2021
Merged
@todo todo bot mentioned this issue Oct 13, 2021
Closed
tcarmelveilleux added a commit that referenced this issue Oct 14, 2021
@tcarmelveilleux @restyled-commits @bzbarsky-apple
Introduce a RNG determinism audit (#10456)
14a4986 
* Introduce a RNG determinism audit

- Add a temporary audit function in ExchangeMgr.cpp.
- ExchangeMgr.cpp is used because it is used by both
  `DeviceController` and `Server` (i.e. by all modalities)
  and it already relies on random numbers.
- Audit must be run manually since it needs to be done on targets
  and comparisons across reboot.
- Audit validates `GetRandU*` and `Crypto::DRBG_get_bytes()`,
  which may both use different underlying impl.

Testing done:

- Unit tests pass
- Manually tested on Linux by commenting-out `srand(seed);` in
  Entropy.cpp, which forces `GetRandU*` to give the same values
  across calls.
- If you are testing this, search for lines starting with `AUDIT:`
  in the log.

Issue #10454

* Move audit to InitEntropy()

- InitEntropy is in `GenericPlatformManagerImpl` and should always
  be called. Lack of the logs will help catch bad init.

* Restyled by clang-format

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
@tcarmelveilleux
Copy link
Contributor Author

All platforms have passed the audit

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Nov 9, 2021
@tcarmelveilleux
Remove temporary RNG entropy audit logs
b3a7fd9 
- Remove the temporary RNG entropy audit now that project-chip#10454
  is complete.

Fixes project-chip#10454
Fixes project-chip#10526
Fixes project-chip#10527
@tcarmelveilleux tcarmelveilleux mentioned this issue Nov 9, 2021
Merged
woody-apple pushed a commit to tcarmelveilleux/connectedhomeip that referenced this issue Nov 9, 2021
@tcarmelveilleux @woody-apple
Remove temporary RNG entropy audit logs
6063421 
- Remove the temporary RNG entropy audit now that project-chip#10454
  is complete.

Fixes project-chip#10454
Fixes project-chip#10526
Fixes project-chip#10527
bzbarsky-apple pushed a commit that referenced this issue Nov 9, 2021
@tcarmelveilleux
Remove temporary RNG entropy audit logs (#11600)
f11de82 
- Remove the temporary RNG entropy audit now that #10454
  is complete.

Fixes #10454
Fixes #10526
Fixes #10527
PSONALl pushed a commit to PSONALl/connectedhomeip that referenced this issue Dec 3, 2021
@tcarmelveilleux
Remove temporary RNG entropy audit logs (project-chip#11600)
242456b 
- Remove the temporary RNG entropy audit now that project-chip#10454
  is complete.

Fixes project-chip#10454
Fixes project-chip#10526
Fixes project-chip#10527
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tcarmelveilleux tcarmelveilleux

Labels
crypto platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove temporary RNG entropy audit logs tcarmelveilleux/connectedhomeip
1 participant
@tcarmelveilleux