-
Notifications
You must be signed in to change notification settings - Fork 696
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
Cryptographically-Secure Pseudo-Random Number Generator (CSPRNG) #522
Conversation
Interesting, and nicely executed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than my inline comments:
- I'd really like to see <stdint.h> explicitly included in every header / module that
intN_t
,uintN_t
are used - API documentation is insufficient. I'd like documentation explaining what a CSPRNG is. Documenting the structures for the corresponding drivers. Documenting semantics of function calls. etc For example "I/Q LSBs-based seeder" does not convey sufficient information to the new developer.
These are overall very desirable additions, but I do have concerns about the design and implementation of things - especially on the CC2538-specific parts - as well as about the fact that API documentation is somewhat on the thin side. In other news, perhaps this could be an opportunity to think about modernised APIs for random number generation (#101) as well as for seeding. Perhaps (not as part of this particular pull), we could end up with drivers for random generators suitable for crypto and random generators for other stuff (e.g. backoffs). |
3de8eff
to
4621387
Compare
Thanks for your feedback. I added several doxygen annotations and two compile tests. |
4985a4e
to
ac408b7
Compare
UPDATE: Seeders now notify the CSPRNG of new seeds and the CSPRNG itself takes care of mixing new seeds with the current one. This way, we can do seeding at more appropriate places,e.g., reading power-up SRAM states earlier at start up and sampling I/Q data after the radio was initialized. Also, this approach is suitable for doing reseeding at runtime - simply call |
Rebased and removed compilation tests because if we enable the CSPRNG by default, the current tests compile the CSPRNG already. Let me know you require further changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to see the module used somewhere. Maybe an example under examples/libs
?
Also I'm not sure if we want this by default given it's not used anywhere (not saying I'm against, genuinely wondering). Couldn't some protocol already in place (DTLS?) benefit from this straight away?
4fe772a
to
94a3cde
Compare
|
One last little thing on my side: as you add a new example, please add a compile test for it on a least one platform. We'll need @g-oikonomou 's approval here. |
According to the paper "Cryptographic Randomness on a CC2538: a Case Study", DTLS does require cryptographic random numbers unless a pre-shared key is used. However, I think I can not change tinyDTLS to use my CSPRNG inside this PR because tinyDTLS is located in a different repository. Another interesting message of this paper is that I/Q readings may be biased under strong interference. As a remedy, I am now waiting for the channel to become clear when getting I/Q readings. |
2fdfb73
to
a9f55e4
Compare
Hi @kkrentz we just discussed this internally again. Just wondering whether you are still active/interested here? Should we revisit this? Do you have any time to do any further work if required? |
I can take care of further adjustments. |
That's great. Thanks. I'll have a good look at it again |
881a765
to
4498838
Compare
1f0a92f
to
232dd0c
Compare
I am not familiar with the cc2538 platform and had trouble reviewing (or testing) this PR with confidence. I was thus thinking to do a cc13xx/cc26xx seeder, but after looking at the task it seems to be overly coupled with csprng (platform developers need to familiarize themselves with csprng). I wonder if we can make the seeders generic, reduce the coupling between the CSPRNG and seeders, and in the process make supporting new platforms much clearer and simpler. Thinking aloud: @kkrentz Could we consider the iq-seeder and sram-seeder as simply True RNGs? If so, we could make a common API for TRNGs (e.g. This is somewhat related to #101, but can probably be handled separately. RIOT-OS HWRNG HAL for reference (but does not guarantee true random). |
Currently, my seeders only produce 256 true random bits. They are not for general purpose use. However, we can seed Seeding at start up would be insufficient to achieve forward and backward security. After all, some "seeders" may have to collect entropy and cannot run directly at start up. Ideally, there should be multiple seeders to have a fallback if one is compromised. |
I don't necessary see this as an issue. I would imagine a TRNG API to be described as something like "Expensive, with no guarantee on amount or at which time random data can be provided. Use with caution as security-related modules in the OS such as csprng may rely on it.".
My example with start-up was inspired by this PR which only does seeding at boot(?). There is of course no issue with asking the TRNG API for data at a later stage (if as you say entropy must be collected), or ask for more data to re-seed. I would imagine a simple csprng-seed-process could fill such a task (which might provide another argument for a platform-agnostic TRNG API as it would allow the process to also be made platform-agnostic and simply call the TRNG API (as opposed to being aware of cc2538-sram-seeder, iq-seeder, etc.)). But again, if I understand correctly this is something that is not yet solved/implemented for either this PR or a TRNG API approach.
I don't see any issue with a TRNG API implementation using multiple sources. For example for cc2538 it could utilize both approaches in this PR (radio and sram), and have a strategy on how it uses these to supply callers with random data. (it might actually be an argument for the TRNG abstraction as the reduced coupling with csprng allows for any number of sources being added or used without needing changes in csprng) Please let me know if I am completely barking at the wrong tree here. Or if I am not conveying myself clearly, I can make a draft PR with some pseudo-implementation to illustrate my points. |
I see two problems with your idea of a "seeding process". First, if I, e.g., seed with I/Q LSBs at runtime, this must be coordinated with the MAC driver. Likewise, the SRAM seeder can only collect entropy at specific times, namely when waking up from LPM 2. Second, that seeding process would need to know which "seeders" exist. By contrast, my current design defaults to use all available seeders without any configuration effort. A more general concern with your suggested TRNG API is that I see no use case for it. I have been happily using the CSPRNG in various protocols and never encountered the need for using true random numbers instead of |
Sorry for the late follow-up. I believe we understand each other viewpoints, and although I would still love to see some more abstraction I don't think we should stall the PR due to this - I can always propose some refactoring in a future PR. I also note that George and Simon looked through this without such concerns. As soon as I find the time I will review the code, and hopefully we can finally get this merged :) |
Thx for the changes. I am reluctant to merge this alone as I don't usually deal with cc2538 or security - let's see if we can entice @g-oikonomou or some of the other @contiki-ng/maintainers Full disclaimer: My main motivation is checking out libcoap, which this PR blocks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see there have been many changes here since I first looked at it. The PR is much more self-contained now - I previously had concerns about some changes that felt off-topic but those appear to have been removed.
I am happy for this to go ahead, no objections.
@kkrentz We want to include this in the upcoming release, are you able to rebase this PR (before Friday)? |
This PR adds a CSPRNG, which, e.g., will come in handy when generating cryptographic keys or hash chains. For generating cryptographic random numbers, it uses AES-128 in OFB mode. For generating an initial seed, it uses an exchangeable
csprng_seeder
. This PR also includes implementations ofcsprng_seeder
s for CC2538-based platforms, such as thecc2538_sram_seeder
, which extracts seeds from power-up SRAM states. For the time being, this CSPRNG does not provide forward and backward security, i.e., once its internal state leaks to an attacker, he can predict past and future outputs of the CSPRNG. This could be changed by calling the configuredcsprng_seeder
not only at startup, but also at runtime once in a while.