Hacker Newsnew | past | comments | ask | show | jobs | submit | cj371's commentslogin

If you're so confident in your experts, maybe respond to my comment where I point out a major bug? https://news.ycombinator.com/item?id=21889302


Sure thing. Thanks for reviewing the code, we are always happy to get additional eyes on it. For your major bug I have to disagree about the major part, the RNG works well but yes it could work better, I will put the long answer in your comment below. As for the short answer I created a video showing how the OnlyKey uses capacitive touch for RNG. 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. Keep in mind the RNG is slowed down for the video, actual entropy gathering is much faster in use - https://vimeo.com/381733010


The microcontroller isn't the only thing that matters in your design. For example, since you're dependant on the ADC for seeding the RNG, it'd be nice to know what is connected to those pins, which a schematic would reveal. I can't tell that just by looking through your clear epoxy.

Even if I did drill holes in the casing and probe components, I have no way of knowing if what I'm seeing is expected or not without a schematic.


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.


Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: