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.

Sign up for GitHub

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

Jump to bottom

Always create signatures with Low R values #13666

Merged
merged 3 commits into from
Aug 13, 2018
Merged

Always create signatures with Low R values #13666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
18dfea0
Always create 70 byte signatures with low R values
achow101 Jul 14, 2018
48b1473
Use 71 byte signature for DUMMY_SIGNATURE_CREATOR
achow101 Jul 15, 2018
e306be7
Use 72 byte dummy signatures when watching only inputs may be used
achow101 Aug 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bench/verify_script.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ static void VerifyScriptBench(benchmark::State& state)
CMutableTransaction txSpend = BuildSpendingTransaction(scriptSig, txCredit);
CScriptWitness& witness = txSpend.vin[0].scriptWitness;
witness.stack.emplace_back();
key.Sign(SignatureHash(witScriptPubkey, txSpend, 0, SIGHASH_ALL, txCredit.vout[0].nValue, SigVersion::WITNESS_V0), witness.stack.back(), 0);
key.Sign(SignatureHash(witScriptPubkey, txSpend, 0, SIGHASH_ALL, txCredit.vout[0].nValue, SigVersion::WITNESS_V0), witness.stack.back());
witness.stack.back().push_back(static_cast<unsigned char>(SIGHASH_ALL));
witness.stack.push_back(ToByteVector(pubkey));

Expand Down
24 changes: 22 additions & 2 deletions src/key.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,15 +189,35 @@ CPubKey CKey::GetPubKey() const {
return result;
}

bool CKey::Sign(const uint256 &hash, std::vector<unsigned char>& vchSig, uint32_t test_case) const {
// Check that the sig has a low R value and will be less than 71 bytes
bool SigHasLowR(const secp256k1_ecdsa_signature* sig)
{
unsigned char compact_sig[64];
secp256k1_ecdsa_signature_serialize_compact(secp256k1_context_sign, compact_sig, sig);

// In DER serialization, all values are interpreted as big-endian, signed integers. The highest bit in the integer indicates
// its signed-ness; 0 is positive, 1 is negative. When the value is interpreted as a negative integer, it must be converted
// to a positive value by prepending a 0x00 byte so that the highest bit is 0. We can avoid this prepending by ensuring that
// our highest bit is always 0, and thus we must check that the first byte is less than 0x80.
return compact_sig[0] < 0x80;
}

bool CKey::Sign(const uint256 &hash, std::vector<unsigned char>& vchSig, bool grind, uint32_t test_case) const {
if (!fValid)
return false;
vchSig.resize(CPubKey::SIGNATURE_SIZE);
size_t nSigLen = CPubKey::SIGNATURE_SIZE;
unsigned char extra_entropy[32] = {0};
WriteLE32(extra_entropy, test_case);
secp256k1_ecdsa_signature sig;
int ret = secp256k1_ecdsa_sign(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, test_case ? extra_entropy : nullptr);
uint32_t counter = 0;
int ret = secp256k1_ecdsa_sign(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, (!grind && test_case) ? extra_entropy : nullptr);

// Grind for low R
while (ret && !SigHasLowR(&sig) && grind) {
WriteLE32(extra_entropy, ++counter);
ret = secp256k1_ecdsa_sign(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, extra_entropy);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe overkill, but you could in theory do

while (secp256k1_ecdsa_sign(secp256k1_context_sign, &sig, hash.begin(), begin(), secp256k1_nonce_function_rfc6979, (counter || (!grind && test_case)) ? extra_entropy : nullptr)
    && !SigHasLowR(&sig) && grind) {
    WriteLE32(extra_entropy, ++counter);
}

to replace

https://github.com/bitcoin/bitcoin/blob/069c87957660adda97083fb77a48cbbcec253418/src/key.cpp#L214-L220

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not. It's kind of hard to read (at least to me).

assert(ret);
secp256k1_ecdsa_signature_serialize_der(secp256k1_context_sign, vchSig.data(), &nSigLen, &sig);
vchSig.resize(nSigLen);
Expand Down
2 changes: 1 addition & 1 deletion src/key.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ class CKey
* Create a DER-serialized signature.
* The test_case parameter tweaks the deterministic nonce.
*/
bool Sign(const uint256& hash, std::vector<unsigned char>& vchSig, uint32_t test_case = 0) const;
bool Sign(const uint256& hash, std::vector<unsigned char>& vchSig, bool grind = true, uint32_t test_case = 0) const;

/**
* Create a compact signature (65 bytes), which allows reconstructing the used public key.
Expand Down
22 changes: 13 additions & 9 deletions src/script/sign.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,22 +417,25 @@ class DummySignatureChecker final : public BaseSignatureChecker
const DummySignatureChecker DUMMY_CHECKER;

class DummySignatureCreator final : public BaseSignatureCreator {
private:
char m_r_len = 32;
char m_s_len = 32;
public:
DummySignatureCreator() {}
DummySignatureCreator(char r_len, char s_len) : m_r_len(r_len), m_s_len(s_len) {}
const BaseSignatureChecker& Checker() const override { return DUMMY_CHECKER; }
bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override
{
// Create a dummy signature that is a valid DER-encoding
vchSig.assign(72, '\000');
vchSig.assign(m_r_len + m_s_len + 7, '\000');
vchSig[0] = 0x30;
vchSig[1] = 69;
vchSig[1] = m_r_len + m_s_len + 4;
vchSig[2] = 0x02;
vchSig[3] = 33;
vchSig[3] = m_r_len;
vchSig[4] = 0x01;
vchSig[4 + 33] = 0x02;
vchSig[5 + 33] = 32;
vchSig[6 + 33] = 0x01;
vchSig[6 + 33 + 32] = SIGHASH_ALL;
vchSig[4 + m_r_len] = 0x02;
vchSig[5 + m_r_len] = m_s_len;
vchSig[6 + m_r_len] = 0x01;
vchSig[6 + m_r_len + m_s_len] = SIGHASH_ALL;
return true;
}
};
Expand All @@ -450,7 +453,8 @@ bool LookupHelper(const M& map, const K& key, V& value)

}

const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR = DummySignatureCreator();
const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR = DummySignatureCreator(32, 32);
const BaseSignatureCreator& DUMMY_MAXIMUM_SIGNATURE_CREATOR = DummySignatureCreator(33, 32);
const SigningProvider& DUMMY_SIGNING_PROVIDER = SigningProvider();

bool IsSolvable(const SigningProvider& provider, const CScript& script)
Expand Down
4 changes: 3 additions & 1 deletion src/script/sign.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,10 @@ class MutableTransactionSignatureCreator : public BaseSignatureCreator {
bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override;
};

/** A signature creator that just produces 72-byte empty signatures. */
/** A signature creator that just produces 71-byte empty signatures. */
extern const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR;
/** A signature creator that just produces 72-byte empty signatures. */
extern const BaseSignatureCreator& DUMMY_MAXIMUM_SIGNATURE_CREATOR;

typedef std::pair<CPubKey, std::vector<unsigned char>> SigPair;

Expand Down
36 changes: 36 additions & 0 deletions src/test/key_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,40 @@ BOOST_AUTO_TEST_CASE(key_test1)
BOOST_CHECK(detsigc == ParseHex("2052d8a32079c11e79db95af63bb9600c5b04f21a9ca33dc129c2bfa8ac9dc1cd561d8ae5e0f6c1a16bde3719c64c2fd70e404b6428ab9a69566962e8771b5944d"));
}

BOOST_AUTO_TEST_CASE(key_signature_tests)
{
// When entropy is specified, we should see at least one high R signature within 20 signatures
CKey key = DecodeSecret(strSecret1);
std::string msg = "A message to be signed";
uint256 msg_hash = Hash(msg.begin(), msg.end());
std::vector<unsigned char> sig;
bool found = false;

for (int i = 1; i <=20; ++i) {
sig.clear();
key.Sign(msg_hash, sig, false, i);
found = sig[3] == 0x21 && sig[4] == 0x00;
if (found) {
break;
}
}
BOOST_CHECK(found);

// When entropy is not specified, we should always see low R signatures that are less than 70 bytes in 256 tries
// We should see at least one signature that is less than 70 bytes.
found = true;
bool found_small = false;
for (int i = 0; i < 256; ++i) {
sig.clear();
std::string msg = "A message to be signed" + std::to_string(i);
msg_hash = Hash(msg.begin(), msg.end());
key.Sign(msg_hash, sig);
found = sig[3] == 0x20;
BOOST_CHECK(sig.size() <= 70);
found_small |= sig.size() < 70;
}
BOOST_CHECK(found);
BOOST_CHECK(found_small);
}

BOOST_AUTO_TEST_SUITE_END()
2 changes: 1 addition & 1 deletion src/test/script_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ class TestBuilder
std::vector<unsigned char> vchSig, r, s;
uint32_t iter = 0;
do {
key.Sign(hash, vchSig, iter++);
key.Sign(hash, vchSig, false, iter++);
if ((lenS == 33) != (vchSig[5 + vchSig[3]] == 33)) {
NegateSignatureS(vchSig);
}
Expand Down
33 changes: 16 additions & 17 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1540,30 +1540,29 @@ int64_t CWalletTx::GetTxTime() const
return n ? n : nTimeReceived;
}

// Helper for producing a max-sized low-S signature (eg 72 bytes)
bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout) const
// Helper for producing a max-sized low-S low-R signature (eg 71 bytes)
// or a max-sized low-S signature (e.g. 72 bytes) if use_max_sig is true
bool CWallet::DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig) const
{
// Fill in dummy signatures for fee calculation.
const CScript& scriptPubKey = txout.scriptPubKey;
SignatureData sigdata;

if (!ProduceSignature(*this, DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigdata))
{
if (!ProduceSignature(*this, use_max_sig ? DUMMY_MAXIMUM_SIGNATURE_CREATOR : DUMMY_SIGNATURE_CREATOR, scriptPubKey, sigdata)) {
return false;
} else {
UpdateInput(tx_in, sigdata);
}
UpdateInput(tx_in, sigdata);
return true;
}

// Helper for producing a bunch of max-sized low-S signatures (eg 72 bytes)
bool CWallet::DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut> &txouts) const
// Helper for producing a bunch of max-sized low-S low-R signatures (eg 71 bytes)
bool CWallet::DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut> &txouts, bool use_max_sig) const
{
// Fill in dummy signatures for fee calculation.
int nIn = 0;
for (const auto& txout : txouts)
{
if (!DummySignInput(txNew.vin[nIn], txout)) {
if (!DummySignInput(txNew.vin[nIn], txout, use_max_sig)) {
return false;
}

Expand All @@ -1572,7 +1571,7 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut>
return true;
}

int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet)
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)
{
std::vector<CTxOut> txouts;
// Look up the inputs. We should have already checked that this transaction
Expand All @@ -1586,26 +1585,26 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
assert(input.prevout.n < mi->second.tx->vout.size());
txouts.emplace_back(mi->second.tx->vout[input.prevout.n]);
}
return CalculateMaximumSignedTxSize(tx, wallet, txouts);
return CalculateMaximumSignedTxSize(tx, wallet, txouts, use_max_sig);
}

// txouts needs to be in the order of tx.vin
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts)
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)
{
CMutableTransaction txNew(tx);
if (!wallet->DummySignTx(txNew, txouts)) {
if (!wallet->DummySignTx(txNew, txouts, use_max_sig)) {
// This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)
// implies that we can sign for every input.
return -1;
}
return GetVirtualTransactionSize(txNew);
}

int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet)
int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* wallet, bool use_max_sig)
{
CMutableTransaction txn;
txn.vin.push_back(CTxIn(COutPoint()));
if (!wallet->DummySignInput(txn.vin[0], txout)) {
if (!wallet->DummySignInput(txn.vin[0], txout, use_max_sig)) {
// This should never happen, because IsAllFromMe(ISMINE_SPENDABLE)
// implies that we can sign for every input.
return -1;
Expand Down Expand Up @@ -2332,7 +2331,7 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const
bool solvable = IsSolvable(*this, pcoin->tx->vout[i].scriptPubKey);
bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable));

vCoins.push_back(COutput(pcoin, i, nDepth, spendable, solvable, safeTx));
vCoins.push_back(COutput(pcoin, i, nDepth, spendable, solvable, safeTx, (coinControl && coinControl->fAllowWatchOnly)));

// Checks the sum amount of all UTXO's.
if (nMinimumSumAmount != MAX_MONEY) {
Expand Down Expand Up @@ -2881,7 +2880,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
txNew.vin.push_back(CTxIn(coin.outpoint,CScript()));
}

nBytes = CalculateMaximumSignedTxSize(txNew, this);
nBytes = CalculateMaximumSignedTxSize(txNew, this, coin_control.fAllowWatchOnly);
if (nBytes < 0) {
strFailReason = _("Signing transaction failed");
return false;
Expand Down
29 changes: 16 additions & 13 deletions src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ class CMerkleTx
};

//Get the marginal bytes of spending the specified output
int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet);
int CalculateMaximumSignedInputSize(const CTxOut& txout, const CWallet* pwallet, bool use_max_sig = false);

/**
* A transaction with a bunch of additional info that only the owner cares about.
Expand Down Expand Up @@ -461,9 +461,9 @@ class CWalletTx : public CMerkleTx
CAmount GetChange() const;

// Get the marginal bytes if spending the specified output from this transaction
int GetSpendSize(unsigned int out) const
int GetSpendSize(unsigned int out, bool use_max_sig = false) const
{
return CalculateMaximumSignedInputSize(tx->vout[out], pwallet);
return CalculateMaximumSignedInputSize(tx->vout[out], pwallet, use_max_sig);
}

void GetAmounts(std::list<COutputEntry>& listReceived,
Expand Down Expand Up @@ -507,20 +507,23 @@ class COutput
/** Whether we know how to spend this output, ignoring the lack of keys */
bool fSolvable;

/** Whether to use the maximum sized, 72 byte signature when calculating the size of the input spend. This should only be set when watch-only outputs are allowed */
bool use_max_sig;

/**
* Whether this output is considered safe to spend. Unconfirmed transactions
* from outside keys and unconfirmed replacement transactions are considered
* unsafe and will not be used to fund new spending transactions.
*/
bool fSafe;

COutput(const CWalletTx *txIn, int iIn, int nDepthIn, bool fSpendableIn, bool fSolvableIn, bool fSafeIn)
COutput(const CWalletTx *txIn, int iIn, int nDepthIn, bool fSpendableIn, bool fSolvableIn, bool fSafeIn, bool use_max_sig_in = false)
{
tx = txIn; i = iIn; nDepth = nDepthIn; fSpendable = fSpendableIn; fSolvable = fSolvableIn; fSafe = fSafeIn; nInputBytes = -1;
tx = txIn; i = iIn; nDepth = nDepthIn; fSpendable = fSpendableIn; fSolvable = fSolvableIn; fSafe = fSafeIn; nInputBytes = -1; use_max_sig = use_max_sig_in;
// If known and signable by the given wallet, compute nInputBytes
// Failure will keep this value -1
if (fSpendable && tx) {
nInputBytes = tx->GetSpendSize(i);
nInputBytes = tx->GetSpendSize(i, use_max_sig);
}
}

Expand Down Expand Up @@ -976,14 +979,14 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
void ListAccountCreditDebit(const std::string& strAccount, std::list<CAccountingEntry>& entries);
bool AddAccountingEntry(const CAccountingEntry&);
bool AddAccountingEntry(const CAccountingEntry&, WalletBatch *batch);
bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts) const
bool DummySignTx(CMutableTransaction &txNew, const std::set<CTxOut> &txouts, bool use_max_sig = false) const
{
std::vector<CTxOut> v_txouts(txouts.size());
std::copy(txouts.begin(), txouts.end(), v_txouts.begin());
return DummySignTx(txNew, v_txouts);
return DummySignTx(txNew, v_txouts, use_max_sig);
}
bool DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut> &txouts) const;
bool DummySignInput(CTxIn &tx_in, const CTxOut &txout) const;
bool DummySignTx(CMutableTransaction &txNew, const std::vector<CTxOut> &txouts, bool use_max_sig = false) const;
bool DummySignInput(CTxIn &tx_in, const CTxOut &txout, bool use_max_sig = false) const;

CFeeRate m_pay_tx_fee{DEFAULT_PAY_TX_FEE};
unsigned int m_confirm_target{DEFAULT_TX_CONFIRM_TARGET};
Expand Down Expand Up @@ -1308,9 +1311,9 @@ class WalletRescanReserver
};

// Calculate the size of the transaction assuming all signatures are max size
// Use DummySignatureCreator, which inserts 72 byte signatures everywhere.
// Use DummySignatureCreator, which inserts 71 byte signatures everywhere.
// NOTE: this requires that all inputs must be in mapWallet (eg the tx should
// be IsAllFromMe).
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet);
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts);
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig = false);
int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig = false);
#endif // BITCOIN_WALLET_WALLET_H