refactor: deduplicate the message sign/verify code #17577

vasild commented at 4:35 PM on November 24, 2019: member

The message signing and verifying logic was replicated in a few places in the code. Consolidate in a newly introduced MessageSign() and MessageVerify() and add unit tests for them.

fanquake added the label Refactoring on Nov 24, 2019

vasild force-pushed on Nov 24, 2019

DrahtBot commented at 5:11 PM on November 24, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)
#18115 (wallet: Pass in transactions and messages for signing instead of exporting the private keys by achow101)
#17953 (refactor: Abstract boost::variant out by elichai)
#17938 (Disallow automatic conversion between disparate hash types by Empact)
#17809 (rpc: Auto-format RPCResult by MarcoFalke)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
#16440 (BIP-322: Generic signed message format by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/util/message.cpp:20 in fcb1d27839 outdated

  15 | +// We moved a call to boost::get() here from another file. It is a good practice
  16 | +// to include the header that provides it (<boost/variant/get.hpp>). However
  17 | +// test/lint/lint-includes.sh:90 complains that a new Boost dependency is
  18 | +// introduced because even though boost::get() is used in many other places in
  19 | +// the code, nobody has bothered to include <boost/variant/get.hpp>.
  20 | +//#include <boost/variant/get.hpp> // For boost::get()

vasild commented at 5:11 PM on November 24, 2019:

For some reason it is possible to use boost::get() without including the header that provides it. Probably included from somewhere else, maybe from QT stuff.

Just drop lines 15-20 or somehow silence the lint script to allow this header to be included (a new boost dependency is not added by this patch)?

vasild commented at 11:14 AM on December 24, 2019:

I dropped lines 15-20

fanquake commented at 6:18 PM on November 24, 2019: member

Looks like this is going to overlap with #17557.

vasild commented at 8:00 PM on November 24, 2019: member

Affairs with conflicting PRs:

#17557 util: Refactor message hashing into a utility function Both PRs target the same duplicated code! However this PR (#17577) has broader scope as it deduplicates more code.
#17493 util: Forbid ambiguous repeated assignments in config file Purely mechanical conflicts. Both PRs touch neighboring code. Either one can be merged first, the second will need to be adjusted (easy).
#17473 refactor: Settings code cleanups Purely mechanical conflicts. Same as above.
#17399 validation: Templatize ValidationState instead of subclassing Purely mechanical conflicts. Same as above.
#16545 Implement missing error checking for ArgsManager flags Purely mechanical conflicts. Same as above.

vasild force-pushed on Nov 24, 2019

vasild commented at 8:08 PM on November 29, 2019: member

@jkczyz What do you think about this PR and its relation to #17557? Merge both in one PR?

jkczyz commented at 4:29 PM on December 4, 2019: contributor

@jkczyz What do you think about this PR and its relation to #17557? Merge both in one PR?

I have #17557 based on an outstanding PR (#17399), though I suppose it didn't have to be. The former resulted from a comment in the latter. I deliberately kept the change small so it could hopefully be merged quicker. Though I wanted to add a test to clarify why the "magic" string is needed.

That said, from a design perspective, I prefer your PR as it hides the complexity around signing a message.

vasild force-pushed on Dec 7, 2019

vasild commented at 8:03 PM on December 7, 2019: member

@jkczyz thanks for the feedback! I did cherry-pick the last two commits from #17557 into this PR under e3d85bcd5 - the hashing utility function and the unit test for it (to clarify why the "magic" string is needed).

vasild commented at 8:14 PM on December 7, 2019: member

Conflicts

Reviewers, this pull request conflicts with the following ones:

* [#17593](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17593.html) (test: move more utility functions into test utility library by mzumsande)

* [#17581](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17581.html) (refactor: Remove settings merge reverse precedence code by ryanofsky)

* [#17580](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17580.html) (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)

* [#17493](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17493.html) (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

* [#17473](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17473.html) (refactor: Settings code cleanups by ryanofsky)

* [#17399](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/17399.html) (validation: Templatize ValidationState instead of subclassing by jkczyz)

* [#16545](https://drahtbot.github.io/bitcoin_core_issue_redirect/r/16545.html) (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

All of the above conflicts are mechanical, small and easy to resolve.

DrahtBot added the label Needs rebase on Dec 16, 2019

instagibbs commented at 4:18 PM on December 19, 2019: member

concept ACK, though I'd probably be more pedantic in calling it "Bitcoin Signed Message Protocol" or something. Unfortunately there's no BIP to refer to.

vasild force-pushed on Dec 24, 2019

vasild commented at 11:23 AM on December 24, 2019: member

@instagibbs Thanks for the review! Where would you put the "Bitcoin Signed Message Protocol" label?

We do message signing and verification now. What about encryption and decryption? Would that be useful feature or just a bloat that is rarely used? If encryption may be added in the future then maybe just "Bitcoin Message Protocol" would be a better name, or "Bitcoin Text Message Protocol".

DrahtBot removed the label Needs rebase on Dec 24, 2019

laanwj commented at 3:21 PM on February 5, 2020: member

I don't think encryption / decryption in bitcoind is a good idea. It is best to avoid using keys for multiple purposes (e.g. use a signing key for encryption, and vice versa). Also it's scope creep: Bitcoin core is a validating full node. It should do that well, and not try to make a GPG-ish mess of it that tries to do everything with a mash-up of features but nothing particularly well…

Sjors commented at 8:58 AM on February 12, 2020: member

See also @achow101's #18115

vasild commented at 10:45 AM on February 12, 2020: member

See also @achow101's #18115 @Sjors thanks for the notice!

This PR (https://github.com/bitcoin/bitcoin/pull/17577) deduplicates message signing and message verification code.

I peeked at https://github.com/bitcoin/bitcoin/pull/18115/ and among other things it deduplicates the message signing code from 2 of 3 places and does not deduplicate the message verification code. Assuming it also fixes the 3rd place where message signing code is repeated, then it will achieve half of this PR.

Should https://github.com/bitcoin/bitcoin/pull/18115/ hit master before this PR, then I will trim this PR to only consolidate the message verification code.

in src/qt/signverifymessagedialog.cpp:198 in 395cc5c198 outdated

 228 | +    const std::string& signature = ui->signatureIn_VM->text().toStdString();
 229 | +    const std::string& message = ui->messageIn_VM->document()->toPlainText().toStdString();
 230 | +
 231 | +    switch (MessageVerify(address, signature, message)) {
 232 | +        case MessageVerificationResult::ERR_INVALID_ADDRESS:
 233 | +            ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");

achow101 commented at 7:59 PM on February 12, 2020:

These ui->statusLabel_VM->setStyleSheet("QLabel { color: red; }");s could be moved out of the switch to further reduce duplication.

vasild commented at 1:06 PM on February 13, 2020:

Done.

vasild commented at 8:08 PM on February 12, 2020: member

I peeked at #18115 and among other things it deduplicates the message signing code from 2 of 3 places and does not deduplicate the message verification code. Assuming it also fixes the 3rd place...

That was fixed and now #18115 deduplicates all 3 message signing repetitions.

MessageSign() from #17577 is equivalent to SignMessage() from #18115. The former has unit tests added in #17577.

in src/util/message.cpp:23 in c1af1190a9 outdated

  19 | +static const std::string MESSAGE_MAGIC = "Bitcoin Signed Message:\n";
  20 | +
  21 | +bool MessageSign(
  22 | +    const CKey& privkey,
  23 | +    const std::string& message,
  24 | +    std::string* signature)

achow101 commented at 8:09 PM on February 12, 2020:

This should be a reference (std::string&) instead of pointer.

vasild commented at 1:06 PM on February 13, 2020:

Done.

in src/test/util_tests.cpp:2105 in 1a00fb8417 outdated

2100 | +{
2101 | +    const std::string unsigned_tx = "...";
2102 | +    const uint256 signature_hash = Hash(unsigned_tx.begin(), unsigned_tx.end());
2103 | +
2104 | +    const uint256 message_hash = MessageHash(unsigned_tx);
2105 | +    BOOST_CHECK_NE(message_hash, signature_hash);

achow101 commented at 8:13 PM on February 12, 2020:

I think you should have on additional check here with the full hashed message. i.e. also hash Bitcoin Signed Message:\n... and compare that with message_hash.

vasild commented at 1:06 PM on February 13, 2020:

Done.

vasild commented at 1:09 PM on February 13, 2020:

Notice: assuming CHashWriter hasher(SER_GETHASH, 0);, then:

hasher << "ab" << "cd";

would produce a different result than:

hasher << "abcd";

in src/util/message.cpp:18 in c1af1190a9 outdated

  14 | @@ -14,7 +15,27 @@
  15 |  #include <string>
  16 |  #include <vector>
  17 |  
  18 | -const std::string strMessageMagic = "Bitcoin Signed Message:\n";
  19 | +static const std::string MESSAGE_MAGIC = "Bitcoin Signed Message:\n";

achow101 commented at 8:14 PM on February 12, 2020:

Renaming this should probably go in the next commit?

vasild commented at 1:06 PM on February 13, 2020:

Done.

achow101 commented at 8:14 PM on February 12, 2020: member

Couple of nits. otherwise ACK 1a00fb84171666451cb5a21ba454755dab6e0328

vasild force-pushed on Feb 13, 2020

Sjors commented at 2:05 PM on February 13, 2020: member

tACK ad39c8fd8f2f7752e4f4508ab880873008c7452e, thanks for adding tests as well

achow101 commented at 9:48 PM on February 13, 2020: member

I'm seeing a new compiler warning:

In file included from ./script/signingprovider.h:13,
                 from ./outputtype.h:10,
                 from rpc/misc.cpp:8:
rpc/misc.cpp: In function ‘UniValue verifymessage(const JSONRPCRequest&)’:
./sync.h:179:104: warning: control reaches end of non-void function [-Wreturn-type]
  179 | #define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
      |                                                                                                        ^
rpc/misc.cpp:273:5: note: in expansion of macro ‘LOCK’
  273 |     LOCK(cs_main);
      |     ^~~~

in src/rpc/misc.cpp:291 in 93cc8500d4 outdated

 289 | +        case MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED:
 290 | +        case MessageVerificationResult::ERR_NOT_SIGNED:
 291 | +            return false;
 292 | +        case MessageVerificationResult::OK:
 293 | +            return true;
 294 |      }

achow101 commented at 9:48 PM on February 13, 2020:

You need another return false statement after this switch.

Sjors commented at 9:09 AM on February 14, 2020:

Nice catch. I'm not getting that -Wreturn-type warning on macOS. Only one of the Travis machines noticed it: https://travis-ci.org/bitcoin/bitcoin/jobs/649943882?utm_medium=notification&utm_source=github_status

vasild commented at 10:47 AM on February 14, 2020:

Added return false;, thanks!

Deduplicate the message verifying code

The logic of verifying a message was duplicated in 2 places:

src/qt/signverifymessagedialog.cpp
  SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()

src/rpc/misc.cpp
  verifymessage()

with the only difference being the result handling. Move the logic into
a dedicated

src/util/message.cpp
  MessageVerify()

which returns a set of result codes, call it from the 2 places and just
handle the results differently in the callers.

2ce3447eb1

Deduplicate the message signing code

The logic of signing a message was duplicated in 3 places:

src/qt/signverifymessagedialog.cpp
  SignVerifyMessageDialog::on_signMessageButton_SM_clicked()

src/rpc/misc.cpp
  signmessagewithprivkey()

src/wallet/rpcwallet.cpp
  signmessage()

Move the logic into

src/util/message.cpp
  MessageSign()

and call it from all the 3 places.

f8f0d9893d

Refactor message hashing into a utility function

And add unit test for it.

The purpose of using a preamble or "magic" text as part of signing and
verifying a message was not given when the code was repeated in a few
locations. Make a test showing how it is used to prevent inadvertently
signing a transaction.

e193a84fb2

vasild force-pushed on Feb 14, 2020

Sjors commented at 12:00 PM on February 14, 2020: member

re-ACK e193a84fb28068e38d5f54fbfd6208428c5bb655

achow101 commented at 3:37 PM on February 14, 2020: member

ACK e193a84fb28068e38d5f54fbfd6208428c5bb655

MarcoFalke referenced this in commit 33861a8367 on Feb 16, 2020

Sjors commented at 12:11 PM on February 19, 2020: member

@meshcollider some of this touches the wallet - and #18115 that builds on top of this definitely does - so maybe you can take a look?

laanwj added this to the "Blockers" column in a project

in src/util/message.h:44 in 2ce3447eb1 outdated

  39 | +/** Verify a signed message.
  40 | + * @param[in] address Signer's bitcoin address, it must refer to a public key.
  41 | + * @param[in] signature The signature in base64 format.
  42 | + * @param[in] message The message that was signed.
  43 | + * @return result code */
  44 | +MessageVerificationResult MessageVerify(

instagibbs commented at 3:34 PM on February 21, 2020:

nit: I'd prefer actual types used instead of strings for all args.

instagibbs commented at 3:47 PM on February 21, 2020:

this is a non-blocking comment :) let's get this merged

instagibbs commented at 3:46 PM on February 21, 2020: member

utACK https://github.com/bitcoin/bitcoin/pull/17577/commits/e193a84fb28068e38d5f54fbfd6208428c5bb655

meshcollider commented at 10:24 AM on February 25, 2020: contributor

utACK e193a84fb28068e38d5f54fbfd6208428c5bb655

Nice cleanup, thanks!

meshcollider merged this on Feb 25, 2020

meshcollider closed this on Feb 25, 2020

vasild deleted the branch on Feb 25, 2020

meshcollider removed this from the "Blockers" column in a project

sidhujag referenced this in commit be3a59ce8e on Feb 25, 2020

jasonbcox referenced this in commit a6c7f25fee on Oct 24, 2020

sidhujag referenced this in commit 3a54196b28 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022