OnlyKey has been in use for about 4 years. It has thousands of active users and is in use in over 40 countries world wide. This is not a new product, and it has a great user community which is not afraid to test, hack, and prove the security of devices.

>OnlyKey has been in use for about 4 years. It has thousands of active users and is in use in over 40 countries world wide. This is not a new product, and it has a great user community which is not afraid to test, hack, and prove the security of devices.

Like literally the first issue was already linked above. Using the psuedo RNG with some analog pin seed isn't really acceptable. It should have a true rng IC that can generate real random numbers from diode bandgap noise or other sources.

Like literally the first post issue is completely incorrect, thats one of the issues in reading a post like this in an online thread, literally that user copied part of but not all of the function that is used for RNG. The part they copied uses analog input as one of the sources of entropy, they failed to also include the 6 capacitive touch inputs that are also inputs to the RNG. Those touch inputs literally change every time you press a button and even with atmospheric changes, i.e. it's cloudy out today, your RNG has changed.

RNG.stir((uint8_t )analog1, sizeof(analog1), sizeof(analog1) 4);

touchread1 = touchRead(TOUCHPIN1);

RNG.stir((uint8_t )touchread1, sizeof(touchread1), sizeof(touchread1));

delay((analog1 % 3) + ((touchread1 + touchread2 + touchread3) % 3)); //delay 0 - 6 ms integrityctr1++;

touchread2 = touchRead(TOUCHPIN2); RNG.stir((uint8_t )touchread2, sizeof(touchread2), sizeof(touchread2));

touchread3 = touchRead(TOUCHPIN3);

RNG.stir((uint8_t )touchread3, sizeof(touchread3), sizeof(touchread3));

touchread4 = touchRead(TOUCHPIN4);

RNG.stir((uint8_t )touchread4, sizeof(touchread4), sizeof(touchread4));

touchread5 = touchRead(TOUCHPIN5);

RNG.stir((uint8_t )touchread5, sizeof(touchread5), sizeof(touchread5)); touchread6 = touchRead(TOUCHPIN6);

RNG.stir((uint8_t )touchread6, sizeof(touchread6), sizeof(touchread6));

unsigned int analog2 = analogRead(ANALOGPIN2);

RNG.stir((uint8_t )analog2, sizeof(analog2), sizeof(analog2) 4);

// Perform regular housekeeping on the random number generator.

RNG.loop();

delay((analog2 % 3) + ((touchread6 + touchread5 + touchread4) % 3)); //delay 0 - 6 ms

integrityctr2++;

if (integrityctr1 != integrityctr2) { //Integrity Check unlocked = false; CPU_RESTART(); return; }

https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...

Even if that analog pin provides a reasonable amount of entropy (which I'm skeptical of), you have a major bug: you're casting the ADC reading to a pointer, and then dereferencing it inside RNG.stir.

Let me say it again: you're taking an ADC reading (in the range of 0-1023) and accessing it as if it's a memory address.

To make things worse, addresses 0 through 1023 on the Kinetis you're using are the vector table. Take a look at that part of your firmware: it's extremely predictable, and only contains a small number of possible values.

Here is the long answer to the comment provided above, as mentioned there its probably easier to take a look at the video first, the blue arrow in the video points to the values that change as the buttons are pressed, you will see the four values per button providing random entropy, this is what goes into RNG.stir - https://vimeo.com/381733010

You will notice that as you mentioned the analog read values don't change much, that is because it is reading the memory address. Keep in mind that the analog read is only an additional source of entropy, not the primary source, that comes from the capacitive touch buttons. The RNG does not need or require this entropy, but you can never really have too much entropy so that's why it was included. So with reading the analog address values what you get is only a small amount of entropy, these address values do change based on user behavior so its still an unpredictable source of entropy, you wouldn't know on any given day how a user will use their key. I.e. I log in to two sites in a different order on two days, it's going to mix in some non-predictable data.

But you are absolutely right, it would be better to mix in the analog read value. For our next firmware release we will update this to include mixing in both the value and the memory address. Thanks again for bringing this up and feel free to create an issue on Github if you see anything else.

You're right. I tried to give this code a charitable read, but it's horrifically wrong, and not just the ADC, but seemingly all inputs here.

At least we now have a better sense of what an A grade from Codacy actually counts for.

To be fair to Codacy, it's not even checking the file that people are pulling all these examples of bad code from.

Just because on your testbench they changed enough for you to guesstimate they provide enough entropy doesn't mean they provide enough entropy for everyone under all circumstances. They are not designed for that purpose and unless you have performed extensive adversarial testing to gain confidence that they can be used as such, you cannot guarantee anything.

You seem to have zero runtime sanity checks too, so if for whatever reason they are not providing entropy for someone, they will be none the wiser.

Sorry, but this is a terrible RNG.

Your "delay 0-6 ms" only delays 0-4 ms.

Not to mention the fact that the only obvious effect of that delay is to expose entropy information to timing analysis.

The delay is 0-2 + 0-2 so yes combining six possible values for a possible delay up to 4ms. The delay inside of an RNG loop obviously does not expose entropy to timing analysis, it does the opposite. As the loop has a small random delay interval the RNG seeds are never predictably read in, adding to the effectiveness of the unpredictability of the RNG which is a good thing.