Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ledger Nano S #3303

Merged
merged 1 commit into from Mar 4, 2018
Merged

Ledger Nano S #3303

merged 1 commit into from Mar 4, 2018

Conversation

@cslashm
Copy link
Contributor

cslashm commented Feb 21, 2018

This a new push request for Hardware Wallet support in Monero client.

It replaces the push request #3095

Goals

The goal of this PR is to propose code modifications to integrate the ledger HW into
monero-wallet-cli.

Approach

The idea relies on 2 main principles:

  • master spend key and view key never leave the device
  • all other needed secret are computed by the device but stored encrypted by the Monero client

The basic approach it to delegate all sensitive data (master key, secret ephemeral key,
key derivation, ....) and related operations to the device. As the device as low
memory the device do not keep itself the values (except for view/spend keys) but once
computed there are encrypted (AES) and return back to monero-wallet-cli. When
they need to be manipulated by the device, they are decrypted on receive.

Moreover using the client for storing the value in encrypted form limits the modification
in the client code. Those values are transfered from one C-structure to another one
as previously.

The code is modified at tow main levels:

. crypto operations, mainly crypto.cpp and cryptonote_format_utils.cpp
. signature level, mainly rctSigs.cpp and cryptonote_tx_utils.cpp

The pdf (https://github.com/LedgerHQ/blue-app-monero/blob/master/doc/developer/blue-app-monero.pdf) gives more details (but is a little be obsolete)

The application device is located at https://github.com/LedgerHQ/blue-app-monero.

Open Integration

The integration has been done with the wishes to be open to any other hardware wallet.
To achieve that a C++ class hw::Device has been introduce. All the Monero
client code has been modified to use this new interface.

Two initial implementation are provided:

  • the "default", which remaps all call to initial Monero code
  • the "ledger", which delegates all call to Ledger device.

Now any HW that provide a class Device implementation would be defacto
supported by the Monero client.

What works

The today code support sending and receiving moneroj. It support main and sub-address.

In the "soon" future, it will support proof of send, proof of reserve.

Then will come other stuffs.

@dEBRUYNE-1

This comment has been minimized.

Copy link
Contributor

dEBRUYNE-1 commented Feb 21, 2018

Referencing #3095 if anyone is interested in previous discussion.

@PabraiVentures

This comment has been minimized.

Copy link

PabraiVentures commented Feb 21, 2018

Is this production tested and ready for general use?

@anonimal

This comment has been minimized.

Copy link
Contributor

anonimal commented Feb 21, 2018

@iDunk5400

This comment has been minimized.

Copy link
Contributor

iDunk5400 commented Feb 22, 2018

unit_tests should not crash if a device is not connected.

GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from build/release/tests/unit_tests/unit_tests...done.
[New LWP 21661]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/home/monero/build/release/tests/unit_tests/unit_tests --data-dir /'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  __GI___libc_free (mem=0x725eb8bd) at malloc.c:2951
  Id   Target Id         Frame 
* 1    Thread 0x7f154c32b780 (LWP 21661) __GI___libc_free (mem=0x725eb8bd) at malloc.c:2951

Thread 1 (Thread 0x7f154c32b780 (LWP 21661)):
#0  __GI___libc_free (mem=0x725eb8bd) at malloc.c:2951
#1  0x00007f154b6df2bd in SCardFreeMemory () from /lib/x86_64-linux-gnu/libpcsclite.so.1
#2  0x000055aacf779d8a in hw::ledger::DeviceLedger::connect() ()
#3  0x000055aacf5a2327 in tools::wallet2::load_keys(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, epee::wipeable_string const&) ()
#4  0x000055aacf5edc8f in tools::wallet2::load(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, epee::wipeable_string const&) ()
#5  0x000055aacf30497f in Serialization_portability_wallet_Test::TestBody() ()
#6  0x000055aacf6e7c99 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ()
#7  0x000055aacf6e1229 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ()
#8  0x000055aacf6c4108 in testing::Test::Run() ()
#9  0x000055aacf6c4abe in testing::TestInfo::Run() ()
#10 0x000055aacf6c51e3 in testing::TestCase::Run() ()
#11 0x000055aacf6cc576 in testing::internal::UnitTestImpl::RunAllTests() ()
#12 0x000055aacf6e9279 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ()
#13 0x000055aacf6e20a1 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ()
#14 0x000055aacf6cafb8 in testing::UnitTest::Run() ()
#15 0x000055aacf0609f1 in main ()

target_link_libraries(device
PUBLIC
pcsclite
crypto

This comment has been minimized.

Copy link
@MoroccanMalinois

MoroccanMalinois Feb 22, 2018

Contributor

crypto

${OPENSSL_CRYPTO_LIBRARIES}

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 22, 2018

Author Contributor

added for next commit

@cslashm

This comment has been minimized.

Copy link
Contributor Author

cslashm commented Feb 22, 2018

@iDunk5400 strange, test should use the default device. Not NanoS one. I check that.

Edited:
I launched

 ./build/release/tests/unit_tests/unit_tests --data-dir /tmp

And it does'nt crash, and do not attemp any access to the device "ledger"

void derivation_to_scalar(const key_derivation &derivation, size_t output_index, ec_scalar &res, hw::Device &device) {
device.derivation_to_scalar(derivation, output_index, res);
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

Could all those be in a device file ? Or at least grouped together. Not a big thing though.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

done.

created a crypto_device.cpp.


void crypto_ops::derivation_to_scalar(const key_derivation &derivation, size_t output_index, ec_scalar &res) {
struct {
key_derivation derivation;
crypto::key_derivation derivation;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

Is this needed ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 22, 2018

Author Contributor

I add this upon complier claimed it did not know the symbol else.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

removed


bool crypto_ops::derive_public_key(const key_derivation &derivation, size_t output_index,
const public_key &base, public_key &derived_key) {
ec_scalar scalar;
crypto::ec_scalar scalar;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

Is this needed ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 22, 2018

Author Contributor

same thing, will try to remove that tomorrow.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

removed


void crypto_ops::derive_secret_key(const key_derivation &derivation, size_t output_index,
const secret_key &base, secret_key &derived_key) {
ec_scalar scalar;
crypto::ec_scalar scalar;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

Is this needed ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

removed

assert(sc_check(&base) == 0);
derivation_to_scalar(derivation, output_index, scalar);
sc_add(&derived_key, &base, &scalar);
hw::ledger::log_hexbuffer("crypto_ops::derive_secret_key: OUT : base", derived_key.data, 32);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

This seems to be in a non-device function.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 22, 2018

Author Contributor

oops, I miss it! Even after reading the diff twice :(

Sorry will be removed

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

removed

}
device.set_signature_mode(device.SIGNATURE_FAKE);
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

Urgh. What is the purpose of these extra calls ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

After re-reading, not needed. Related to previous thoughts.

Removed

blockchain_db
p2p
epee
device

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

o_O Surely this is not needed, right ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 22, 2018

Author Contributor

The linker make me crazy about think found or not. YES I need help on this point. Could someone check and try to clean the dependencies?

@@ -33,7 +33,7 @@
#include "crypto-tests.h"

bool check_scalar(const crypto::ec_scalar &scalar) {
return crypto::sc_check(crypto::operator &(scalar)) == 0;
return sc_check(crypto::operator &(scalar)) == 0;

This comment has been minimized.

Copy link
@moneromooo-monero

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

because of:

/home/cme/Projects/Git/Monero/monero-cslashm/tests/crypto/crypto.cpp:36:18: error: ‘sc_check’ is not a member of ‘crypto’
   return crypto::sc_check(crypto::operator &(scalar)) == 0;

Moreover sc_check is defined in src/crypto/crypto-ops.c outside any namspace. So I dont understand this crypto::

Do I miss something?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 24, 2018

Contributor

Well, it was compiling fine before, no ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 24, 2018

Author Contributor

To be honest I do not remember as I had dis-activated test compilation for a long period.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

That needs to be worked out. It should not be needed unless something broke.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

see below

crypto::ge_tobytes(crypto::operator &(res), &point);
ge_p2 point;
ge_fromfe_frombytes_vartime(&point, reinterpret_cast<const unsigned char *>(&h));
ge_tobytes(crypto::operator &(res), &point);

This comment has been minimized.

Copy link
@moneromooo-monero

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

same as above

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Same, something broke and needs fixing.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

OK I think I found and I'm right.

in src/crypto/crypto.cpp we have

namespace crypto {

  using std::abort;
  using std::int32_t;
  using std::int64_t;
  using std::size_t;
  using std::uint32_t;
  using std::uint64_t;

  extern "C" {
    #include "crypto-ops.h"
    #include "random.h"
  }
....

The file test/crypto/crypto.cpp directly includes src/crypto/crypto.cpp (which a bit "ugly" I think).
This inclusion seems to temporarily move crypto-ops definition to crypto namespace

In my PR "crypto-ops.h" is incuded earlier pushing the definitions out of any namespace, as it should
according to namespace-less definition to crypto-ops.c/crypto-ops.h .

In conclusion I think this modifications should be keep.
(I also clean up my includes modification in the upcoming push update)

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Why does crypto.h now include crypto-ops.h ? Is it really needed ? You seem to add just new prototypes, so th enew include doesn't seem needed at first glance, and it's apparently causing this namepace change.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

new/moved API use ge_p3

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 28, 2018

Contributor

I've waded through this code, and I see now how it is. I'm fine with those changes.

crypto::hash_to_ec(key, tmp);
crypto::ge_p3_tobytes(crypto::operator &(res), &tmp);
ge_p3_tobytes(crypto::operator &(res), &tmp);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 22, 2018

Contributor

Why all the namespace changes ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 23, 2018

Author Contributor

same as above

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Here too.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

see above

cslashm added a commit to cslashm/monero that referenced this pull request Feb 23, 2018
@fluffypony

This comment has been minimized.

Copy link
Collaborator

fluffypony commented Feb 24, 2018

@cslashm to save you guys the hassle of having to rebase we've temporarily paused merging PRs, do you think you'll be able to do the last few tweaks to this on Monday? That way we don't leave things frozen for too long:)

@cslashm

This comment has been minimized.

Copy link
Contributor Author

cslashm commented Feb 24, 2018


namespace {
static std::map<std::string, Device&> registry;
static int inited = []() -> int {

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Put this into a function please. This is ripe for initialization order fiasco issues ... You'd have to move everything into a struct that has a single instance created in static memory, because otherwise the creation algorithm is racy between the two elements.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

You mean you want I use a function call in main let's device_init() from main and totally discard the use of static lambda construction?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

So, I move the code in a hw::device_init() function. And I call this func from all main() functions

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 26, 2018

Contributor

There's "on_startup" in src/common/util.cpp for these things.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

got it

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 27, 2018

Contributor

This does not solve the problem I was referencing. In C++, statics are initialized before main. If another constructor tries to ask for the device, and an object of that type is created in static memory, undefined behavior results. The only way to fix this specific problem is to do:

device& get_device(const std::string& name) {
    struct devices {
        devices() : device_map() { ... init... };

        std::map<std::string, std::unique_ptr<device> device_map;
    };
    static const devices instance;
    return instance.device_map.at(name);
}

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 27, 2018

Contributor

I should note that no constructor is calling this before main. But its easy to add this and add hard to spot undefined behavior.

@@ -112,10 +112,14 @@ namespace rct {

//does a * G where a is a scalar and G is the curve basepoint
void scalarmultBase(key & aG, const key &a);
void scalarmultBase(key & aG, const key &a, hw::Device &device);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

How are these redirects useful? Why wouldn't the places using them just call the function on the device directly?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

see other comment (Moneromooo request + device routing)

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

See other responses about this topic


key scalarmultBase(const key & a, hw::Device &device) {
key aG;
device.scalarmultBase(aG, a);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Why doesn't this function just return this value? Why the out parameter argument here?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

compute aG = a.G, as non device function.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 27, 2018

Contributor

Why doesn't the virtual method return the value like this function does?

}

secret_key generate_keys(public_key &pub, secret_key &sec, hw::Device &device) {
secret_key rng;

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

I mentioned this below, why these redirect functions? Why not just call the function directly?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

For the following reasons:

  • Upon @moneromooo-monero request who do not want modification in original low crypto functions
  • to manage the device abstraction between the various device implementations

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 27, 2018

Contributor

So wait, is the only reason to "blind" the device type in the function call? I.e. the header for the type does not have to be included in some other library, and instead forwards to here? There has to be a better arrangement for this ...

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

There are equivalent to the following ones defined in crypto.h/cpp and using underlaying device

static secret_key generate_keys(public_key &pub, secret_key &sec, const secret_key& recovery_key = secret_key(), bool recover = false);
friend secret_key generate_keys(public_key &pub, secret_key &sec, const secret_key& recovery_key, bool recover);

secret_key generate_keys(public_key &pub, secret_key &sec, hw::Device &device) {
secret_key rng;
device.generate_keys(pub, sec, secret_key(), false, rng);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Why is the device returning secret keys? Is this the view key?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

secret key are returned AES encrypted as said in PR comment. View/Spend key are never returned, even encrypted. As reminder:

The basic approach it to delegate all sensitive data (master key, secret ephemeral key,
key derivation, ....) and related operations to the device. As the device as low
memory the device do not keep itself the values (except for view/spend keys) but once
computed there are encrypted (AES) and return back to monero-wallet-cli. When
they need to be manipulated by the device, they are decrypted on receive.

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 27, 2018

Contributor

Lets starts with this - why are there two secrets keys here? What is being generated and returned by this call?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

as said:

There are equivalent to the following ones defined in crypto.h/cpp and using underlaying device

static secret_key generate_keys(public_key &pub, secret_key &sec, const secret_key& recovery_key = secret_key(), bool recover = false);
friend secret_key generate_keys(public_key &pub, secret_key &sec, const secret_key& recovery_key, bool recover);
#ifdef DEBUGLEDGER
//bool get_secret_keys(crypto::secret_key &viewkey , crypto::secret_key &spendkey);
#endif
bool generate_chacha_key(const cryptonote::account_keys &keys, crypto::chacha_key &key);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Theres lots to add in here, but add in the override tag to all of thee virtual functions so the compiler errors out if the base class interface changes. It will do this if the base class functions are pure virtual (because a definition is required), but otherwise this keyword helps subtle issues with interface changes.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

override added everywhere in device_default and device_ledger

DeviceDefault(const DeviceDefault &device);
~DeviceDefault();

DeviceDefault& operator=(const DeviceDefault &device);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Do the opposite, mark this as deleted: DeviceDefault& operator=(DeviceDefault const&) = delete;. The name of this class also breaks with existing style, but I guess @moneromooo-monero hasn't been concerned ...

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

renaming done, a bit boring but done:

  • Device=>device
  • DeviceDefault => device_default
  • DeviceLedger => device_ledger

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

So, If correctly understand I should mark all assignment copy as deleted to avoid their usage

Let me test that and think about it

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

done

class DeviceDefault : public hw::Device {
public:
DeviceDefault();
DeviceDefault(const DeviceDefault &device);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Mark as deleted, this time with DeviceDelete(DeviceDelete const&) = delete;

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

done

MDEBUG( "Device "<<this->id <<" Created");
}

DeviceLedger::DeviceLedger(const DeviceLedger &device): DeviceLedger() {

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Are the copy construtor and assignment ever used? I would just delete them, because its unlikely to be in use with this specific implementation.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

Rereading all, yes copy/assignment constructors are no more used. I removed all of them by marking them
=delete

MDEBUG( "Device "<<this->id << " LOCKed");
}
void DeviceLedger::unlock_device() {
MDEBUG( "Ask for UNLOCKING for device "<<this->id);

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 26, 2018

Contributor

Don't do this. It can throw, and leave the device locked. Best case this would have to be wrapped in its own try/catch block.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

Not sure to understand what you suggest. MDEBUG may throw exception?
So I modified with:

  try {
    MDEBUG( "Ask for UNLOCKING for device "<<this->id);
  } catch (...) {
  }

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 27, 2018

Contributor

Yes MDEBUG can throw in many different locations internally (it allocates memory, even if debug logging if off) .... yes I know.

@@ -1090,7 +1070,7 @@ void wallet2::process_new_transaction(const crypto::hash &txid, const cryptonote
for (size_t i = 0; i < additional_tx_pub_keys.size(); ++i)
{
additional_derivations.push_back({});
if (!generate_key_derivation(additional_tx_pub_keys[i], keys.m_view_secret_key, additional_derivations.back()))
if (!generate_key_derivation(additional_tx_pub_keys[i], keys.m_view_secret_key, additional_derivations.back(),device))

This comment has been minimized.

Copy link
@h908714124

h908714124 Feb 26, 2018

Contributor

Separate by one space, please.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

U+1F612

for (size_t n = 0; n < tx.vout.size(); ++n)
{
const cryptonote::txout_to_key* const out_key = boost::get<cryptonote::txout_to_key>(std::addressof(tx.vout[n].target));
if (!out_key)
continue;

crypto::public_key derived_out_key;
bool r = derive_public_key(derivation, n, address.m_spend_public_key, derived_out_key);
bool r = derive_public_key(derivation, n, address.m_spend_public_key, derived_out_key,device);

This comment has been minimized.

Copy link
@h908714124

h908714124 Feb 26, 2018

Contributor

spacing

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

U+1F612

@@ -486,6 +486,11 @@ namespace tools
void generate(const std::string& wallet, const epee::wipeable_string& password,
const cryptonote::account_public_address &account_public_address,
const crypto::secret_key& viewkey = crypto::secret_key());
/*!
*
*/

This comment has been minimized.

Copy link
@h908714124

h908714124 Feb 26, 2018

Contributor

Please complete the commentary, or remove it.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 26, 2018

Author Contributor

U+1F612

This comment has been minimized.

Copy link
@h908714124

h908714124 Feb 27, 2018

Contributor

Sorry. I was trying to help.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

This is right, though. This is pointless AFAICT :P

static_assert(sizeof(chacha_key) <= sizeof(hash), "Size of hash must be at least that of chacha_key");
tools::scrubbed_arr<char, HASH_SIZE> pwd_hash;
crypto::cn_slow_hash(data, size, pwd_hash.data());
crypto::cn_slow_hash_pre(data, size, pwd_hash.data(), prehashed);
memcpy(&key, pwd_hash.data(), sizeof(key));
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

I think it might be clearer to have a separate generate_chacha_key_prehashed, and leave the current code untouched.

This comment has been minimized.

Copy link
@m2049r

m2049r Mar 10, 2018

Contributor

this breaks builds for arm & x86 as cn_slow_hash_pre() exists only for

#if !defined NO_AES && (defined(__x86_64__) || (defined(_MSC_VER) && defined(_WIN64)))
//-----------------------------------------------------------------
hw::device& account_keys::get_device() const {
return *m_device;
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Did you try having a const/const and a non-const/non-const as suggested ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

yes. Whatever the way i choose it implies too many modification. I really would like postpone this.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

OK, I'll look at it once it's merged.

void account_base::create_from_device(const std::string &device_name)
{

hw::device &hwdev = hw::get_device("ledger");// m_keys.get_device();

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

"ledger" should not be in the lib. If this is the device_name, use it. Otherwise, pass another parameter.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

let me see that, yes maybe here a rest of the first integration

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

yes, You are right.

hwdev.init();
hwdev.connect();
hwdev.get_public_address(m_keys.m_account_address);
#ifdef DEBUGLEDGER

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Quick mass sed to DEBUG_HWDEVICE ?

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

done

crypto::secret_key m;
hwdev.get_subaddress_secret_key(a, index, m);
return m;
}

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

I have the same opinion as vtnerd, it would be better to keep the same prototype, just with an extra device parameter. But at this point I'm OK with leaving those if it's too much hassle.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 27, 2018

Author Contributor

I will see. The waythe various subaddress functions are used and mixed make keeping the double prototype a bit hard and maybe dangerous. Need to think calmly about this.


target_link_libraries(device
PUBLIC
pcsclite

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Still doesn't look optional to me.

char dd[32];
char logstr[128];

memmove(dd,d,len);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

I do not see the bounds checks though. Like if (len < 0 || len >= sizeof(dd)) do_something_errory();

@@ -33,7 +33,7 @@
#include "crypto-tests.h"

bool check_scalar(const crypto::ec_scalar &scalar) {
return crypto::sc_check(crypto::operator &(scalar)) == 0;
return sc_check(crypto::operator &(scalar)) == 0;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

That needs to be worked out. It should not be needed unless something broke.

crypto::ge_tobytes(crypto::operator &(res), &point);
ge_p2 point;
ge_fromfe_frombytes_vartime(&point, reinterpret_cast<const unsigned char *>(&h));
ge_tobytes(crypto::operator &(res), &point);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Same, something broke and needs fixing.

crypto::hash_to_ec(key, tmp);
crypto::ge_p3_tobytes(crypto::operator &(res), &tmp);
ge_p3_tobytes(crypto::operator &(res), &tmp);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Here too.

@cslashm

This comment has been minimized.

Copy link
Contributor Author

cslashm commented Feb 27, 2018

/* SETUP */
/* ======================================================================= */

static std::map<std::string, device&> registry;

This comment has been minimized.

Copy link
@vtnerd

vtnerd Feb 27, 2018

Contributor

This needs to be std::unique_ptr<device> not device&. Otherwise we are constantly going to get memcheck complaints about this.

toHash[3 * i + 2] = aG[i];
toHash[3 * i + 3] = aHP[i];
rv.II[i] = scalarmultKey(Hi, xx[i]);
}
}
precomp(Ip[i].k, rv.II[i]);

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Feb 27, 2018

Contributor

Would be nice to avoid breaking indent

@stoffu
stoffu approved these changes Feb 28, 2018
Copy link
Contributor

stoffu left a comment

Looks good overall. Only minor comments.

@@ -515,8 +515,11 @@ void slow_hash_free_state(void)
* @param length the length in bytes of the data
* @param hash a pointer to a buffer in which the final 256 bit hash will be stored
*/
void cn_slow_hash(const void *data, size_t length, char *hash) {
cn_slow_hash_pre(data,length,hash,0);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

Use false instead of 0

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

done

device_default(const device_default &device) = delete;
device_default& operator=(const device_default &device) = delete;

explicit operator bool() const { return false; };

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

override

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

done


class Keymap {
public:
std::vector<struct ABPkeys> ABP;

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

Use class to avoid a warning (or define ABPkeys as struct, or just write std::vector<ABPkeys>)

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

of course! Dont know why struct...

done

this->buffer_send[offset] = 0x00;
offset += 1;
//index
static_assert(sizeof(cryptonote::subaddress_index) == 8);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

static_assert needs a second parameter which is a string describing the error.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

done

this->buffer_send[offset] = 0x00;
offset += 1;
//index
static_assert(sizeof(cryptonote::subaddress_index) == 8);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

Same

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

done

return print_seed(false);
}

bool simple_wallet::encrypted_seed(const std::vector<std::string> &args/* = std::vector<std::string>()*/)
{
if (m_wallet->key_on_device())

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

This test can be inserted to print_seed instead to take effect for both seed and encrypted_seed at the same time.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

added to print_seed

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

I meant not only adding to print_seed, but also removing it from seed and encrypted_seed because they both call print_seed internally.

This comment has been minimized.

Copy link
@cslashm

cslashm Mar 1, 2018

Author Contributor

ok, removed

test_tx,
test_ptx);
}
hwdev.set_signature_mode(hwdev.SIGNATURE_FAKE);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

hw::device::SIGNATURE_FAKE to explicitly tell that it's a static const

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

done

}
hwdev.set_signature_mode(hwdev.SIGNATURE_FAKE);
}

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

Looks like the same code needs to be added to create_transactions_from as well.

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

let me see that in detail; who call it ...

Maybe I should reject this command/function today

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

It's needed for the sweep_all, sweep_below, sweep_single commands. You can add the same code to create_transactions_from just like you did to create_transactions_2. Very simple.

This comment has been minimized.

Copy link
@cslashm

cslashm Mar 1, 2018

Author Contributor

done

@@ -6819,6 +6850,8 @@ std::vector<wallet2::pending_tx> wallet2::create_transactions_2(std::vector<cryp
unsigned int original_output_index = 0;
std::vector<size_t>* unused_transfers_indices = &unused_transfers_indices_per_subaddr[0].second;
std::vector<size_t>* unused_dust_indices = &unused_dust_indices_per_subaddr[0].second;
hw::device &hwdev = m_account.get_device();
hwdev.set_signature_mode(hwdev.SIGNATURE_FAKE);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

hw::device::SIGNATURE_FAKE

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

done

if ((!dsts.empty()) ||
(dsts.empty() && !(adding_fee || !preferred_inputs.empty() || should_pick_a_second_output(use_rct, txes.back().selected_transfers.size(), *unused_transfers_indices, *unused_dust_indices)) )
) {
hwdev.set_signature_mode(hwdev.SIGNATURE_REAL);

This comment has been minimized.

Copy link
@stoffu

stoffu Feb 28, 2018

Contributor

hw::device::SIGNATURE_REAL

This comment has been minimized.

Copy link
@cslashm

cslashm Feb 28, 2018

Author Contributor

done

@cslashm cslashm force-pushed the cslashm:Ledger-NanoS branch from 1a41fed to 3351dfe Mar 1, 2018
@stoffu

This comment has been minimized.

Copy link
Contributor

stoffu commented Mar 2, 2018

Please don't forget to address this simple compile error:

src/wallet/api/wallet_manager.cpp:154:29: error: no matching member function for call to 'verify_password'
            return tools::wallet2::verify_password(keys_file_name, password, no_spend_key);
fail_msg_writer() << tr("command not supported by HW wallet");
return true;
}

This comment has been minimized.

Copy link
@stoffu

stoffu Mar 2, 2018

Contributor

You forgot to delete this.

This comment has been minimized.

Copy link
@cslashm

cslashm Mar 2, 2018

Author Contributor

yes, removed

@stoffu

This comment has been minimized.

Copy link
Contributor

stoffu commented Mar 2, 2018

Please consider applying the following patch:

diff --git a/src/device/CMakeLists.txt b/src/device/CMakeLists.txt
index cdb35d2a..26389220 100644
--- a/src/device/CMakeLists.txt
+++ b/src/device/CMakeLists.txt
@@ -67,7 +67,7 @@ monero_add_library(device
 target_link_libraries(device
   PUBLIC
     ${PCSC_LIBRARIES}
-    crypto
+    cncrypto
     ringct
     ${OPENSSL_CRYPTO_LIBRARIES}
     ${GNU_READLINE_LIBRARY}
diff --git a/src/device/device_ledger.cpp b/src/device/device_ledger.cpp
index d22ea5f7..58496f93 100644
--- a/src/device/device_ledger.cpp
+++ b/src/device/device_ledger.cpp
@@ -329,8 +329,15 @@ namespace hw {
 
       this->disconnect();
 
+#ifdef SCARD_AUTOALLOCATE
       dwReaders = SCARD_AUTOALLOCATE;
-      if ((rv = SCardListReaders(this->hContext, NULL, (LPSTR)&mszReaders, &dwReaders)) == SCARD_S_SUCCESS) {
+#else
+      rv = SCardListReaders(this->hContext, NULL, NULL, &dwReaders);
+      if (rv != SCARD_S_SUCCESS)
+        return false;
+      mszReaders = (LPSTR)calloc(dwReaders, sizeof(char));
+#endif
+      if ((rv = SCardListReaders(this->hContext, NULL, mszReaders, &dwReaders)) == SCARD_S_SUCCESS) {
         char* p;
         const char* prefix = this->name.c_str();
 
@@ -360,7 +367,11 @@ namespace hw {
       }
 
       if (mszReaders) {
+#ifdef SCARD_AUTOALLOCATE
         SCardFreeMemory(this->hContext, mszReaders);
+#else
+        free(mszReaders);
+#endif
         mszReaders = NULL;
       }
       if (rv != SCARD_S_SUCCESS) {
diff --git a/src/simplewallet/simplewallet.cpp b/src/simplewallet/simplewallet.cpp
index e6009517..02deee0c 100644
--- a/src/simplewallet/simplewallet.cpp
+++ b/src/simplewallet/simplewallet.cpp
@@ -3187,12 +3187,6 @@ bool simple_wallet::stop_mining(const std::vector<std::string>& args)
     return true;
   }
 
-  if (m_wallet->key_on_device())
-  {
-    fail_msg_writer() << tr("command not supported by HW wallet");
-    return true;
-  }
-
   COMMAND_RPC_STOP_MINING::request req;
   COMMAND_RPC_STOP_MINING::response res;
   bool r = net_utils::invoke_http_json("/stop_mining", req, res, m_http_client);
diff --git a/src/wallet/api/wallet_manager.cpp b/src/wallet/api/wallet_manager.cpp
index bb144227..9153eafc 100644
--- a/src/wallet/api/wallet_manager.cpp
+++ b/src/wallet/api/wallet_manager.cpp
@@ -151,7 +151,7 @@ bool WalletManagerImpl::walletExists(const std::string &path)
 
 bool WalletManagerImpl::verifyWalletPassword(const std::string &keys_file_name, const std::string &password, bool no_spend_key) const
 {
-           return tools::wallet2::verify_password(keys_file_name, password, no_spend_key);
+           return tools::wallet2::verify_password(keys_file_name, password, no_spend_key, hw::get_device("default"));
 }
 
 std::vector<std::string> WalletManagerImpl::findWallets(const std::string &path)

The only changes that may require a bit more care are those in ledger_device.cpp. The code was mostly copied from this tutorial site: https://ludovicrousseau.blogspot.jp/2010/04/pcsc-sample-in-c.html

Actually, the code on the website looks a lot similar to yours; perhaps did you also use this site when writing the code? Anyway, I noticed an oddity in your code where you (an the site) wrote

SCardListReaders(this->hContext, NULL, (LPSTR)&mszReaders, &dwReaders)

which is strange because mszReaders is already LPSTR (so &mszReaders would be LPSTR*). The above patch corrects this, please check.

@cslashm

This comment has been minimized.

Copy link
Contributor Author

cslashm commented Mar 2, 2018

@stoffu (LPSTR)&mszReaders is necessary. It's kind of C polymorphism hack, maybe a bit ugly but it works as is.

So I mod a bit your code proposition

@stoffu
stoffu approved these changes Mar 4, 2018
@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

moneromooo-monero commented Mar 4, 2018

Please squash and fix the commit message and then it can go in.

@cslashm cslashm force-pushed the cslashm:Ledger-NanoS branch from d636ad2 to efc5ecf Mar 4, 2018
@cslashm

This comment has been minimized.

Copy link
Contributor Author

cslashm commented Mar 4, 2018

squashed

The basic approach it to delegate all sensitive data (master key, secret
ephemeral key, key derivation, ....) and related operations to the device.
As device has low memory, it does not keep itself the values
(except for view/spend keys) but once computed there are encrypted (with AES
are equivalent) and return back to monero-wallet-cli. When they need to be
manipulated by the device, they are decrypted on receive.

Moreover, using the client for storing the value in encrypted form limits
the modification in the client code. Those values are transfered from one
C-structure to another one as previously.

The code modification has been done with the wishes to be open to any
other hardware wallet. To achieve that a C++ class hw::Device has been
introduced. Two initial implementations are provided: the "default", which
remaps all calls to initial Monero code, and  the "Ledger", which delegates
all calls to Ledger device.
@cslashm cslashm force-pushed the cslashm:Ledger-NanoS branch from efc5ecf to e745c1e Mar 4, 2018
Copy link
Collaborator

fluffypony left a comment

Reviewed