In April, I took a closer look at another cryptocurrency hardware wallet that is based on a fork of the Trezor One code. During this analysis,
I found a number of memory safety issues and other code problems, which are described in the following article.
To my knowledge, none of the issues represents a practical threat to end users of the wallet, but many of the issues should have been discovered during development and testing.
I’m a freelance Security Consultant and currently available for new projects.
If you are looking for assistance to secure your projects or organization, contact me.
General Analysis Approach
The fuzzing setup was comparable to the environment that I used in the past to analyze the Trezor One, KeepKey and other hardware wallets (which are based on similar code originating from the Trezor project). To summarize, in each case a modified version of the wallet firmware code is runnable under x86_64 with clang, sanitizers and libFuzzer. Please see some of the previous articles on hardware wallet vulnerabilities if you are interested in the details.
To my knowledge, the Skycoin firmware variant was forked off the Trezor One repository sometime in 2018. Given that I had already performed a lot of in-depth fuzzing research and bughunting on the upstream Trezor One code by mid-2018, looking at the Skycoin firmware now in 2020 after two years of development is - in a sense - also a study about the type of memory-related issues that are (re-)introduced into a codebase during restructuring and rewrites by a new development team if no semi-continuous fuzzing is done.
Fuzzing a codebase with sanitizers usually results in a pile of bugs of various types and severity levels, and this project was no exception. As described in the Coordinated Disclosure section, a larger number of bugs often comes with significant overhead in terms of testing, reporting and patching, which can be a challenge when trying to report all of them instead of ignoring the lower-severity ones.
Here is an overview of the issues that I reported:
out of bounds write
dangerous “tiny message” definition leading to message handling issues
may be difficult to trigger for an attacker in a real-world scenario (since they are e.g., only reachable on an uninitialized device or similar edge case)
may not be very controllable (resulting in e.g., “only” a crash)
may require user confirmation that partially prevents harmful usage and frequent calls (e.g., information disclosure of small memory areas or computation based on uninitialized memory)
may be broken, but behave reasonably for most use cases (undefined behavior)
Details for Vulnerability No. 1
This issue is interesting since it originates from the special “tiny message” protobuf USB message handling in the original code that has spawned multiple out of bounds write issues over the years.
As outlined in the technical introduction for CVE-2019-18671, which represents one of the issue variants, the USB message handling of the wallet is split into the complex protobuf messages (that usually consist of multiple USB messages) and a special subclass of “tiny” protobuf messages which fit into a single 64 byte USB message. This classification is meaningful due to the restricted threading and memory capabilities of the target microcontroller platform since it allows imposing limits for nested or parallel processing of messages.
The underlying memory safety problem is that protobuf messages can be larger in their decoded form than in their encoded form (which is passed between devices), sometimes significantly larger. As a result, the decoded form of a protobuf message which is transported within a 64 byte message frame can exceed the uint8_t msg_tiny buffer that is reserved for the decoded result if the developers are not careful with the individual protobuf message definitions.
I discovered the original case of this problem in the Trezor One code in early 2018. By chance, the discovery happened at a good time in the development cycle since the bug was found before getting shipped to customers, so it was not widely reported as a practical security issue. Countermeasures were put in place upstream to remove the issue and also to prevent the reintroduction of this problem via _Static_assert checks that are performed at each compilation. The second part was important, since seemingly harmless development changes to the tiny message protobuf definition files could silently increase the decoded message footprint beyond the size of the target buffer for tiny messages, resulting in memory safety problems.
It appears that these upstream fixes were not picked up by the Skycoin developers on their own during the last two years (see messages.c) and that they made a similar development mistake through adding a large data field to the initialize message.
As a result, the decoded protobuf size of the Skycoin init message is 66, which conflicts with the 64 byte size of the target buffer. The Nanopb library used for protobuf decoding will not abort the processing and instead write behind the target buffer.
The resulting ASAN warning looks like this:
The danger of this type of memory corruption vulnerability comes from the fact that the fault happens very early in the USB message processing. In particular, the out of bounds write takes place before any more specific checks for user confirmation, PIN code, message processing state machine or other mitigating factors come into play. This makes any negative effects very easy to reach for both host-based malware as well as for attackers in a wallet theft scenario, which is bad for this kind of security device.
Fortunately for Skycoin, it appears that the overwritten memory does not hold anything important. Since I don’t have access to a physical Skycoin hardware wallet, I did not look closely at this myself, but it is plausible that the memory corruption does not have practical security impacts on any of their released firmware. I still consider it a noteworthy security issue, but did not request a CVE for this since no practical security impact was observed.
Details for Vulnerabilities No. 2 to No. 13
Not all of the issues are worth a lengthy description, please consider the referenced patches. It is possible that I will add some descriptions to this article at a later time.
Hardware wallets are expected to be very trustworthy devices, and it is not the best sign if the kind of issues listed above remain in the codebase for a longer period of time. I have no objective numbers in terms in terms of “bug density”, but I think it is fair to say that the code quality of this project can still be improved and that the code could be written more defensively.
Only few cryptocurrency projects produce and maintain their own hardware wallet, which obviously has something to do with the necessary effort and development costs. To keep the project manageable and secure over longer periods of time, my recommendation would be to remove any code functionality that is not absolutely essential and concentrate on the quality of the core code.
To go outside of the scope of the Skycoin hardware wallet:
it is no secret in the industry that the C language is difficult to use safely, particularly in systems where a lot of handling of external inputs is involved.
Many failure cases directly compromise security, and it is easy to introduce bugs or miss them during analysis.
My personal recommendation for any new security-related projects in 2020 is to implement as much functionality in “memory-safe” languages as possible, for example via MicroPython on microcontroller projects.
The disclosure process has not been as smooth and straightforward as I had hoped. Initially, the security contact address given in the project README.md was not functional and I had to make a public request before I could report the issues to them confidentially. The resulting delay caused uncertainties about the bug bounty status of findings #2 and #9 (an open GitHub PR indirectly changed some of the relevant code) which was a point for discussion.
I felt that there were more discussions than usual about whether the mentioned code positions were actually reachable and why they should be fixed.
To a limited degree, this is part of the normal disclosure process, but it can take a lot of extra effort to dig deeper into the code behavior than the developers did, particularly when reporting a dozen issues.
The timeline documents some of the relevant delays and e.g., patch-related extra steps.
Thankfully, Skycoin revised the early low bug bounty reward during the disclosure, which I appreciate.
It took around ~75 days from my first disclosure mail to the main set of patches to be public. I am not sure if a firmware update has been released yet (status 2020-08-08).