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

Is the ESPIDF implementation actually correct? #397

Open
briansmith opened this issue Feb 20, 2024 · 3 comments
Open

Is the ESPIDF implementation actually correct? #397

briansmith opened this issue Feb 20, 2024 · 3 comments

Comments

@briansmith
Copy link
Contributor

When reviewing the corresponding change to ring, briansmith/ring#1944, I noticed that the implementation in getrandom for ESPIDF seems really questionable. The ESPIDF documentation is quite vague on what promises its PRNG is making, especially when the OS isn't fully configured. It also encourages people to use a userspace CSPRNG; it isn't clear if this suggestion is intended to just address performance or security or both.

In particular, should the ESPIDF random API considered more of an entropy source, but not a full CSPRNG?

In ring we've taken the approach, temporarily, of using getrandom on this OS only if the user opts in with a feature flag that draws attention to this concern.

@briansmith
Copy link
Contributor Author

I also think that we should consider adding support for the custom feature for this "OS".

@newpavlov
Copy link
Member

newpavlov commented Feb 21, 2024

This issue has more information about esp_fill_random. This line certainly raises red flags:

When using the random number generator, make sure at least either the SAR ADC, high-speed ADC, or RTC20M_CLK is enabled. Otherwise, pseudo-random numbers will be returned.

I would expect to at least get an error code in this case. Also, I couldn't find whether they use proper whitening of entropy received from hardware. We probably should work with ESP IDF people to clarify exact guarantees and if necessary maybe ask them to add a separate function for cryptographically secure randomness.

@josephlr
Copy link
Member

Looking at espressif/esp-idf#8725 and the ESP-IDF RNG docs, it seems like the only thing we are missing API-wise is a function to check if we are in the "secure configuration" or not, which would basically check that either:

  • The RF subsystem is enabled
  • bootloader_random_enable() is active

My reading of the docs is that if one of those conditions is true, ESP-IDF guarantees esp_random will emit secure random numbers, so would be fine for use with this crate. Users will still want to use a wrapping CSPRNG if they are querying a bunch of randomness, as esp_random is rate limited. However, that's not an ESP-IDF specific issue. A bunch of our supported targets can't handle high throughput. This is why we already recommend using a user-space CSPRNG if you need a bunch of randomness.

For now we should probably try to convince Espressif to add an bool esp_random_is_secure() function.

Until such a function exists, we should probably just add a warning in the docs that you have to be running ESP-IDF in the secure configuration mentioned above to get cryptographically secure random numbers. Disabling support entirely seems counterproductive as it's not like there's anything better ESP-IDF users can do right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants