5 ms·

It seems this is literally written in the horrible Arduino "everything in one huge file" style: https://github.com/trustcrypto/OnlyKey-Firmware/blob/master/Onl

It seems this is literally written in the horrible Arduino "everything in one huge file" style: https://github.com/trustcrypto/OnlyKey-Firmware/blob/master/OnlyKey_Beta/OnlyKey_Beta.ino 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 https://github.com/trustcrypto/libraries/tree/master/onlykey

blattimwind 7y ago
> 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.
- 0x0359463 7y ago
  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_secret_keys_from_gnuk_nitrokey_start/ 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 https://support.nitrokey.com/t/nitrokey-vs-onlykey/638
Stratoscope 7y ago
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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L2384 https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04... https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L2488 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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L1418 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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L626 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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L3301 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/527113dfeeb20e921f6c8cbf821fe94383843b21/onlykey/okeeprom.c#L700 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/527113dfeeb20e921f6c8cbf821fe94383843b21/onlykey/okeeprom.h#L141 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.
- 7y ago
cr7pt0 7y ago
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 https://github.com/trustcrypto/OnlyKey-Firmware as well as libraries here https://github.com/trustcrypto/libraries 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.
- delfinom 7y ago
  And I am going to have to give Codacy a grade of F but if this is what they consider grade A.
  - actionowl 7y ago
    As a Codacy user for $DAYJOB I can guarantee you that Codacy deserves worst than grade F.
- marcan_42 7y ago
  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.
  - cr7pt0 7y ago
    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/dashboard https://app.codacy.com/manual/onlykey/OnlyKey-Firmware/dashb... All libraries are included and also receive a grade of A.
    - 7y ago
userbinator 7y ago
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.
- cr7pt0 7y ago
  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 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.

blattimwind 7y ago

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

0x0359463 7y ago
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_secret_keys_from_gnuk_nitrokey_start/ 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 https://support.nitrokey.com/t/nitrokey-vs-onlykey/638

Stratoscope 7y ago

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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L2384 https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb04... https://github.com/trustcrypto/libraries/blob/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L2488 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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L1418 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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L626 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/5bd1f8eb15eb0463487089f9df531f3384286886/onlykey/okcore.cpp#L3301 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/527113dfeeb20e921f6c8cbf821fe94383843b21/onlykey/okeeprom.c#L700 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/527113dfeeb20e921f6c8cbf821fe94383843b21/onlykey/okeeprom.h#L141 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.

7y ago

cr7pt0 7y ago

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 https://github.com/trustcrypto/OnlyKey-Firmware as well as libraries here https://github.com/trustcrypto/libraries 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.

delfinom 7y ago
And I am going to have to give Codacy a grade of F but if this is what they consider grade A.
- actionowl 7y ago
  As a Codacy user for $DAYJOB I can guarantee you that Codacy deserves worst than grade F.
marcan_42 7y ago
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.
- cr7pt0 7y ago
  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/dashboard https://app.codacy.com/manual/onlykey/OnlyKey-Firmware/dashb... All libraries are included and also receive a grade of A.
  - 7y ago

userbinator 7y ago

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.

cr7pt0 7y ago
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 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.