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.

Consulting

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.

The Vulnerabilities

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:

id type summary reference
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 respSkycoinAddress->addresses_count likely patch
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 memcpy() on NULL pointer patch
7 out of bounds write incorrect buffer size, unsafe sprintf() patch
8 out of bounds write incorrect buffer size patch
9 unintended unsigned integer overflow passing check with 99 + 4294967295 patch
10 information disclosure use-of-uninitialized-value unclear
11 information disclosure use-of-uninitialized-value patch
12 information disclosure use-of-uninitialized-value patch
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 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[64] 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:

- CONFIDENTIAL uint8_t msg_tiny[64];
+ CONFIDENTIAL uint8_t msg_tiny[128];
+ _Static_assert(sizeof(msg_tiny) >= sizeof(Cancel), "msg_tiny too tiny");
+ _Static_assert(sizeof(msg_tiny) >= sizeof(Initialize), "msg_tiny too tiny");
+ _Static_assert(sizeof(msg_tiny) >= sizeof(PassphraseAck), "msg_tiny too tiny");
+ _Static_assert(sizeof(msg_tiny) >= sizeof(ButtonAck), "msg_tiny too tiny");
+ _Static_assert(sizeof(msg_tiny) >= sizeof(PinMatrixAck), "msg_tiny too tiny");

messages.c

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:

==30532==ERROR: AddressSanitizer: global-buffer-overflow on address [...]
WRITE of size 66 at 0x0000006a79c0 thread T0
    #0 0x4f16fe in __asan_memset
    #1 0x553c94 in pb_field_set_to_default
    #2 0x53c9f0 in pb_message_set_to_defaults
    #3 0x53c81e in pb_decode
    [...]

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.

Personal Summary

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.

Coordinated Disclosure

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

Relevant Product

variant source fix references
Skycoin hardware-wallet GitHub patches, firmware revision unclear ?

Detailed Timeline

Date info
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-12 Issue discussion
2020-05-13 Issue discussion
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.

Bug Bounty

Skycoin provided a bug bounty for these issues.