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

Extend (& deprecate) lib_random #4

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Extend (& deprecate) lib_random #4

wants to merge 11 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 23, 2017

Work to date - please review it is what we want.
(history should be squashed)

henkmuller and others added 7 commits October 20, 2017 12:07
…simple implementation that will produce random if the attacker does not have physical access to the chip
…uld probably be removed and replaced with ordinary EXPLORER-200.
deprecated random_create_generator_from_hw_seed() making it's build conditional (add 'RANDOM_ENABLE_HW_SEED=1' to the Makefile)
Make random_prng_server(...,client interface random_pool ?rpi,...) optinal
@samchesney samchesney self-assigned this Nov 24, 2017
Copy link

@samchesney samchesney left a comment

Choose a reason for hiding this comment

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

Hi @robert-lytton, apologies for the delay in giving feedback on this pull request. I like the way the implementation is layered from bit to pool to prng, and have added a few questions and comments against particular parts of the source.

We have a few concrete use cases for this library as both a RNG and an entropy source now, and it looks like a common feature of them both is to have access to C functions with a prototype similar to the standard C int rand(void) function. I think that we should be able to provide that via a simple wrapper to the pool and prng APIs of this library, do you see any problem with that?
I will need to confirm that we will not need a CSPRNG for this, but I think not. Can the the values from the bit and pool APIs be considered cryptographically secure?
Did you have an implementation in mind for a CSPRNG? Would it have a very similar API to the PRNG added here?

Are the functions in random.h provided an a very low compute/fast PRNG alternative to random_prng.h?

Cheers,
Sam

#include <stdint.h>
#include <stddef.h>

#ifndef REFERENCE_PARAM

Choose a reason for hiding this comment

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

Is there a reason for defining REFERENCE_PARAM here (and in random.h) rather than using the macro from xccompat.h?

Copy link
Author

Choose a reason for hiding this comment

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

no,
xccompat.h should be included

Copy link
Author

Choose a reason for hiding this comment

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

random.h has now been removed - use clib alternatives.

timer tmr;
tmr :> last_time;
STORE32(last_time, &per_tile_last_time);
setps(0x60B, 2);

Choose a reason for hiding this comment

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

We could replace the magic numbers in the calls to getps() and setps() with the defines in xs2a_registers.h if this library is to support only XS2. Unfortunately the XS1 ring oscillator defines in xs1l_registers.h have a slightly different prefix, otherwise we could just include xs1_registers.h and have it work for both architectures...

Copy link
Author

Choose a reason for hiding this comment

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

yes

Copy link
Author

Choose a reason for hiding this comment

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

the decision/roadmap/work to fix target headers has been on hold since Mr Fyles blocked it.
The single character fix is somewhere in bugzilla...

Copy link
Author

Choose a reason for hiding this comment

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

done

tmr :> time;
// If the timer wraps, we will miss the opportunity - tough!
// N.B. unsigned wrapping has defined behaviour.
if (time - last_time > TIME_FOR_ONE_BIT) {

Choose a reason for hiding this comment

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

Why do we delay reading the ring oscillator after it is stopped?

Copy link
Author

Choose a reason for hiding this comment

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

Ask henk,
I assume if we don't, the probability field is skewed.

Copy link
Author

Choose a reason for hiding this comment

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

comment added

@ghost
Copy link
Author

ghost commented Aug 13, 2018

I think that we should be able to provide that via a simple wrapper to the pool and prng APIs of this library, do you see any problem with that?

No, apart from extra code.
It may be possible to reduce the footprint for distinct use cases.

@ghost
Copy link
Author

ghost commented Aug 13, 2018

I will need to confirm that we will not need a CSPRNG for this

The PRNG is revealing 32bits out of either 57, 88 or 113 bits, the other bits remain hidden (unless the user connects a debugger).

It is up to the user to decide:
how many hidden bits are required;
how much entropy the seeds must start with;
how often entropy needs to be injected into the state;

I believe this is best left as an explicit implementation detail.

@ghost
Copy link
Author

ghost commented Aug 13, 2018

Are the functions in random.h ...

This is deprecated backward comparability.
Initially it attempted as adding entropy to the initial seed.
It now offers little over the version in stdlib.h viz rand(), srand(), drand48() etc.

samchesney and others added 4 commits August 15, 2018 11:35
Ensure CI system treats it as an xCORE application, and attempts a 
build.
@ghost
Copy link
Author

ghost commented Aug 15, 2018

I think that we should be able to provide that via a simple wrapper to the pool and prng APIs of this library, do you see any problem with that?

Simple wrapper?
The problem is the creation of interfaces and placement of the interfaced tasks - along with the passing of these parameters.

It would be nice if the XC language allowed collections of select-case statements to be built up and dynamically dispatched opaquely.
However, in the current language the list is statically presented at compiler time (select statement) and dynamically dispatched via guard statement etc.

Until that day arrives, I am not sure of the best way to mix C and XC, along with adding the necessary top level knitting, especially in an easy to use library.
May be we should leave this as an implementors business until a clear idiom (or XC v3) becomes available.

@samchesney samchesney assigned samchesney and ghost and unassigned samchesney Aug 15, 2018
@samchesney
Copy link

I think that we should be able to provide that via a simple wrapper to the pool and prng APIs of this library, do you see any problem with that?

Simple wrapper?
The problem is the creation of interfaces and placement of the interfaced tasks - along with the passing of these parameters.

Starting the lib_random server as a task in xC is not an issue. However, we'll need C compatible API to access the client side, which we can use to set defines when building 3rd party libraries. These 3rd party libraries are configured as follows.

Kernel config:

#define configRAND32()    ulRand()

IP stack config:

#define ipconfigRAND32()    ulRand()

PKCS11 config:

/* Random number generation */

/* C_SeedRandom mixes additional seed material into the token's
 * random number generator. */
CK_PKCS11_FUNCTION_INFO(C_SeedRandom)
#ifdef CK_NEED_ARG_LIST
(
  CK_SESSION_HANDLE hSession,  /* the session's handle */
  CK_BYTE_PTR       pSeed,     /* the seed material */
  CK_ULONG          ulSeedLen  /* length of seed material */
);
#endif


/* C_GenerateRandom generates random data. */
CK_PKCS11_FUNCTION_INFO(C_GenerateRandom)
#ifdef CK_NEED_ARG_LIST
(
  CK_SESSION_HANDLE hSession,    /* the session's handle */
  CK_BYTE_PTR       RandomData,  /* receives the random data */
  CK_ULONG          ulRandomLen  /* # of bytes to generate */
);
#endif

TLS library config:

#define MBEDTLS_ENTROPY_HARDWARE_ALT

int mbedtls_hardware_poll(void * data, unsigned char * output, size_t len, size_t * olen) {
...
}

The following mbedTLS documentation looks useful:

@ghost
Copy link
Author

ghost commented Aug 29, 2018

Erm....
I'll read the documentation and see if I can work out how it works.

@ghost
Copy link
Author

ghost commented Aug 30, 2018

However, we'll need C compatible API to access the client side, which we can use to set defines when building 3rd party libraries. These 3rd party libraries are configured as follows.

This looks like an adapter is needed into a particular 3rd party library.
Such a specific adapter is not appropriate for a library - although having a general purpose C API that can be used by the adapter and others may be useful... but this is speculation.

I'll write suitable code and you can decide if it lives in the libraries 'examples', the libraries API or alongside the client (mbed) code.

@ghost
Copy link
Author

ghost commented Aug 30, 2018

#define configRAND32() ulRand()
#define ipconfigRAND32() ulRand()

These could use the stdlib.h rand() function.
As they will probably expose information to the outside world, they should not use the same seed as the tls - even if they share the same library code viz via a C wrapped random_prng.h.

@ghost
Copy link
Author

ghost commented Aug 30, 2018

p.s. here is the 'good enough' prng behind rand().... good enough for Donald good enough for me:

__extension__ static __thread unsigned long long _rand_next = 1;
void srand(unsigned int seed) {
  _rand_next = seed;
}
int rand(void) {
  /* This multiplier was obtained from Knuth, D.E., "The Art of
     Computer Programming," Vol 2, Seminumerical Algorithms, Third
     Edition, Addison-Wesley, 1998, p. 106 (line 26) & p. 108 */
  _rand_next =_rand_next * __extension__ 6364136223846793005LL + 1;
  return (int)((_rand_next >> 32) & RAND_MAX);
}

@ghost
Copy link
Author

ghost commented Aug 30, 2018

I've taken a 'quick' look at the cryptsoft API:
https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__11__15__RANDOM__NUMBER__GENERATION__FUNCTIONS.html

This is generating random numbers for you, I believe.
So I assume all that is needed is the seed:
pSeed, /* the seed material */
This could be got from the random_pool.h.
I have no idea how, when & how often C_SeedRandom() is called to add entropy...
p.s. It could call random_bit.h directly, rather than having random_pool.h running too)

@ghost
Copy link
Author

ghost commented Aug 30, 2018

The TLS library looks like it need hooks into the entropy.
https://tls.mbed.org/api/entropy__poll_8h.html#a45ce4792a68304d592fb711bd8f2fc86
This is probably best done by implementing specific code for the TLS that sits ontop of the randome_bit.h API (I assume it keeps it's own pool)

@ghost
Copy link
Author

ghost commented Aug 30, 2018

Please do comment if I am incorrect regarding how these components work, or if the application architecture needs additional components that require something else.

@samchesney
Copy link

#define configRAND32() ulRand()
#define ipconfigRAND32() ulRand()

These could use the stdlib.h rand() function.
As they will probably expose information to the outside world, they should not use the same seed as the tls - even if they share the same library code viz via a C wrapped random_prng.h.

Understood, thank you. I've created https://github.com/xmos/amazon_freertos_experiment/issues/29 to track this.

@samchesney
Copy link

I've taken a 'quick' look at the cryptsoft API:
https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__11__15__RANDOM__NUMBER__GENERATION__FUNCTIONS.html

This is generating random numbers for you, I believe.
So I assume all that is needed is the seed:
pSeed, /* the seed material */
This could be got from the random_pool.h.
I have no idea how, when & how often C_SeedRandom() is called to add entropy...
p.s. It could call random_bit.h directly, rather than having random_pool.h running too)

If we use random_bit() for this would it mean we were always passing a seed with of 0b00000000 or 0b00000001 and setting ulSeedLen to 1?

@ghost
Copy link
Author

ghost commented Aug 30, 2018

If we use random_bit() for this would it mean we were always passing a seed with of 0b00000000 or 0b00000001 and setting ulSeedLen to 1?

I have not looked at the implementation, but yes, that looks like one way of doing it.
Another way would be to have a timer interrupt gathering bits at a rate of 1bit/20000ticks (see #define TIME_FOR_ONE_BIT 20000) and then when it had 32bits it could send them all.
I have not looked at the expectations of the implementation so would not know what is better/more efficient.

It should be noted that random_pool.h could be used to gather the bits, but it seems too heavy weight an API (and it is written in XC). Thus implementing a really simple pooling function seems sensible in this case.

@ghost
Copy link
Author

ghost commented Aug 30, 2018

N.B.
If the 'cryptsoft' and 'mbed tls' libraries are running on differnt cores, they can each have their own random_bit.h source of entropy.

If they are running on the same core, they will need to share the bits co-operatively to prevent starvation or a race condition - the library is not logical-core safe by design! The random_bit_claim() is there to make this sharing safe.

@samchesney
Copy link

I found this documentation on how to port mbed TLS. So I agree, we can start off using the random_bit API for this too (and hopefully find that it performs well enough polling for a byte of entropy at a time in mbedtls_hardware_poll().

@samchesney
Copy link

I think all of the abov means that in actual fact this pull request is almost ready to merge?

@samchesney
Copy link

I've taken a 'quick' look at the cryptsoft API:
https://www.cryptsoft.com/pkcs11doc/v220/group__SEC__11__15__RANDOM__NUMBER__GENERATION__FUNCTIONS.html

This is generating random numbers for you, I believe.

Looking into this further, I'm not sure if I'm missing something. I thought this was specifying an API, but not providing an implementation?
I think we need to provide a matching implementation for C_GenerateRandom().

interface random_prng prngi;
par {
[[distribute]]
random_prng_server(prngi, null, prng57, null);

Choose a reason for hiding this comment

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

@robert-lytton should the third argument passed be prng rather than prng57, or have I misunderstood?


/** Method that returns a pseudo-random number.
*
* ====== WARNING ====================================================

Choose a reason for hiding this comment

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

@robert-lytton does this warning still hold true?
I'm trying to ensure I have correctly understood which applications this PRNG can be safely used for, but I'm struggling to reconcile the warning in the source with the comments in this review:

I will need to confirm that we will not need a CSPRNG for this

The PRNG is revealing 32bits out of either 57, 88 or 113 bits, the other bits remain hidden (unless the user connects a debugger).

It is up to the user to decide:
how many hidden bits are required;
how much entropy the seeds must start with;
how often entropy needs to be injected into the state;

I believe this is best left as an explicit implementation detail.

@samchesney samchesney removed their assignment Oct 11, 2020
@mbanth mbanth added the type:enhancement New feature or request label Apr 20, 2021
@mbanth mbanth modified the milestone: Publish Before Resolution Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants