Merge JoinMarket-Org/joinmarket-clientserver#1070: Validate message signature encoding

0507f6a117 Validate message signature encoding (Adam Gibson)

Pull request description:

  Fixes #1069.
  Before this commit, a non-hex encoded pubkey sent
  as part of the privmsg signature raised an Exception,
  this commit fixes this, checking the encoding of both
  the pubkey and signature fields before sending them
  from daemon to client.

ACKs for top commit:
  kristapsk:
    ACK 0507f6a117. Checked diff between commits, test suite passes, did successful signet coinjoin. Merging.

Tree-SHA512: d6b59fc340b015157544330a0a4624999559d9f2bcc5ee1ef189558bdab5fb2a2cf80287d1020bfe5be1f4919037b794a0fa4bbe2292a80aca0a03e9de23384a

jmdaemon/jmdaemon/message_channel.py

@@ -8,6 +8,7 @@ from jmdaemon import encrypt_encode, decode_decrypt, COMMAND_PREFIX,\
     encrypted_commands, commitment_broadcast_list, offername_list,\
     fidelity_bond_cmd_list
 from jmbase.support import get_log
+from jmbase import hextobin
 from functools import wraps
 
 log = get_log()
@@ -918,7 +919,6 @@ class MessageChannel(object):
         #Other ill formatted messages will be caught in the try block.
         if len(message) < 2:
             return
-
         if message[0] != COMMAND_PREFIX:
             log.debug('message not a cmd')
             return
@@ -926,16 +926,31 @@ class MessageChannel(object):
         if cmd_string not in plaintext_commands + encrypted_commands:
             log.debug('cmd not in cmd_list, line="' + message + '"')
             return
+        badsigmsg = "Sig not properly appended to privmsg, ignoring"
         #Verify nick ownership
-        sig = message[1:].split(' ')[-2:]
+        try:
+            pub, sig = message[1:].split(' ')[-2:]
+        except Exception:
+            log.debug(badsigmsg)
+            return
         #reconstruct original message without cmd
         rawmessage = ' '.join(message[1:].split(' ')[1:-2])
-        #sanity check that the sig was appended properly
-        if len(sig) != 2 or len(rawmessage) == 0:
-            log.debug("Sig not properly appended to privmsg, ignoring")
+        # can happen if not enough fields for command, (stuff), pub, sig:
+        if len(rawmessage) == 0:
+            log.debug(badsigmsg)
+            return
+        # Sanitising signature before attempting to verify:
+        # Note that the sig itself can be any garbage, because `ecdsa_verify`
+        # swallows any fail and returns False; but the pubkey is assumed
+        # to be hex-encoded, and the signature base64 encoded, so check early:
+        try:
+            dummypub = hextobin(pub)
+            dummysig = base64.b64decode(sig)
+        except Exception:
+            log.debug(badsigmsg)
             return
         self.daemon.request_signature_verify(
-            rawmessage + str(self.hostid), message, sig[1], sig[0], nick,
+            rawmessage + str(self.hostid), message, sig, pub, nick,
             NICK_HASH_LENGTH, NICK_MAX_ENCODED, str(self.hostid))
 
     def on_verified_privmsg(self, nick, message):

jmdaemon/test/test_message_channel.py

@@ -17,8 +17,11 @@ import traceback
 import threading
 import binascii
 import jmbitcoin as bitcoin
+from jmbase import bintohex
 from dummy_mc import DummyMessageChannel
 
+# note: completely invalid sig is fine, as long as it's encoded right:
+dummy_sig_str = bintohex(b"\x02"*33) + " " + base64.b64encode(b"\x01"*72).decode()
 
 jlog = get_log()
 
@@ -151,8 +154,8 @@ def test_setup_mc():
     #Simulate order receipt on 2 of 3 msgchans from this nick;
     #note that it will have its active chan set to mc "1" because that
     #is the last it was seen on:
-    dmcs[0].on_privmsg(cp1, "!reloffer 0 4000 5000 100 0.2 abc def")
-    dmcs[1].on_privmsg(cp1, "!reloffer 0 4000 5000 100 0.2 abc def")
+    dmcs[0].on_privmsg(cp1, "!reloffer 0 4000 5000 100 0.2 " + dummy_sig_str)
+    dmcs[1].on_privmsg(cp1, "!reloffer 0 4000 5000 100 0.2 " + dummy_sig_str)
     time.sleep(0.5)
     #send back a response
     mcc.privmsg(cp1, "fill", "0")
@@ -210,7 +213,7 @@ def test_setup_mc():
     #first, pretend they all showed up on all 3 mcs:
     for m in dmcs:
         for cp in cps:
-            m.on_privmsg(cp, "!reloffer 0 400000 500000 100 0.002 abc def")
+            m.on_privmsg(cp, "!reloffer 0 400000 500000 100 0.002 " + dummy_sig_str)
     #next, call main fill function
     mcc.fill_orders(new_offers, 1000, "dummypubkey", "dummycommit")
     #now send a dummy transaction to this same set.
@@ -242,7 +245,7 @@ def test_setup_mc():
     #have the cps rearrive
     for m in dmcs:
         for cp in cps:
-            m.on_privmsg(cp, "!reloffer 0 4000 5000 100 0.2 abc def")
+            m.on_privmsg(cp, "!reloffer 0 4000 5000 100 0.2 " + dummy_sig_str)
 
     #####################################################################
     #next series of messages are to test various normal and abnormal
@@ -255,7 +258,7 @@ def test_setup_mc():
     #invalid missing field
     dmcs[0].on_pubmsg(cps[2], "!hp2")
     #receive commitment via privmsg to trigger commitment_transferred
-    dmcs[0].on_privmsg(cps[2], "!hp2 deadbeef abc def")
+    dmcs[0].on_privmsg(cps[2], "!hp2 deadbeef " + dummy_sig_str)
     #simulate receipt of order cancellation
     #valid
     dmcs[0].on_pubmsg(cps[2], "!cancel 2")