sanitise untrusted error bytes before logging it

full-blown paranoia kicking in

electrum/base_crash_reporter.py

@@ -30,7 +30,7 @@ from typing import NamedTuple, Optional
 from .version import ELECTRUM_VERSION
 from . import constants
 from .i18n import _
-from .util import make_aiohttp_session
+from .util import make_aiohttp_session, error_text_str_to_safe_str
 from .logging import describe_os_version, Logger, get_git_version
 
 
@@ -80,7 +80,8 @@ class BaseCrashReporter(Logger):
         report = json.dumps(report)
         coro = self.do_post(proxy, BaseCrashReporter.report_server + "/crash.json", data=report)
         response = asyncio.run_coroutine_threadsafe(coro, asyncio_loop).result(timeout)
-        self.logger.info(f"Crash report sent. Got response [DO NOT TRUST THIS MESSAGE]: {response!r}")
+        self.logger.info(
+            f"Crash report sent. Got response [DO NOT TRUST THIS MESSAGE]: {error_text_str_to_safe_str(response)}")
         response = json.loads(response)
         assert isinstance(response, dict), type(response)
         # sanitize URL
@@ -98,7 +99,7 @@ class BaseCrashReporter(Logger):
         )
         return ret
 
-    async def do_post(self, proxy, url, data):
+    async def do_post(self, proxy, url, data) -> str:
         async with make_aiohttp_session(proxy) as session:
             async with session.post(url, data=data, raise_for_status=True) as resp:
                 return await resp.text()

electrum/lnpeer.py

@@ -22,7 +22,7 @@ from . import ecc
 from .ecc import sig_string_from_r_and_s, der_sig_from_sig_string
 from . import constants
 from .util import (bfh, log_exceptions, ignore_exceptions, chunks, OldTaskGroup,
-                   UnrelatedTransactionException)
+                   UnrelatedTransactionException, error_text_bytes_to_safe_str)
 from . import transaction
 from .bitcoin import make_op_return
 from .transaction import PartialTxOutput, match_script_against_template, Sighash
@@ -241,13 +241,14 @@ class Peer(Logger):
 
     def on_warning(self, payload):
         chan_id = payload.get("channel_id")
+        err_bytes = payload['data']
         self.logger.info(f"remote peer sent warning [DO NOT TRUST THIS MESSAGE]: "
-                         f"{payload['data'].decode('ascii')}. chan_id={chan_id.hex()}")
+                         f"{error_text_bytes_to_safe_str(err_bytes)}. chan_id={chan_id.hex()}")
         if chan_id in self.channels:
-            self.ordered_message_queues[chan_id].put_nowait((None, {'warning': payload['data']}))
+            self.ordered_message_queues[chan_id].put_nowait((None, {'warning': err_bytes}))
         elif chan_id in self.temp_id_to_id:
             chan_id = self.temp_id_to_id[chan_id] or chan_id
-            self.ordered_message_queues[chan_id].put_nowait((None, {'warning': payload['data']}))
+            self.ordered_message_queues[chan_id].put_nowait((None, {'warning': err_bytes}))
         else:
             # if no existing channel is referred to by channel_id:
             # - MUST ignore the message.
@@ -256,20 +257,21 @@ class Peer(Logger):
 
     def on_error(self, payload):
         chan_id = payload.get("channel_id")
+        err_bytes = payload['data']
         self.logger.info(f"remote peer sent error [DO NOT TRUST THIS MESSAGE]: "
-                         f"{payload['data'].decode('ascii')}. chan_id={chan_id.hex()}")
+                         f"{error_text_bytes_to_safe_str(err_bytes)}. chan_id={chan_id.hex()}")
         if chan_id in self.channels:
             self.schedule_force_closing(chan_id)
-            self.ordered_message_queues[chan_id].put_nowait((None, {'error': payload['data']}))
+            self.ordered_message_queues[chan_id].put_nowait((None, {'error': err_bytes}))
         elif chan_id in self.temp_id_to_id:
             chan_id = self.temp_id_to_id[chan_id] or chan_id
-            self.ordered_message_queues[chan_id].put_nowait((None, {'error': payload['data']}))
+            self.ordered_message_queues[chan_id].put_nowait((None, {'error': err_bytes}))
         elif chan_id == bytes(32):
             # if channel_id is all zero:
             # - MUST fail all channels with the sending node.
             for cid in self.channels:
                 self.schedule_force_closing(cid)
-                self.ordered_message_queues[cid].put_nowait((None, {'error': payload['data']}))
+                self.ordered_message_queues[cid].put_nowait((None, {'error': err_bytes}))
         else:
             # if no existing channel is referred to by channel_id:
             # - MUST ignore the message.
@@ -337,12 +339,11 @@ class Peer(Logger):
         q = self.ordered_message_queues[channel_id]
         name, payload = await asyncio.wait_for(q.get(), LN_P2P_NETWORK_TIMEOUT)
         # raise exceptions for errors/warnings, so that the caller sees them
-        if payload.get('error'):
+        if (err_bytes := (payload.get("error") or payload.get("warning"))) is not None:
+            err_type = "error" if payload.get("error") else "warning"
+            err_text = error_text_bytes_to_safe_str(err_bytes)
             raise GracefulDisconnect(
-                f"remote peer sent error [DO NOT TRUST THIS MESSAGE]: {payload['error'].decode('ascii')}")
-        elif payload.get('warning'):
-            raise GracefulDisconnect(
-                f"remote peer sent warning [DO NOT TRUST THIS MESSAGE]: {payload['warning'].decode('ascii')}")
+                f"remote peer sent {err_type} [DO NOT TRUST THIS MESSAGE]: {err_text}")
         if name != expected_name:
             raise Exception(f"Received unexpected '{name}'")
         return payload

electrum/network.py

@@ -46,7 +46,7 @@ from . import util
 from .util import (log_exceptions, ignore_exceptions, OldTaskGroup,
                    bfh, make_aiohttp_session, send_exception_to_crash_reporter,
                    is_hash256_str, is_non_negative_integer, MyEncoder, NetworkRetryManager,
-                   nullcontext)
+                   nullcontext, error_text_str_to_safe_str)
 from .bitcoin import COIN
 from . import constants
 from . import blockchain
@@ -235,8 +235,9 @@ class UntrustedServerReturnedError(NetworkException):
         return _("The server returned an error.")
 
     def __repr__(self):
+        e = self.original_exception
         return (f"<UntrustedServerReturnedError "
-                f"[DO NOT TRUST THIS MESSAGE] original_exception: {repr(self.original_exception)}>")
+                f"[DO NOT TRUST THIS MESSAGE] original_exception: {error_text_str_to_safe_str(repr(e))}>")
 
 
 _INSTANCE = None
@@ -924,14 +925,15 @@ class Network(Logger, NetworkRetryManager[ServerAddr]):
         except (RequestTimedOut, asyncio.CancelledError, asyncio.TimeoutError):
             raise  # pass-through
         except aiorpcx.jsonrpc.CodeMessageError as e:
-            self.logger.info(f"broadcast_transaction error [DO NOT TRUST THIS MESSAGE]: {repr(e)}")
+            self.logger.info(f"broadcast_transaction error [DO NOT TRUST THIS MESSAGE]: {error_text_str_to_safe_str(repr(e))}")
             raise TxBroadcastServerReturnedError(self.sanitize_tx_broadcast_response(e.message)) from e
         except BaseException as e:  # intentional BaseException for sanity!
-            self.logger.info(f"broadcast_transaction error2 [DO NOT TRUST THIS MESSAGE]: {repr(e)}")
+            self.logger.info(f"broadcast_transaction error2 [DO NOT TRUST THIS MESSAGE]: {error_text_str_to_safe_str(repr(e))}")
             send_exception_to_crash_reporter(e)
             raise TxBroadcastUnknownError() from e
         if out != tx.txid():
-            self.logger.info(f"unexpected txid for broadcast_transaction [DO NOT TRUST THIS MESSAGE]: {out} != {tx.txid()}")
+            self.logger.info(f"unexpected txid for broadcast_transaction [DO NOT TRUST THIS MESSAGE]: "
+                             f"{error_text_str_to_safe_str(out)} != {tx.txid()}")
             raise TxBroadcastHashMismatch(_("Server returned unexpected transaction ID."))
 
     async def try_broadcasting(self, tx, name) -> bool:

electrum/paymentrequest.py

@@ -39,7 +39,7 @@ except ImportError:
     sys.exit("Error: could not find paymentrequest_pb2.py. Create it with 'contrib/generate_payreqpb2.sh'")
 
 from . import bitcoin, constants, ecc, util, transaction, x509, rsakey
-from .util import bfh, make_aiohttp_session
+from .util import bfh, make_aiohttp_session, error_text_bytes_to_safe_str
 from .invoices import Invoice, get_id_from_onchain_outputs
 from .crypto import sha256
 from .bitcoin import address_to_script
@@ -94,12 +94,8 @@ async def get_payment_request(url: str) -> 'PaymentRequest':
             if isinstance(e, aiohttp.ClientResponseError):
                 error += f"\nGot HTTP status code {e.status}."
                 if resp_content:
-                    try:
-                        error_text_received = resp_content.decode("utf8")
-                    except UnicodeDecodeError:
-                        error_text_received = "(failed to decode error)"
-                    else:
-                        error_text_received = error_text_received[:400]
+                    error_text_received = error_text_bytes_to_safe_str(resp_content)
+                    error_text_received = error_text_received[:400]
                     error_oneline = ' -- '.join(error.split('\n'))
                     _logger.info(f"{error_oneline} -- [DO NOT TRUST THIS MESSAGE] "
                                  f"{repr(e)} text: {error_text_received}")
@@ -306,12 +302,8 @@ class PaymentRequest:
             if isinstance(e, aiohttp.ClientResponseError):
                 error += f"\nGot HTTP status code {e.status}."
                 if resp_content:
-                    try:
-                        error_text_received = resp_content.decode("utf8")
-                    except UnicodeDecodeError:
-                        error_text_received = "(failed to decode error)"
-                    else:
-                        error_text_received = error_text_received[:400]
+                    error_text_received = error_text_bytes_to_safe_str(resp_content)
+                    error_text_received = error_text_received[:400]
                     error_oneline = ' -- '.join(error.split('\n'))
                     _logger.info(f"{error_oneline} -- [DO NOT TRUST THIS MESSAGE] "
                                  f"{repr(e)} text: {error_text_received}")

electrum/tests/test_util.py

@@ -344,3 +344,26 @@ class TestUtil(ElectrumTestCase):
         self.assertFalse(util.is_subpath("/a/b/c", "c"))
         self.assertFalse(util.is_subpath("a", "/a/b/c"))
         self.assertFalse(util.is_subpath("c", "/a/b/c"))
+
+    def test_error_text_bytes_to_safe_str(self):
+        # ascii
+        self.assertEqual("'test'", util.error_text_bytes_to_safe_str(b"test"))
+        self.assertEqual('"test123 \'QWE"', util.error_text_bytes_to_safe_str(b"test123 'QWE"))
+        self.assertEqual("'prefix: \\x08\\x08\\x08\\x08\\x08\\x08\\x08\\x08malicious_stuff'",
+                         util.error_text_bytes_to_safe_str(b"prefix: " + 8 * b"\x08" + b"malicious_stuff"))
+        # unicode
+        self.assertEqual("'here is some unicode: \\\\xe2\\\\x82\\\\xbf \\\\xf0\\\\x9f\\\\x98\\\\x80 \\\\xf0\\\\x9f\\\\x98\\\\x88'",
+                         util.error_text_bytes_to_safe_str(b'here is some unicode: \xe2\x82\xbf \xf0\x9f\x98\x80 \xf0\x9f\x98\x88'))
+        # not even unicode
+        self.assertEqual("""\'\\x00\\x01\\x02\\x03\\x04\\x05\\x06\\x07\\x08\\t\\n\\x0b\\x0c\\r\\x0e\\x0f\\x10\\x11\\x12\\x13\\x14\\x15\\x16\\x17\\x18\\x19\\x1a\\x1b\\x1c\\x1d\\x1e\\x1f !"#$%&\\\'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\\x7f\\\\x80\\\\x81\\\\x82\\\\x83\\\\x84\\\\x85\\\\x86\\\\x87\\\\x88\\\\x89\\\\x8a\\\\x8b\\\\x8c\\\\x8d\\\\x8e\\\\x8f\\\\x90\\\\x91\\\\x92\\\\x93\\\\x94\\\\x95\\\\x96\\\\x97\\\\x98\\\\x99\\\\x9a\\\\x9b\\\\x9c\\\\x9d\\\\x9e\\\\x9f\\\\xa0\\\\xa1\\\\xa2\\\\xa3\\\\xa4\\\\xa5\\\\xa6\\\\xa7\\\\xa8\\\\xa9\\\\xaa\\\\xab\\\\xac\\\\xad\\\\xae\\\\xaf\\\\xb0\\\\xb1\\\\xb2\\\\xb3\\\\xb4\\\\xb5\\\\xb6\\\\xb7\\\\xb8\\\\xb9\\\\xba\\\\xbb\\\\xbc\\\\xbd\\\\xbe\\\\xbf\\\\xc0\\\\xc1\\\\xc2\\\\xc3\\\\xc4\\\\xc5\\\\xc6\\\\xc7\\\\xc8\\\\xc9\\\\xca\\\\xcb\\\\xcc\\\\xcd\\\\xce\\\\xcf\\\\xd0\\\\xd1\\\\xd2\\\\xd3\\\\xd4\\\\xd5\\\\xd6\\\\xd7\\\\xd8\\\\xd9\\\\xda\\\\xdb\\\\xdc\\\\xdd\\\\xde\\\\xdf\\\\xe0\\\\xe1\\\\xe2\\\\xe3\\\\xe4\\\\xe5\\\\xe6\\\\xe7\\\\xe8\\\\xe9\\\\xea\\\\xeb\\\\xec\\\\xed\\\\xee\\\\xef\\\\xf0\\\\xf1\\\\xf2\\\\xf3\\\\xf4\\\\xf5\\\\xf6\\\\xf7\\\\xf8\\\\xf9\\\\xfa\\\\xfb\\\\xfc\\\\xfd\\\\xfe\\\\xff\'""",
+                         util.error_text_bytes_to_safe_str(bytes(range(256))))
+
+    def test_error_text_str_to_safe_str(self):
+        # ascii
+        self.assertEqual("'test'", util.error_text_str_to_safe_str("test"))
+        self.assertEqual('"test123 \'QWE"', util.error_text_str_to_safe_str("test123 'QWE"))
+        self.assertEqual("'prefix: \\x08\\x08\\x08\\x08\\x08\\x08\\x08\\x08malicious_stuff'",
+                         util.error_text_str_to_safe_str("prefix: " + 8 * "\x08" + "malicious_stuff"))
+        # unicode
+        self.assertEqual("'here is some unicode: \\\\u20bf \\\\U0001f600 \\\\U0001f608'",
+                         util.error_text_str_to_safe_str("here is some unicode: ₿ 😀 😈"))

electrum/util.py

@@ -2059,3 +2059,26 @@ def get_running_loop() -> Optional[asyncio.AbstractEventLoop]:
         return asyncio.get_running_loop()
     except RuntimeError:
         return None
+
+
+def error_text_str_to_safe_str(err: str) -> str:
+    """Converts an untrusted error string to a sane printable ascii str.
+    Never raises.
+    """
+    return error_text_bytes_to_safe_str(err.encode("ascii", errors='backslashreplace'))
+
+
+def error_text_bytes_to_safe_str(err: bytes) -> str:
+    """Converts an untrusted error bytes text to a sane printable ascii str.
+    Never raises.
+
+    Note that naive ascii conversion would be insufficient. Fun stuff:
+    >>> b = b"my_long_prefix_blabla" + 21 * b"\x08" + b"malicious_stuff"
+    >>> s = b.decode("ascii")
+    >>> print(s)
+    malicious_stuffblabla
+    """
+    # convert to ascii, to get rid of unicode stuff
+    ascii_text = err.decode("ascii", errors='backslashreplace')
+    # do repr to handle ascii special chars (especially when printing/logging the str)
+    return repr(ascii_text)