winnermicro / micropython

MicroPython - a lean and efficient Python implementation for microcontrollers and constrained systems
https://micropython.org
MIT License
9 stars 2 forks source link

uos.urandom: not very random [fix suggested] #2

Closed robert-hh closed 2 years ago

robert-hh commented 4 years ago

uos.random() will return the same values for calls within a short time frame (1 ms?). The reason is, that is is initialized on every call with the time. That looks not good. The RNG should be initialized only once at boot time, and, if possible, with something less predictable. But that is a minor issue. I just mention it because I noticed it by chance.

robert-hh commented 4 years ago

I had a glance at the random data, and even if I get large chunks, the look bad, even without statistical tests. I have attached a few data plots of RNG data from the W600 compared to those from my linux machine. rng_samples.zip

robert-hh commented 4 years ago

I had a look at the RNG in the WM_SDK. The access there seems to be directly to the HW registers. Do you know, how long the RNG circuitry needs to create a new set of 16 or 32 random bits? There should be a minimum time to wait between the accesses?

robert-hh commented 4 years ago

OK. So i made another test by changing the code into:

Then I collected two sets:

a) getting 20000 bytes in 10000 calls to uos.urandom() with two bytes each. Doing that, there was some delay between two accesses to the hardware RNG b) getting 20000 bytes in a single call to uos.urandom(). Doing that, the hardware RNG was read out very fast. That was already included in the previous test and just repeated here.

The result is in the attached file: w600rng.zip Getting 2 bytes each with a time between the calls results in better looking data. Obviously, the hardware RNG needs some grace period between two accesses. For a real quality check, much more data has to be collected. But at least this is an improvement.

robert-hh commented 4 years ago

Adding a short dealy in the SDK function improved the picture. The code now looks as:

int tls_crypto_random_bytes(unsigned char *out, u32 len)
{
    unsigned int val;
    uint32 inLen = len;
    int randomBytes = 2;
    val = tls_reg_read32(HR_CRYPTO_SEC_CFG);
    randomBytes = val & (1 << RNG_SWITCH) ? 4 : 2;
    while(inLen > 0)
    {
        val = tls_reg_read32(HR_CRYPTO_RNG_RESULT);
        if(inLen >= randomBytes)
        {
            memcpy(out, (char*)&val, randomBytes);
            out += randomBytes;
            inLen -= randomBytes;
            tls_os_time_delay(0);
        }
        else
        {
            memcpy(out, (char*)&val, inLen);
            inLen = 0;
        }
    }
    return ERR_CRY_OK;
}

The call to tls_os_time_delay(0) just consumes a few clock cycles which are not taken away by the code optimizer. Plot below. rng_with_wait.zip

wdyichen commented 4 years ago

Thanks for your enlightenment, I improved the random number seed that can be truly random, please refer to my attachment code.

moduos.zip

robert-hh commented 4 years ago

Getting noise from the adc is surely a much better seed than taking the time. The delay call you inserted is however at the wrong place. In has to be inserted into the SKD function, as shown above. The problem there is, that if code accesses the HR_CRYPTO_RNG_RESULT register too fast, the content of that register has not changed much, and therefore the resulting data is not random. That happens if more than 2 bytes are requested from the function tls_crypto_random_bytes(). Obviously you can change the call in uos.urandom to request only 2 bytes on each call to tls_crypto_random_bytes(). The time needed for the call and returning is long enough for the RNG to proceed sufficiently. But that will make uos.urandom slow, if many bytes are needed.

wdyichen commented 4 years ago

This is a compromise solution. WMSDK cannot be changed for the time being, it can only be modified in mpy. Once tls_crypto_random_init is called, random numbers start to be generated, so even if there is no delay in tls_crypto_random_bytes, the random number obtained for the first time will change. Of course, I will consider modifying this into the WMSDK in the future.

robert-hh commented 4 years ago

I could change uos.urandom() to get larger amounts in repeated calls for two bytes of RNG data, like it is done by tls_crypto_random_bytes(). That would be slower, but the data would be good.

robert-hh commented 4 years ago

I had a look at the data generated for seeding, and it looks so far. The doubts I have with the actual code are as follows: After seeding the RNG starts in a repeatable fashion, which only depends from the seeding value. That means, that for the first few numbers after seeding the ADC noise is the RNG source. Therefore I would still suggest to do seeding only once, and not to stop the RNG, once started. I assume the drawback is an increased sleep current. the function os_urandom() could then look like:

STATIC mp_obj_t os_urandom(mp_obj_t num) {
    mp_int_t n = mp_obj_get_int(num);
    vstr_t vstr;
    static bool seeded = false; // Seed the RNG on the first call to uos.urandom
    if (seeded == false) {
        /* adc floating in the air can get the seed of true random number */
        tls_crypto_random_init(w600_adc_get_allch_4bit_rst(), CRYPTO_RNG_SWITCH_32); 
        seeded = true;
    }
    vstr_init_len(&vstr, n);
    tls_crypto_random_bytes((unsigned char *)vstr.buf, n);
    return mp_obj_new_str_from_vstr(&mp_type_bytes, &vstr);
}
STATIC MP_DEFINE_CONST_FUN_OBJ_1(os_urandom_obj, os_urandom);
robert-hh commented 4 years ago

I did some test of the RNG data with the NIST800-22 test suite, both with data collected in blocks of 1kbyte and collected as single bytes. The new version behaves well for 1k blocks. All other tests fail. When data was collected byte-by-byte, the results was extremely bad.