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

ESP-IDF: Enable unconditionally #583

Merged
merged 1 commit into from
Jan 13, 2025
Merged

ESP-IDF: Enable unconditionally #583

merged 1 commit into from
Jan 13, 2025

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented Jan 10, 2025

After reviewing the updated ESP-IDF random documentation:

Given that ESP-IDF provides binding of getrandom/getentropy to esp_fill_random, it's clear they intend for
cryptographic libraries (such as their fork of WolfSSL) to use them. Furthermore, it seems like the only time the Hardware RNG would lack sufficient entropy is during early boot, so I added a section to our "Early boot" documentation noting this issue.

Also note that Rust's standard library unconditionally supports ESP-IDF for both hash seed generation and generating cryptographic random bytes, see https://github.com/rust-lang/rust/blob/62bf38fa600f4beb878d61c537837729d4ee689e/library/std/src/sys/random/espidf.rs#L7 I don't think there's a good reason to deviate from the standard library here.

@josephlr josephlr added this to the v0.3 milestone Jan 10, 2025
@josephlr josephlr requested a review from newpavlov January 10, 2025 05:39
After reviewing the updated ESP-IDF random documentation:
  - https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/system/random.html
  - https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf#rng

I think we should enable this backend by default.

Given that ESP-IDF provides such bindings, it's clear they intend for
cryptographic libraries (such as their fork of WolfSSL) to use them.
Furthurmore, it seems like the only time the Hardware RNG wouldn't be
seeded with sufficient entropy is during early boot, so I added a
section to our "Early boot" documentation noting this issue.

Also note that Rust's standard library unconditonally supports ESP-IDF
for both hash seed generation and generating cryptographic random bytes,
see https://github.com/rust-lang/rust/blob/62bf38fa600f4beb878d61c537837729d4ee689e/library/std/src/sys/random/espidf.rs#L7

Signed-off-by: Joe Richey <[email protected]>
@josephlr josephlr changed the title ESP-IDF: Enable by default and use getrandom() ESP-IDF: Enable unconditionally Jan 10, 2025
@josephlr
Copy link
Member Author

@newpavlov the first version of this PR used the getrandom bindings provided by ESP-IDF, but using those would require binding to ESP-IDF's errno, which seems unnecessarily complicated, so I just kept the implementation as-is.

@newpavlov
Copy link
Member

newpavlov commented Jan 10, 2025

it's clear they intend for cryptographic libraries (such as their fork of WolfSSL) to use them

It's not so clear for me, considering the following remark:

If none of the above conditions are true, the output of the RNG should be considered as pseudo-random only.

In other words, without a proper hardware setup users can get potentially predictable entropy.

Also note that Rust's standard library unconditionally supports ESP-IDF for both hash seed generation and generating cryptographic random bytes

IIRC std uses entropy sources only for seeding hash maps (well, technically it also uses it for things like thread ID generation on Windows, but it's not relevant), so it's less strict about entropy quality (e.g. it tries to use GRND_INSECURE on Linux). Meanwhile in our case we strive to always provide good quality entropy even during early boot.

I would be fine to use esp_fill_random for "insecure" functions (see #546), but I am hesitant to use them by default for "secure" functions without some kind of a check function, so to be conservative I think it makes sense to ask users to make a conscious decision.

See #397 for the earlier discussion. I also raised these concerns in espressif/esp-idf#8725, but, unfortunately, there are no updates on it.

cc @briansmith

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I guess we can simply say that it's a potential deficiency of the target and we did our best given the provided platform capabilities. In the issue they wrote the following:

Note that esp_random_fill() can be used to seed a software CSPRNG, which is done in mbedtls, as far as I know. I.e., esp_random_fill() is similar to the libc getentropy() function.

While I don't think that a getentropy()-like function should ever return potentially predictable entropy, it may be fine to defer to the intention of ESP IDF maintainers.

@newpavlov newpavlov merged commit 2648e60 into master Jan 13, 2025
59 checks passed
@newpavlov newpavlov deleted the espidf branch January 13, 2025 14:43
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

Successfully merging this pull request may close these issues.

2 participants