Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Revert "Revert "Add Ledger Nano S to hardware wallets"" #1363

Merged
merged 1 commit into from Nov 3, 2016

Conversation

Projects
None yet
3 participants
Contributor

EricLarch commented Sep 11, 2016 edited

Our PR #1337 was mistakenly merged and subsequently reverted, which had for effect to close it without a merge. I'm therefore opening this "revert revert" PR so the discussion can be finalized about integration of the Ledger Nano S hardware wallet on the "Choose your Bitcoin wallet" page.

Contributor

crwatkins commented Sep 11, 2016

@EricLarch Thank you very much! Sorry about the confusion.

Contributor

crwatkins commented Oct 19, 2016

I have reviewed the Ledger Nano S based on the current wallet requirements criteria and my evaluation is below. The summary is that the wallet passes on security and overall design. However, because the device was released less than three months ago, I cannot at this time recommend it for listing, but I will recommend it for listing on 29 October 2016.

Note that as a "hardware wallet," only the hardware and firmware components of the device were evaluated. Wallet software that runs externally to the Nano S device was not evaluated in this review. As an aside, I would like to change this in the future and evaluate combinations of hardware and software as a wallet system, but that's for another day. The Ledger Wallet Chrome app, Copay (only in P2SH mode; P2PKH mod is not supported), Electrum, Mycelium, and Greenbits were used during this review, but not evaluated.

I concur with the scoring in the pull request.

Ledger Nano S

Firmware v1.2

Review Version 2016101901

The wallet list is based on the personal evaluation of the maintainer(s) and regular contributors of this site, according to the criteria detailed below.

These requirements are meant to be updated and strengthened over time. Innovative wallets are exciting and encouraged, so if your wallet has a good reason for not following some of the rules below, please submit it anyway and we'll consider updating the rules.

NOTE The hardware device used for testing was supplied by Ledger to bitcoin.org at no cost.

NOTE While not a required criteria, the Nano S does not display the receive address for requested payments. This is a significant omission in an otherwise impressive feature set. Ledger claims that this will be included in a future version.

NOTE Only the hardware/firmware is being evaluated here. The wallet software running external to the device is ignored as out of scope.

Basic requirements:

  • Sufficient users and/or developers feedback can be found without concerning issues, or independent security audit(s) is available

PASS No concerning issues

  1. Amazon.com product reviews
  2. https://bitcoinmagazine.com/articles/bitcoin-hardware-wallet-review-ledger-may-have-caught-up-to-trezor-with-nano-s-1473944111
  3. https://bitcointalk.org/index.php?topic=899253.640
  4. https://bitcointalk.org/index.php?topic=899253.660
  5. https://www.reddit.com/r/ledgerwallet/
  • No indication that users have been harmed considerably by any issue in relation to the wallet

PASS No evidence found based on web searches including reddit and bitcointalk

  • No indication that security issues have been concealed, ignored, or not addressed correctly in order to prevent new or similar issues from happening in the future

PASS No indication. Ledger is active and responsive on public forums. Bugs found during testing were quickly fixed.

  • No indication that the wallet uses unstable or unsecure libraries

PASS No indication at https://github.com/LedgerHQ

  • No indication that changes to the code are not properly tested

PASS No indication

  • Wallet was publicly announced and released since at least 3 months

NOTE Released about 29 July 2016; eligible for listing on 29 October 2016

PASS No concerning bugs found in the current version

NOTE During testing a bug was found in which the wallet displayed the change transaction (address and amount) instead of the payment transaction. This was quickly fixed by Ledger in the current version.

  • Website supports HTTPS and 301 redirects HTTP requests

PASS www.ledgerwallet.com redirects to HTTPS

PASS https://www.ledgerwallet.com A+ rating

  • Website serving executable code or requiring authentication uses HSTS with a max-age of at least 180 days

PASS https://www.ledgerwallet.com uses HSTS with a max-age of 365 days

  • The identity of CEOs and/or developers is public

PASS http://www.ledger.co/#team

  • Avoid address reuse by displaying a new receiving address for each transaction in the wallet UI

N/A Addresses, or paths, are chosen by wallet software

  • Avoid address reuse by using a new change address for each transaction

N/A Addresses, or paths, are chosen by wallet software

  • If private keys or encryption keys are stored online:
    • Refuses weak passwords (short passwords and/or common passwords) used to secure access to any funds, or provides an aggressive account lock-out feature in response to failed login attempts along with a strict account recovery process.

PASS Keys are stored in the device. Three failed PIN entires results in keys being erased.

  • If user has no access over its private keys:

N/A

  • Provides 2FA authentication feature
  • Reminds the user to enable 2FA by email or in the main UI of the wallet
  • User session is not persistent, or requires authentication for spending
  • Provides account recovery feature
  • If user has exclusive access over its private keys:
    • Allows backup of the wallet

PASS Allows manual copying of 24 word BIP39 phrase at startup

NOTE Nano S requires confirmation of a portion of the backup phrase

  • Restoring wallet from backup is working

PASS Device was able to be restored using BIP39 phrase and BIP39 phrase was able to be used on other compatible wallets using a standard BIP44 path

PASS https://github.com/LedgerHQ/

  • If user has no access to some of the private keys in a multi-signature wallet:

N/A

  • Provides 2FA authentication feature
  • Reminds the user to enable 2FA by email or in the main UI of the wallet
  • User session is not persistent, or requires authentication for spending
  • Gives control to the user over moving their funds out of the multi-signature wallet
  • For hardware wallets:
    • Uses the push model (computer malware cannot sign a transaction without user input)

PASS Physical button presses on the device are required for confirmation

  • Protects the seed against unsigned firmware upgrades

PASS The seed is erased during all firmware upgrades

  • Supports importing custom seeds

PASS A seed generated by another wallet was imported successfully

  • Provides source code and/or detailed specification for blackbox testing if using a closed-source Secure Element

PASS Source code is provided for the Bitcoin application which runs on top of the firmware https://github.com/LedgerHQ/blue-app-btc

NOTE Some source is code for the secure (ST31) part is not available. Some details are at https://blog.ledger.co/secure-hardware-and-open-source-ecd26579d839

Optional criteria (some could become requirements):

  • Received independent security audit(s)

NOTE No known audits

  • Does not show "received from" Bitcoin addresses in the UI

N/A Wallet hardware does not display received transactions

NOTE Ledger Chrome app: Shows "received from" Bitcoin addresses (FAIL)

  • Uses deterministic ECDSA nonces (RFC 6979)

PASS A transaction was signed with both Nano S and pybitcointools, verifying RFC 6979 signatures with low S

  • Provides a bug reporting policy on the website

PASS http://support.ledgerwallet.com/help_center

  • If user has no access over its private keys:

N/A

  • Full reserve audit(s)
  • Insurance(s) against failures on their side
  • Reminds the user to enable 2FA in the main UI of the wallet
  • If user has exclusive access over its private keys:
    • Supports HD wallets (BIP32)

PASS Uses standard BIP32/BIP44 paths

  • Provides users with step to print or write their wallet seed on setup

PASS

  • Uses a strong KDF and key stretching for wallet storage and backups

N/A

  • On desktop platform:
    • Encrypt the wallet by default

N/A

  • For hardware wallets:
    • Prevents downgrading the firmware

PASS Firmware is not currently prevented from being downgraded, but the seed is erased during any upgrade or downgrade

Contributor

Cobra-Bitcoin commented Oct 22, 2016

Cool! That was a very thorough review.

Unless some new information comes to light, I'll be merging this on the 29th then.

Contributor

crwatkins commented Oct 31, 2016

I recommend the Ledger Nano S for listing.

@Cobra-Bitcoin Cobra-Bitcoin merged commit c042173 into bitcoin-dot-org:master Nov 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment