https://github.com/trustcrypto/OnlyKey-Firmware/blob/master/...
The funny thing is they have a "Source code reviewed by Codacy" badge on the readme claiming the code is grade A... but if you actually click through, of course Codacy didn't pick up the .ino file at all, so in fact nothing of substance is being reviewed. That .ino file wouldn't pass any style review... it's a mess.
Anyway, looks like that firmware is incomplete (e.g. "onlykey.h" is missing). Just a quick scroll through the code gives me zero confidence in this thing, code quality wise. Someone who can't consistently indent code almost certainly isn't qualified to be writing security-critical software.
Edit: looks like the rest of the code is here, and yeah, it doesn't inspire much confidence (7000+ lines of code in okcore.cpp, ouch): https://github.com/trustcrypto/libraries/tree/master/onlykey
Meanwhile Nitrokey has actually been audited by Cure53.
As others have mentioned they were actually hacked https://old.reddit.com/r/crypto/comments/bis3pf/extract_pgp_...
Nitrokey does not support half of the features OnlyKey does and even the users on their own forum prefer OnlyKey -https://support.nitrokey.com/t/nitrokey-vs-onlykey/638
All libraries are included and also receive a grade of A.
#1 > The "security" of this device is a joke, just look at how randomness is derived:
Unfortunately, this commenter posted this without reviewing any of the security documentation available for OnlyKey. Had they reviewed they would see that we specifically address how analog input alone is not sufficient entropy for a cryptographically secure number generator and one of the unique features used with OnlyKey is using capacitive touch input for our RNG. This random input is generated every time you touch a button on OnlyKey, it's different for every person, and its truly random. https://docs.crp.to/security.html#cryptographically-secure-r...
#2 > Meaning that there is no hardware security whatsoever and it's trivial to extract all your keys from the device if you ever lose it. Whoops.
Again, had the commenter taken the time to read a bit they would see that this is completely false. As others have already mentioned, OnlyKey is not an Arduino, OnlyKey uses some of the great Arduino software libraries that are available open source and the Arduino IDE. This is completely unrelated to hardware. As for the OnlyKey hardware security we use Freescale Kinetis flash security to securely lock data on the key. As for side channel attack countermeasures we list several that are in use. For full details read this - https://docs.crp.to/security.html#hardware-security
When it comes to security questions, trust an expert, not the top post on a thread. For more information about CryptoTrust, the makers of OnlyKey you can find our team with internationally recognized security credentials here - https://crp.to/t/
For more info on OnlyKey:
Get started - https://onlykey.io/start
General documentation - https://docs.crp.to/
FAQs - https://docs.crp.to/faq.html
Compare to Yubikey - https://crp.to/p/
Setup and User's Guide - https://docs.crp.to/usersguide.html
Features - https://docs.crp.to/features.html
Support - https://forum.onlykey.io/
List of supported services - https://onlykey.io/pages/works-with-onlykey
Some examples...
Compare these two blocks of assignments and memcpy calls:
https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...
https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...
Yes, they are as identical as they appear. The only differences (other than a couple of lines commented out in one) are the use of 'data' in the first and 'large_resp_buffer+offset' in the second, along with some arbitrary whitespace differences. (The first uses spaces around the + operators, the second does not.) And all the hard coded numbers! What do they mean?
Or this block of code that appears to be a limited version of a decimal number formatter:
https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...
Or this code that keeps checking the same flags over and over again instead of combining the tests:
https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...
(Scroll horizontally to see all the repeated tests!)
Or this code with the same logic repeated 24 times:
https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04...
The next function after that one also has 24 copies of duplicate logic.
Well, the logic isn't entirely duplicated. The individual cases call functions like onlykey_eeget_urllen1, onlykey_eeget_urllen2, ... onlykey_eeget_urllen24, and onlykey_eeset_urllen1, onlykey_eeset_urllen2, ... onlykey_eeset_urllen24. Here are those functions:
https://github.com/trustcrypto/libraries/blob/527113dfeeb20e...
Yes, they are all identical except for the different constants each one uses:
https://github.com/trustcrypto/libraries/blob/527113dfeeb20e...
This pattern of "24 copies of the same logic with different constants" occurs all through the code. Look through okeeprom.h/cpp for several other examples.
None of this inspires confidence that the code can be trusted.
It's a shame this code isn't so good out of the box, but for all we know there are proprietary devices purporting to do the same job which also have poor code. The difference between the devices is we can review, edit/improve, share, and run the improved code for this device. The software freedom is a feature unto itself. So one is still better off with this device (or another device that runs on entirely FLOSS) over any proprietary device that purports to do the same job.
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.
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.
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.
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...
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.
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.
At least we now have a better sense of what an A grade from Codacy actually counts for.
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.
Not to mention the fact that the only obvious effect of that delay is to expose entropy information to timing analysis.
If you read further into the source you will see that analog read is only one of the sources of entropy, it uses capacitive touch from a user's skin and this TRNG passed dieharder tests - https://webhome.phy.duke.edu/~rgb/General/dieharder.php
Technically, doesn't not passing dieharder mean something with respect to cryptographic security, though?
https://en.wikipedia.org/wiki/Randomness_tests
Also, which level of security of a PRG is sufficient for keys like this?
None. The difference between a good CSPRNG and a broken one might not even be in the construction at all, but in who knows the seed. For example, a keystream generated using Chacha20 or AES-CTR makes for a good CSPRNG... except if the attacker knows the key.
Those tests can be used on raw sources to learn about the quality of those inputs. In this case applying those tests directly to the analogRead() on a specific source of hardware (your entire circuit and manufacturing process will effect this, and will even vary from board to board) can give you an estimate as to how much entropy you can expect from each call.
Understanding your where that entropy is coming from is significantly more important, gate voltage breakdown, fluctuations from the pins acting as antennas, in the current temperature and humidity is where analogRead() on a floating pin largely comes from. Other sources can be radioactive decay of particles, timing of events that are outside of the system (such as the time between a device being plugged in and the first time a person touches a key).
These all provide small amounts of entropy (except for radioactive decay, that's a really good one). The next step is mixing entropy. There is a lot of good math showing that with proper mixing, even adding known inputs from an attacker into an entropy pool doesn't decrease the entropy in the pool (it's no less random). If time isn't an issue you can add in a large number of readings from the same source, though sampling faster than the source changes won't get you anything.
That mixing allows you get to up to a minimum threshold of randomness (the seed) where you can use a cryptographically secure pseudorandom number generator (CSRNG). These also have proofs of a different type showing that input bits have an equal chance of modifying any bit of the output which can then be mixed back into the seed getting a very very large amount of effectively good randomness that can be used for keys and the like.
The trick here is that you're effectively at war with attackers, the more of your entropy sources an attacker can predict or control, the weaker your overall input to the CSRNG is going to be. If they can get this down to a small possibility space they can predict the input to the CSRNG and in turn fully predict its output which will reveal your keys.
If an attacker has a way to measure timings on the device a large number of times they may be able to infer the internal state of the system and once again get your keys.
So it's not really about the quality of that final output that is the problem and that's largely what people doing these projects analyze with these tests.
One final bit I'd like to cover. These tests can provide you some information about the final quality of the output (mostly whether it's broken or not) but even for that they're usually used incorrectly. If the CSRNG is implemented correctly but say you always seed it with the value "0", it will pass the tests with flying colors.
For devices like these they should be fully reset, have a small amount of randomness output, fully reset, sampled again... thousands to millions of times. This will help you determine if the range of possible inputs to the system is inherently flawed and most projects I've seen (including this one) don't seem to do that.
This passes dieharder. Completely meaningless.
It seems pretty bad that merely grounding 8 pins on this device will reduces its entropy to basically to a handful of noise bits from the ADC?
https://docs.crp.to/security.html#cryptographically-secure-r...
There's a reason we have real secure elements with anti-tamper mechanisms. The problem is that as far as I know there aren't any that you can develop for without signing an NDA.
For example ATECC508A, a common secure element chip used in a lot of designs. It does ECDSA signing, using DUAL_EC_DRBG (based on the description, it's not mentioned) and produces non-deterministic ECDSA signatures. You can establish this by asking it to sign the same message twice, and the nonce selection is random rather than static for the two requests. This is a very strong indicator that the chip is significantly weak as it's not using the standard RFC6979 which was specified in 2013.
Commonly a lot of "secure" software implementations use the output of the STM32's "TRNG" as a source of entropy, such as many Bitcoin hardware wallets. I don't believe that this is a strong design, based on the documentation that has been made public. It is supposedly based on the output of multiple synchronized ring oscillators which are XOR'd to produce a output into a 32 bit buffer. The documentation goes to a huge length to try and justify it as a secure source of entropy, but the speed of it (the RNG RDY flag) is much too fast for it to possibly be true.
uint32_t random32(void) {
static uint32_t last = 0, new = 0;
while (new == last) {
if ((RNG_SR & (RNG_SR_SECS | RNG_SR_CECS | RNG_SR_DRDY)) == RNG_SR_DRDY) {
new = RNG_DR;
}
}
last = new;
return new;
}
The general comment is that people seem to be far too trusting in these devices actually implementing what they say they are, or using output from hardware RNGs in a way that directly exposes the application if they were to fail or be producing predictable output.
https://www.ria.ee/en/news/possible-security-vulnerability-d...
EDIT: Spoke too soon, claims Kinetis Flash Security is enabled (https://docs.crp.to/security.html#flashsecurity). This looks like it also disables JTAG access, so that is a plus ("8.3.2 Security Interactions with Debug", https://www.pjrc.com/teensy/K20P64M72SF1RM.pdf).
Other than that, this C code has a lot of smell - for example, the repeated use of the ptr variable looks like what something someone unfamiliar with the C type system would use: https://github.com/trustcrypto/OnlyKey-Firmware/blob/c71d207...
> Meaning that there is no hardware security whatsoever and it's trivial to extract all your keys from the device if you ever lose it. Whoops.
Most micros, ones used in various Arduinos included have fuse bits so there is at least the minimal level of protection. Question whether they used it even...
https://github.com/trustcrypto/libraries/blob/master/randomb...
For anyone wanting to try this out (it will compile with plain GCC if you add):
#include <stdlib.h>
#include <stdio.h>
void main() {
unsigned char* buffer;
buffer = malloc(32 * sizeof(char));
randombytes(buffer, 32);
for (int i=0; i < 32; i++) {
printf("%02x", buffer[i]);
}
printf("\n");
free(buffer);
}
RE the RNG implementation, looks to be at https://github.com/trustcrypto/libraries/blob/master/Crypto/..., and looks to have some support for hardware RNGs on certain boards, but not others. Does sound like there's no hardware protection involved.
But I wonder if you could get better randomnes by, instead of naively pulling one low entropy 10-bit value from analogRead, you pulled 128 bits from successive analogReads and only kept the lowest significant bit.
Network packet timings aren’t random either and might be attacker controlled as well.
And yeah, analogRead() is a function from the Arduino library because .. well, apparently there's an Arduino compatible chip inside that does all the cryptographic operations. Meaning that there is no hardware security whatsoever and it's trivial to extract all your keys from the device if you ever lose it. Whoops.
[0] https://arxiv.org/pdf/1212.3777.pdf