[DONOT MERGE] Ledger HW code review 1

[cslashm]

Goals

The goal of this PR is to propose code modifications to integrate the ledger HW into
monero-wallet-cli. This code mod is not ready to merge (see below) but it is
time to ask to the dev team and tech guys a first review for opinion.

Approach

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.

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.

As the transaction is constructed several times, the device signature operates in tow
modes: "FAKE" and "REAL". In fake mode, the device signs the the transaction with
random key without asking anything to the use. In real mode, the device use the
right keys and ask the user to valid (on the HW screen) fees, destinations, amounts.
For now this is handled in wallet2.cpp:7015.

NEEDHELP: some time the real mode switch and resign fails and the correct signature is not generated. There is certainly a better way to do that :)

What is supported

Creation and Restore

A new command line option has been added: --generate-from-device

*Spend/receive money

Subaddress

Seems to work.

More to come

I will look at Export/Import Keyimage

I hope to add multi-sig.

LightWallet need be more studied

more....

DO NOT MERGE

There is some clean up to do. NEEDHELP to fix. Review to merge. Commands to
lock when device is used (unsupported commands ....). TX mutex to manage. Security to add. ....

[moneromooo-monero]

When modifying a function to add a device parameter which changes the whole function, please do something like this at the top of the function:

if (device)
{
  /* do stuff with the device */
  return result;
}

This will avoid the existing code being all changed by pointless indents and make reviewing easier.

[gtklocker]

Kudos for this admirable work! However, are we sure that having an extra argument on every function just for Ledger is the best way to go?

I'll use crypto_ops as an example but what I'm saying applies to more parts of the code in pretty much the same way:

From what I see, crypto_ops holds no state and mostly acts as a namespace instead of a class. On its own I'm not quite sure why this is so (and it's not a change that this PR made), but let's not focus on it right now.

I definitely appreciate that the API is backwards compatible. However, if/when we add support for more HW devices, should we expect to augment these arguments once more? Should our methods be a huge if statement switching between devices? What happens if we want to remove support for Ledger at some point in the future? Plus, if many devices (in the future) are passed to, say, crypto_ops::generate_keys, and many of them are valid, what's going to happen? Should every function crypto_ops include code to account for these cases?

In any case the if (device) { ... } code and the way it's all mixed with the local wallet code feels a bit repetitive and error-prone.

It looks to me like the HW wallet and local wallet code should be clearly separated.

Disclaimer: I didn't have the chance to fully examine this PR and I am in no way familiar with the Monero codebase.

[moneromooo-monero]

Could be something like this, assuming this sample function:

void secret_key_to_public_key(crypto::secret_key&, crypto::public_key&);

You rename that to:

void secret_key_to_public_key_default(crypto::secret_key&, crypto::public_key&);

Which gives you few diffs in the crypto code, and none in the callers, then:

void secret_key_to_public_key(device_t *device, crypto::secret_key &skey, crypto::public_key &pkey)
{
device->secret_key_to_public_key(skey, pkey);
}

Then add a device for default:

typedef struct
{
void (*secret_key_to_public_key)(crypto::secret_key &skey, crypto::public_key &pkey);
/* and all the others ones */
} device_t;

device_t *default_device = { &secret_key_to_public_key_default };
device_t *ledger_device = { &some_other_function_doing_the_ledger_equivalent };

Though, on second thought, you get diffs in the callers anyway to insert the device parameter, so might as well keep the original functions name the right way, and make the trampoline functiuon secret_key_to_public_key_for or something.

[jroelofs]

The compiler will optimize out this dead memset, leaving that key data on the stack.

Use scrubbed_arr<char, 200> prekey instead.

[cslashm]

OK, noted.

[jroelofs]

This writes into offset 4 again, instead of 6, over-writing what was stored on line 523. Is that intentional?

[cslashm]

Yes It is. Code is today not optimized to help debug and update.

Command in buffer_send is

    00(u8) INS(u8) P1(u8) P1(u8) LEN(8) PAYLOAD(LEN bytes)

[jroelofs]

This writes into offset 4 again, instead of 6, over-writing what was stored on line 487. Is that intentional?

[cslashm]

See above. All command use the same template. It will be factorized.

Thanks for all your comments

[cslashm]

gtklocker, I aggree with you, but the separation between HW wallet and local wallet is a little bit hard today. Indeed today the Monero code is a mix of objects, global variables and stateless namspace.

I try to find the best way to delegate computation to device with modifying the fewest API. (And had to change some part of my first approach for subaddress)

moneromooo-monero, I like your approach but with slightly modification. Instead of defining struct with function pointers, I think it would be better to use inheritance.

class Device {
    virtual void secret_key_to_public_key(crypto::secret_key &skey, crypto::public_key &pkey) = 0;
   /* other delegated function */
}

class DefaultDevice : Device {
    void secret_key_to_public_key(crypto::secret_key &skey, crypto::public_key &pkey);
    /* other concrete delegated function */
}
class LedgerHWDevice : Device {
    void secret_key_to_public_key(crypto::secret_key &skey, crypto::public_key &pkey);
    /* other concrete delegated function */
}

Now you still have a problem to this kind of code:

      if (device) {   
        hashes.push_back(key());                   
      } else {
        hashes.push_back(hash2rct(h));
      }

Which is not a crypto delegation but local variablas handling.

Then you also have to deal with the following problem with (for example) the sc_add function.
Sometimes you explicitly want to call the default one, as in proveRange, sometimes you want to call the device one.

Finally there is some tricky case such as in MLSAG_GEN:

            if (device) {
                Hi = hashToPoint(pk[index][i]);
                device.mlsag_prepare(Hi, xx[i], alpha[i] , aG[i] , aHP[i] , rv.II[i]);
                toHash[3 * i + 1] = pk[index][i];
                toHash[3 * i + 2] = aG[i];
                toHash[3 * i + 3] = aHP[i];
                precomp(Ip[i].k, rv.II[i]);
                /*debug code removes for words clarity*/
            } else {
                toHash[3 * i + 1] = pk[index][i];
               /*multisig code removed for words clarity */
               {
                    Hi = hashToPoint(pk[index][i]);
                    skpkGen(alpha[i], aG[i]); //need to save alphas for later..
                    aHP[i] = scalarmultKey(Hi, alpha[i]);
                    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]);   
            }

A final remark, I do not says my mods are the best way but as the HW app code is open, (almost ;-) ) right specified, try to protect all part (key, secret TX key, derivation, key amount...) it can be implemented by all other future HW wallet.
So supporting a new HW, will not add a new param but just an other DeviceXXX type.

[moneromooo-monero]

The advantage is using a struct is that it stays callable from C.

In that example code above, prepare_mlsag is "skpkGen + a couplescalarmultKey", right ? All the other code seems the same. Then I think it'd be fine having a function which does this (maybe get it a more descriptive function, luigi might have a good name for it).

[cslashm]

But do we need to be callable at C level?

for the tricky case, do you suggest to apply the same patten to all such cases? I mean replace code:

void foo(..., Device: &device) {

    COMMON_CODE;

    if(device) {
        code1;
    } else {
        code2
    }

    COMMON_CODE;
}

by

void sub_foo(..., Device: &device) {
  if(device) {
        code1;
    } else {
        code2
    }
}

void foo(..., Device: &device) {
    COMMON_CODE;
    sub_foo(....,device);
    COMMON_CODE;
}

[moneromooo-monero]

The current crypto functions are already called from C.

As for your example, I guess it depends on the details, but my preliminary opinion is that if we end up adding more devices later, the "if" version will grow a lot, while the indirection one will not, so the indirection one seems best. I suppose if it's in a small performance sensitive path, that calculation might change.

[cslashm]

Ok for C calling. Is there any plan to clean code code and made it full C++ ?

So does it mean that I should perform the following in tricky part:

void foo(..., Device: &device) {
    COMMON_CODE;
    device->sub_foo(....);
    COMMON_CODE;
}

[gtklocker]

@moneromooo-monero any example with C calling?

@cslashm Your inheritance proposal is what I had in mind. In this sense, just like you said, crypto_ops should become an interface and there should be crypto ops implemented for a local wallet and for Ledger. Then you select the correct crypto_ops once in your code, and you're good to go. This helps keep the code separate and less error prone (and makes extensibility a piece of cake).

I realise that this is a big structural change that should probably not be the focus on this PR, and while I'm pretty excited to get this code merged ASAP I don't think we should just be hasty and not do this the way we think is right. Again, the tradeoff is obvious. Just my 2 cents.

I'm not sure I understand your final remark, could you be a please restate it?

[moneromooo-monero]

About cleaning up: depends what there is to cleanup, you'll have to give examples:
About converting everyhting to C++: what would be the point ?
About an example with C calling: I do not understand what you want here.

[cslashm]

@gtklocker My last remark was about such code

            if (device) {
                Hi = hashToPoint(pk[index][i]);
                device.mlsag_prepare(Hi, xx[i], alpha[i] , aG[i] , aHP[i] , rv.II[i]);
                toHash[3 * i + 1] = pk[index][i];
                toHash[3 * i + 2] = aG[i];
                toHash[3 * i + 3] = aHP[i];
                precomp(Ip[i].k, rv.II[i]);
                /*debug code removes for words clarity*/
            } else {
                toHash[3 * i + 1] = pk[index][i];
               /*multisig code removed for words clarity */
               {
                    Hi = hashToPoint(pk[index][i]);
                    skpkGen(alpha[i], aG[i]); //need to save alphas for later..
                    aHP[i] = scalarmultKey(Hi, alpha[i]);
                    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]);   
            }

that, if I correctly understood the @moneromooo-monero words, should be refactor as:

  device->well-named_func(hashToPoint,xx[i], alpha[i] , aG[i] , aHP[i] , rv.II[i], p[i].k)

or at least as

  well-named_func(device, hashToPoint,xx[i], alpha[i] , aG[i] , aHP[i] , rv.II[i], p[i].k)

[cslashm]

@moneromooo-monero when I speak all in C++ I think about (for example) transforming the stateless crypto-ops in a non stateless object that could delegate itself some computation. Cleanup is just other things like that, wrong word maybe.

But we are speaking about change the arch of the code which is a complicated subject and this PR is maybe not the right place. And I don't know if is the right timing :)

The real question today is what about this PR and where to put the cursor between "as soon as possible" and "what about future HW" . I'm afraid the answer is not on my side :)

I'm definitively open to any suggestion/request to modify my code to make this happen, and happen with compliance with future and other HW ;)

[moneromooo-monero]

You could have a stateful object, with the state being the device. But its member functions would need to call the C functions for the "no device" case. and those functions will have to keep in C since the're called by other C stuff. And frankly I enjoy the fact they compile in a split second rather than a split minute :)

[cslashm]

I'm already at around 20 min to compile, one more or less does not make the diff 🤣

More seriously, I will have a look (next week) to see what could be the device interface to avoid the if(device) everywhere

[rndbr]

thanks for doing this @cslashm -- was just checking to see if you've had any time to work on this lately

[cslashm]

Brief

Upon discussion on IRC#monero-dev with moneromoo and some here remarks,
I propose this new integration approach.

The two main ideas are to minimize original code modification, especially
the original crypto-ops sensitive parts, and to allow (almost) easy integration
of other HW (thinking they use the same kind of approach)

Important Note

The previous PR worked. I did full send/receive on testnet.
So the Proof of Concept has proved the concept :)

This update has not been debugged yet. Debug is sometime (always?) a pain and
I want Monero team to agree with the integration model before
diving again in the crypto limbs :-).

Model integration

Simple case

For almost original function that need to be delegated a new one is defined.

e.g., for the original function:

namespace foo {
   retType func(type1 arg1, type2 arg2) {
       //Some code computing retval;
       return retval
   }
}

it is duplicated using parametric polymorphism

namespace foo {
   retType func(type1 arg1, type2 arg2) {
       //Some code computing retval;
       return retval

   retType func(type1 arg1, type2 arg2, hw::Device &device) {
       retType retval;
       device.fun(arg1, arg2, retval);
       return retval
   }
}

The hw::Device class is basically a error class defined as:

namespace hw {
    class Device {

       namespace {
            //device funcion not supported
            inline bool dfns()  {throw std::runtime_error("device function not supported");return false;}
        }

        virtual bool func(type1 &arg1, type2 &arg2, retType &retval) { return dfns();}
    }
}

For now two real class are defined: hw::code:DefaultDevive and hw:ledger::LedgerDevice.

The former is basically a proxy forwarder :

namespace hw {
    namespace core {
    class DefaultDevice :: public hw::Device{

    bool func(type1 &arg1, type2 &arg2, retType &retval) {
        retval = foo::func1(arg1,arg2)
        return true;
    }
}

The hw::ledger::LedgerDevice class is defined in the same manner but interacty with the NanoS instead of forwarding call.

Thus the original function is untouched and still be available for normal usage. More over the common Device abstraction allow future
HW to be integrated easier.

Complex case

For some complex case of the followig form:

namespace foo {
   retType func(type1 arg1, type2 arg2) {

      //long complex code

      //code to delegate

      //more long complex code
   }
}

the function is not duplicated, but only the code to delegate is moved.

namespace foo {
    retType func(type1 arg1, type2 arg2, hw::Device &device) {

       //long complex code

       device->func(arg1,arg2)

       //more long complex code
    }
}

and device is so:

namespace hw {
    namespace core {
    class DefaultDevice :: public hw::Device{

    bool func(type1 &arg1, type2 &arg2) {
     //code to delegate
    }
}

A typical example is MLSAG_Gen in rctSigs.cpp.

[moneromooo-monero]

I agree with the new approach, thanks.
At a quick glance, half the changes are spurious changes (adding an unnecessary namespace::, whitespace, swappines lines without need, etc), makes review harder, and these will need removing (not necessarily now, but hopefully before actual review :)).

[cslashm]

Thank you moneromooo. This is is a very great great news!
I will start the new debug next week.

Yes I will make a full review with no space removing/adding, new line...and other things
I saw the problem just after the push and did not find energy to fix it for this intermediate push ;)

[rndbr]

hi @cslashm, was wondering if you had an update on this for github or reddit as it's been about 2 weeks. thanks

[cslashm]

Hi @rndbr,

Was busy last weeks on other ledger stuff.

Restart the integration since yesterday.

[rndbr]

hi @cslashm, just pinging you again on behalf of the monero community. :) I figure I do it every 2 weeks or so, so it's not too annoying, but the community has a better sense where things currently are. thank you for your work on this.

[cslashm]

Replaced by #3303

[moneromooo-monero]

+duplicate