Hacker News Reader

> 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.

Meanwhile Nitrokey has actually been audited by Cure53.

The "security" of Nitrokey is a joke.

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

I understand the Arduino model is different than other projects but we proudly use Arduino as it's open source and has lots of great features. As we use the Arduino model you can find that our source consists of the .ino you mentioned here https://github.com/trustcrypto/OnlyKey-Firmware as well as libraries here https://github.com/trustcrypto/libraries. Our code is reviewed by Codacy and yes, it does receive a grade of A. For the .ino grading you will need to look at the OnlyKey-Firmware Github repo and for the libraries check out the libraries library. I think some of the confusion in your comment here may be related to how Arduino works, all source can be found on Github.

And I am going to have to give Codacy a grade of F but if this is what they consider grade A.

As a Codacy user for $DAYJOB I can guarantee you that Codacy deserves worst than grade F.

It seems you're confused as to what Codacy is reviewing. Look at their dashboard for the OnlyKey-Firmware repo. They are not reviewing your .ino file at all, because they do not consider that file extension as code. Only the toplevel C files are covered.

The .ino file is included in Codacy review and receives a grade of A. You can find that here - https://app.codacy.com/manual/onlykey/OnlyKey-Firmware/dashb...

All libraries are included and also receive a grade of A.

You just changed that. It was not included when I looked, and this fact is obvious by the "OnlyKey-Firmware has decreased 1% in quality in the last 7 days." banner. You have made no commits to the repo since Oct 23, so the only way the quality would decrease in the past week is if you changed the settings to include the .ino file.

I wanted to make sure I clearly address these comments, one of the issues in reading a post like this in an online thread is the most upvoted post can also be the most incorrect, and misleading.

#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

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

I can safely say that a lot of proprietary crypto code (as in, stuff that is in very widespread use and costs $$$) is not unlike this either. In some ways this is actually more straightforward to read and understand since it's in one file and not wrapped in a dozen layers of abstraction.

Definitely true, anyone who has ever seen proprietary crypto code knows this. Reviewing one file that is 7000 lines long is more straight forward than reviewing 7000 lines of code split in multiple files. It's open source and we will continue to make it better. If the biggest criticism here is the large file size, RNG complaint (top post is incorrect about analog read, they missed that we also use 6 touch buttons to seed RNG), and code style then it's a safe bet that OnlyKey source is better than most of the proprietary security keys out there. Of course it's not possible to know for sure as they are closed source, but you can look at past vulnerabilities. Like this one https://crocs.fi.muni.cz/public/papers/rsa_ccs17 it's not a theoretical RNG issue like the criticism here has been, it's an actual exploitable vulnerability that affected Yubikey and tons of smart cards. This exploit was on devices that were already FIPS and CC certified. Another thing to consider is the way the researchers found this was by statistically testing a bunch of keys, they didn't even review the source so you can imagine how many more security vulnerabilities they would find if they did.

The 7000 lines don't bother me as much as the complete lack of refactoring, heavy use of magic numbers repeated throughout, and logical expressions that duplicate logic over and over again.

Some examples...

Compare these two blocks of assignments and memcpy calls:

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.

> 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.

You have no access to hardware schematics. You have no idea what hardware defects are present that may compromise security no matter how much code you write. FLOSS means shit here.

This is incorrect, a schematic only shows what electronics should contain. It doesn't provide any proof of what hardware actually contains. For that the best way to verify is to visually look at the hardware, we made OnlyKey hardware easy to verify with a clear transparent coating. When you look at OnlyKey you will see one Freescale K20 MCU, you can read the manufacturer number on it and know exactly what is in your key.

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.