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.
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 amount 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:
|1||out of bounds write||dangerous “tiny message” definition leading to message handling issues||patch|
|2||out of bounds write||insufficient input validation for an array index based access via
|3||out of bounds read + write, potential crash||unsigned integer underflow in the mnemonic handling, segfault depending on architecture||patch|
|4||undefined behavior||conversion issue for boolean inputs in nanopb dependency||not patched, upstream documentation|
|5||undefined behavior||undefined left shifts||patched case 1, unpatched case 2|
|6||undefined behavior or crash||
|7||out of bounds write||incorrect buffer size, unsafe
|8||out of bounds write||incorrect buffer size||patch|
|9||unintended unsigned integer overflow||passing check with
|13||out of bounds write||insufficient input validation||unclear|
Keep in mind that some of these 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 behaviour)
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.
Here is a part of the actual patch:
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 size byte 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 Vulnerability No. 2 - 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).
|Skycoin hardware-wallet||Github||patches, firmware revision unclear||?|
|2020-04-12||First disclosure attempt to Skycoin (email problem)|
|2020-04-12||Second disclosure attempt to Skycoin|
|2020-04-13||Attempted report of additional issues to Skycoin|
|2020-04-18||Attempted report of additional issue to Skycoin|
|2020-04-26||Public request to receive a functional disclosure contact address|
|2020-04-28||Third disclosure attempt to Skycoin (email problem)|
|2020-04-28||Fourth disclosure attempt to Skycoin|
|2020-05-01||Disclosure acknowledgment by Skycoin|
|2020-05-05||Initial Skycoin disclosure assessment and first bug bounty valuation|
|2020-05-05||Discussion about disclosure timing regarding PR#55 and missed issue #1|
|2020-05-11||Skycoin considers one issue as critical|
|2020-05-14||Conference call with Skycoin|
|2020-06-04||Skycoin plans to include patches in scheduled June release|
|2020-06-07||Notification about additional stack-buffer-overflow read to Skycoin|
|2020-06-08||Skycoin communicates the updated bug bounty valuation|
|2020-06-25||Skycoin publishes main set of patches in PR#61|
|2020-06-30||Note about insufficient patch sent to Skycoin|
|2020-07-02||Discussion of open issue with Skycoin|
|2020-07-09||Skycoin publishes additional patch for open issue|
|2020-07-25||Request for feedback about patch release|
|2020-08-07||Note to Skycoin about potentially unfixed issues #4, #5, #10|
|2020-08-08||Publication of this blog post|
A Note About the Research
I want to emphasize that this research was done on my own time and initiative. In particular, it was not incentivized by SatoshiLabs, for whom I do some paid security research on the upstream project.
Skycoin provided a bug bounty for these issues.