6 ms·

I have to agree with you. What impresses me even more is that they are selling it already, and marketing as “open-source”. I would leave a note here that if an

by klhugo 7y ago

I have to agree with you. What impresses me even more is that they are selling it already, and marketing as “open-source”. I would leave a note here that if anybody is interested in doing something similar, please get some feedback from community before starting commercialization.

lonelappde 7y ago
There's a dead simple "quack/crank" test for security products. If it hasn't been publicly discussed and analyzed for at least year, but is already for sale as a "usable device" not a "prototype", the seller is either fraud or a fool, and regardless of which, is not to be trusted.
- cr7pt0 7y ago
  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.
  - delfinom 7y ago
    >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.
    - cr7pt0 7y ago
      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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L2259 https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...
      - marcan_42 7y ago
        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.
      - cj371 7y ago
        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.
        raphlinus 7y ago
        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.
        fmj 7y ago
        To be fair to Codacy, it's not even checking the file that people are pulling all these examples of bad code from.
        cr7pt0 7y ago
        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 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.

by klhugo 7y ago

lonelappde 7y ago

There's a dead simple "quack/crank" test for security products. If it hasn't been publicly discussed and analyzed for at least year, but is already for sale as a "usable device" not a "prototype", the seller is either fraud or a fool, and regardless of which, is not to be trusted.

cr7pt0 7y ago
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.
- delfinom 7y ago
  >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.
  - cr7pt0 7y ago
    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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L2259 https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...
    - marcan_42 7y ago
      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.
    - cj371 7y ago
      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.
      - raphlinus 7y ago
        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.
        fmj 7y ago
        To be fair to Codacy, it's not even checking the file that people are pulling all these examples of bad code from.
      - cr7pt0 7y ago
        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 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.