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