From d61c6037eaf39c1d91ec6a4a344ba6f8418f67e0 Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 27 May 2024 17:09:45 +0000 Subject: [PATCH 1/2] ecc: add test that verify_usermessage does not enforce low-S rule --- electrum/ecc.py | 2 ++ tests/test_bitcoin.py | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/electrum/ecc.py b/electrum/ecc.py index a57075aae..882a31f9a 100644 --- a/electrum/ecc.py +++ b/electrum/ecc.py @@ -566,6 +566,8 @@ class ECPrivkey(ECPubkey): return sig64 def ecdsa_sign_recoverable(self, msg32: bytes, *, is_compressed: bool) -> bytes: + assert len(msg32) == 32, len(msg32) + def bruteforce_recid(sig64: bytes): for recid in range(4): sig65 = construct_ecdsa_sig65(sig64, recid, is_compressed=is_compressed) diff --git a/tests/test_bitcoin.py b/tests/test_bitcoin.py index be0b05330..df86cac95 100644 --- a/tests/test_bitcoin.py +++ b/tests/test_bitcoin.py @@ -246,6 +246,15 @@ class Test_bitcoin(ElectrumTestCase): self.assertFalse(ecc.verify_usermessage_with_address(addr1, b'wrong', msg1)) self.assertFalse(ecc.verify_usermessage_with_address(addr1, sig2, msg1)) + def test_signmessage_low_s(self): + """`$ bitcoin-cli verifymessage` does NOT enforce the low-S rule for ecdsa sigs. This tests we do the same.""" + addr = "15hETetDmcXm1mM4sEf7U2KXC9hDHFMSzz" + sig_low_s = b'Hzsu0U/THAsPz/MSuXGBKSULz2dTfmrg1NsAhFp+wH5aKfmX4Db7ExLGa7FGn0m6Mf43KsbEOWpvUUUBTM3Uusw=' + sig_high_s = b'IDsu0U/THAsPz/MSuXGBKSULz2dTfmrg1NsAhFp+wH5a1gZoH8kE7O05lE65YLZFzLx3sh/rDzXMbo1dQAJhhnU=' + msg = b'Chancellor on brink of second bailout for banks' + self.assertTrue(ecc.verify_usermessage_with_address(address=addr, sig65=base64.b64decode(sig_low_s), message=msg)) + self.assertTrue(ecc.verify_usermessage_with_address(address=addr, sig65=base64.b64decode(sig_high_s), message=msg)) + def test_signmessage_segwit_witness_v0_address(self): msg = b'Electrum' # p2wpkh-p2sh From 07c80d2ca14b54f2c41c81fbcbaa8b7e4b3e42ef Mon Sep 17 00:00:00 2001 From: SomberNight Date: Mon, 27 May 2024 17:12:33 +0000 Subject: [PATCH 2/2] ecc: ecdsa_verify to enforce low-S rule The low-S rule for ecdsa signatures is mandated by Bitcoin Core policy/standardness (though not by consensus). If we get signatures from untrusted sources, we should mandate they obey the policy rules. (e.g. from LN peers) Note that we normalize the signatures in the sig format conversion methods (DER <-> (r,s) <-> sig64). The BOLTs treat high-S signatures as invalid, and this changes our behaviour to that. (previously we would silently normalize the S value) see https://github.com/bitcoin/bitcoin/pull/6769 see https://github.com/lightning/bolts/pull/807 --- electrum/ecc.py | 14 +++++++++++--- tests/test_ecc.py | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/electrum/ecc.py b/electrum/ecc.py index 882a31f9a..4c86cfd8d 100644 --- a/electrum/ecc.py +++ b/electrum/ecc.py @@ -342,7 +342,13 @@ class ECPubkey(object): # check message return self.ecdsa_verify(sig65[1:], msg32) - def ecdsa_verify(self, sig64: bytes, msg32: bytes) -> bool: + def ecdsa_verify( + self, + sig64: bytes, + msg32: bytes, + *, + enforce_low_s: bool = True, # policy/standardness rule + ) -> bool: assert_bytes(sig64) if len(sig64) != 64: return False @@ -353,7 +359,8 @@ class ECPubkey(object): ret = _libsecp256k1.secp256k1_ecdsa_signature_parse_compact(_libsecp256k1.ctx, sig, sig64) if 1 != ret: return False - ret = _libsecp256k1.secp256k1_ecdsa_signature_normalize(_libsecp256k1.ctx, sig, sig) + if not enforce_low_s: + ret = _libsecp256k1.secp256k1_ecdsa_signature_normalize(_libsecp256k1.ctx, sig, sig) pubkey = self._to_libsecp256k1_pubkey_ptr() if 1 != _libsecp256k1.secp256k1_ecdsa_verify(_libsecp256k1.ctx, sig, msg32, pubkey): @@ -439,7 +446,8 @@ def verify_usermessage_with_address(address: str, sig65: bytes, message: bytes, else: return False # check message - return public_key.ecdsa_verify(sig65[1:], h) + # note: `$ bitcoin-cli verifymessage` does NOT enforce the low-S rule for ecdsa sigs + return public_key.ecdsa_verify(sig65[1:], h, enforce_low_s=False) def is_secret_within_curve_range(secret: Union[int, bytes]) -> bool: diff --git a/tests/test_ecc.py b/tests/test_ecc.py index 8197a8449..d9f0c129d 100644 --- a/tests/test_ecc.py +++ b/tests/test_ecc.py @@ -116,3 +116,27 @@ class TestSchnorr(ElectrumTestCase): for tag, msg in data: self.assertEqual(bip340_tagged_hash__from_libsecp(tag, msg), ecc.bip340_tagged_hash(tag, msg)) + + +class TestEcdsa(ElectrumTestCase): + + def test_verify_enforces_low_s(self): + # privkey = ecc.ECPrivkey(bytes.fromhex("d473e2ec218dca8e3508798f01cdfde0135fc79d95526b12e3537fe57e479ac1")) + # r, low_s = privkey.ecdsa_sign(msg32, sigencode=lambda x, y: (x,y)) + # pubkey = ecc.ECPubkey(privkey.get_public_key_bytes()) + pubkey = ecc.ECPubkey(bytes.fromhex("03befe4f7c92eaed73fb8eddac28c6191c87c6a3546bf8dc09643e1e10bc6f5ab0")) + msg32 = sha256("hello there") + r = 29658118546717807188148256874354333643324863178937517286987684851194094232509 + # low-S + low_s = 9695211969150896589566136599751503273246834163278279637071703776634378000266 + sig64_low_s = ( + int.to_bytes(r, length=32, byteorder="big") + + int.to_bytes(low_s, length=32, byteorder="big")) + self.assertTrue(pubkey.ecdsa_verify(sig64_low_s, msg32)) + # high-S + high_s = ecc.CURVE_ORDER - low_s + sig64_high_s = ( + int.to_bytes(r, length=32, byteorder="big") + + int.to_bytes(high_s, length=32, byteorder="big")) + self.assertFalse(pubkey.ecdsa_verify(sig64_high_s, msg32)) + self.assertTrue(pubkey.ecdsa_verify(sig64_high_s, msg32, enforce_low_s=False))