From 0e392d1f2aa3dc60ade825a794e9de28d8d88f7d Mon Sep 17 00:00:00 2001
From: Sergi Delgado Segura <sergi.delgado.s@gmail.com>
Date: Wed, 1 Apr 2020 17:04:33 +0200
Subject: [PATCH] Cryptographer.sigrec_decode assumed signatures where properly
 encoded.

- Fixes a bug in sigrec_decode where the decoding function assumend that the first by was formatted as 31 + SigRec. Non properly encoded signatures made the function to crash due to an overflow (int_to_bytes(x) for negative x)
---
 common/cryptographer.py                | 18 +++++++++++++-----
 test/common/unit/test_cryptographer.py |  9 +++++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/common/cryptographer.py b/common/cryptographer.py
index ebe15ac..e7d4fac 100644
--- a/common/cryptographer.py
+++ b/common/cryptographer.py
@@ -43,6 +43,7 @@ def hash_160(message):
     return h160
 
 
+# NOTCOVERED
 def sigrec_encode(rsig_rid):
     """
     Encodes a pk-recoverable signature to be used in LN. ```rsig_rid`` can be obtained trough
@@ -63,6 +64,7 @@ def sigrec_encode(rsig_rid):
     return sigrec
 
 
+# NOTCOVERED
 def sigrec_decode(sigrec):
     """
     Decodes a pk-recoverable signature in the format used by LN to be input to ``PublicKey.from_signature_and_message``.
@@ -72,12 +74,18 @@ def sigrec_decode(sigrec):
 
     Returns:
         :obj:`bytes`: the decoded signature.
+
+    Raises:
+        :obj:`ValueError`: if the SigRec is not properly encoded (first byte is not 31 + recovery id)
     """
 
-    rid, rsig = int_to_bytes(sigrec[0] - 31), sigrec[1:]
-    rsig_rid = rsig + rid
+    int_rid, rsig = sigrec[0] - 31, sigrec[1:]
+    if int_rid < 0:
+        raise ValueError("Wrong SigRec")
+    else:
+        rid = int_to_bytes(int_rid)
 
-    return rsig_rid
+    return rsig + rid
 
 
 # FIXME: Common has not log file, so it needs to log in the same log as the caller. This is a temporary fix.
@@ -299,9 +307,9 @@ class Cryptographer:
             return None
 
         sigrec = pyzbase32.decode_bytes(zb32_sig)
-        rsig_recid = sigrec_decode(sigrec)
 
         try:
+            rsig_recid = sigrec_decode(sigrec)
             pk = PublicKey.from_signature_and_message(rsig_recid, LN_MESSAGE_PREFIX + message, hasher=sha256d)
             return pk
 
@@ -313,7 +321,7 @@ class Cryptographer:
 
         except Exception as e:
             if "failed to recover ECDSA public key" in str(e):
-                logger.error("Cannot recover public key from signature".format(type(rsig_recid)))
+                logger.error("Cannot recover public key from signature")
             else:
                 logger.error("Unknown exception", error=str(e))
 
diff --git a/test/common/unit/test_cryptographer.py b/test/common/unit/test_cryptographer.py
index ddbb707..bb60125 100644
--- a/test/common/unit/test_cryptographer.py
+++ b/test/common/unit/test_cryptographer.py
@@ -208,6 +208,15 @@ def test_recover_pk():
     assert isinstance(rpk, PublicKey)
 
 
+def test_recover_pk_invalid_sigrec():
+    message = "Hey, it's me"
+    signature = "ddbfb019e4d56155b4175066c2b615ab765d317ae7996d188b4a5fae4cc394adf98fef46034d0553149392219ca6d37dca9abdfa6366a8e54b28f19d3e5efa8a14b556205dc7f33a"
+
+    # The given signature, when zbase32 decoded, has a fist byte with value lower than 31.
+    # The first byte of the signature should be 31 + SigRec, so this should fail
+    assert Cryptographer.recover_pk(message, signature) is None
+
+
 def test_recover_pk_ground_truth():
     # Use a message a signature generated by c-lightning and see if we recover the proper key
     message = b"Test message"