Repository: magic-wormhole/magic-wormhole (language: python)

Issue:
Noise limits leak out of the Dilation API
Currently, the 65KiB limit of Noise messages accidentally leaks out of the L2 abstractions. It should not.


Discussion / hints:

Fix patch:
diff --git a/NEWS.md b/NEWS.md
index ca0dfc74..93d25e5a 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -13,12 +13,15 @@ User-visible changes in "magic-wormhole":
 * SECURITY: Replace "weird" characters in receiver's display (#476)
 * Handle SSH keys with comments properly (#434)
 * Properly send `use-version` in Dilation
+* Split Noise messages invisibly to the application layer (#484)
 
 Thanks to the many contributors of bugs, patches, and other help with this
 release:
 
 * Jelle van der Waa aka @jelly
 * Matthias Riße https://github.com/matrss
+* meejah https://meejah.ca
+
 
 ## Release 0.12.0 (04-Apr-2020)
 
diff --git a/docs/dilation-protocol.md b/docs/dilation-protocol.md
index bd2a0b72..5c8e29ae 100644
--- a/docs/dilation-protocol.md
+++ b/docs/dilation-protocol.md
@@ -211,16 +211,24 @@ If the wrong handshake is received, the connection must be dropped.
 For debugging purposes, the node might want to keep looking at data beyond the first incorrect character and log a few hundred characters until the first newline.
 
 Everything beyond the last byte of the handshake consists of Noise protocol messages.
-Noise itself has a 65536-byte (`2**16`) limit on message sizes.
-However, the L2 protocol will accept any message up to an unsigned 4-byte integer in length (4.0 GiB or `2**32` bytes).
+
+### L2 Message Framing
+
+Noise itself has a 65535-byte (`2**16 - 1`) limit on encoded message sizes -- however the *payload* is 16 bytes smaller that this limit.
+The L2 protocol can deliver any *encoded* message up to an unsigned 4-byte integer in length (4.0 GiB or `2**32` bytes).
+Due to overhead, the actual limit for the payload of each frame is 4293918703 bytes (65537 Noise messages with 65519 bytes of payload each).
 
 The encoding works like this: there is a 4-byte big-endian length field, followed by some number of Noise packets.
 There is no leading length field on each Noise packet: implementations MUST respect the Noise limits.
-So if the length field indicates a message bigger than 65536, the reader pulls 65536 bytes out of the stream, decrypts that blob as a Noise message, subtracts 65536 from the total and continues.
-The last Noise message will obviously be less than 65536 bytes.
+So if the length field indicates a message bigger than 65535, the reader pulls 65535 bytes out of the stream, decrypts that blob as a Noise message, subtracts 65535 from the total and continues.
+The last Noise message will obviously be less than or exactly 65535 bytes.
 
 The entire decoded blob is then "one L2 message" and is delivered upstream.
 
+On the encoding side, note that 16 bytes of each maximum 65535-byte Noise message is used for authentication data.
+This means that when encoding _payload_, implementations pull at most 65519 bytes of plaintext at once and encrypt it (yielding 65535 bytes of ciphertext).
+Implementations should avoid sending enormous messages like this, but it is possible.
+
 The Noise cryptography uses the `NNpsk0` pattern with the Leader as the first party (`"-> psk, e"` in the Noise spec), and the Follower as the second (`"<- e, ee"`).
 The pre-shared-key is the "Dilation key", which is statically derived from the master PAKE key using HKDF.
 Each L2 connection uses the same Dilation key, but different ephemeral keys, so each gets a different session key.
@@ -256,6 +264,39 @@ After sending their KCM, the Follower will wait for either an empty KCM (at whic
 Other connections and/or listening sockets are stopped.
 
 
+### L2 Message Payload Encoding
+
+Above, we talk about *frames*.
+Inside each frame is a plaintext payload (of maximum 4293918703 bytes as above).
+These plaintexts are binary-encoded messages of the L2 protocol layer, consisting of these types with corresponding 1-byte tags:
+
+- KCM: `0x00`
+- PING: `0x01`
+- PONG: `0x02`
+- OPEN: `0x03`
+- DATA: `0x04`
+- CLOSE: `0x05`
+- ACK: `0x06`
+
+Every message starts with its tag.
+Following the tag is a message-specific encoding.
+In all messages, a "subchannel-id" (if present) is a 4-byte big-endian unsigned int.
+A "sequence-number" (if present) is a 4-byte big-endian unsigned int.
+
+The messages are encoded like this (after the tag):
+
+- KCM: no other data
+- PING: arbitrary 4 byte "ping id"
+- PONG: arbitrary 4 byte "ping id"
+- OPEN: subchannel-id, sequence-number
+- DATA: subchannel-id, sequence-number, data
+- CLOSE: subchannel-id, sequence-number
+- ACK: sequence-number
+
+For example, an OPEN would be encoded in 9 bytes of payload -- so the resulting Noise message is 9 + 16 bytes, surrounded by a frame with leading 4-byte size for 29 bytes.
+A DATA message is thus 9 bytes plus the actual "data payload" (when wrapped in Noise, and following the limits in the framing section, this means the absolute biggest single application message possible is 4293918703 - 9 or 4293918694 bytes).
+
+
 ### Python Implementation Details
 
 For developers attempting to understand the Python reference implementation (in the `wormhole._dilation` package):
diff --git a/src/wormhole/_dilation/_noise.py b/src/wormhole/_dilation/_noise.py
index 10052646..6f2dbfac 100644
--- a/src/wormhole/_dilation/_noise.py
+++ b/src/wormhole/_dilation/_noise.py
@@ -1,3 +1,6 @@
+NOISE_MAX_PAYLOAD = (2**16 - 1) - 16  # 65535 minus 16 bytes authentication data
+NOISE_MAX_CIPHERTEXT = (2**16 - 1)  # 65535
+
 try:
     from noise.exceptions import NoiseInvalidMessage
 except ImportError:
diff --git a/src/wormhole/_dilation/connection.py b/src/wormhole/_dilation/connection.py
index 15796759..2d4c08c8 100644
--- a/src/wormhole/_dilation/connection.py
+++ b/src/wormhole/_dilation/connection.py
@@ -12,7 +12,7 @@
 from ..observer import OneShotObserver
 from .encode import to_be4, from_be4
 from .roles import LEADER, FOLLOWER
-from ._noise import NoiseInvalidMessage, NoiseHandshakeError
+from ._noise import NoiseInvalidMessage, NoiseHandshakeError, NOISE_MAX_PAYLOAD, NOISE_MAX_CIPHERTEXT
 
 # InboundFraming is given data and returns Frames (Noise wire-side
 # bytestrings). It handles the relay handshake and the prologue. The Frames it
@@ -332,6 +332,7 @@ def _is_role(_record, _attr, value):
 class _Record(object):
     _framer = attrib(validator=provides(IFramer))
     _noise = attrib()
+
     _role = attrib(default="unspecified", validator=_is_role) # for debugging
 
     n = MethodicalMachine()
@@ -416,8 +417,20 @@ def process_handshake(self, frame):
 
     @n.output()
     def decrypt_message(self, frame):
+        # opposite of the encoding: if we have _more_ than what a
+        # single Noise packet can hold, we have to build up the real
+        # plaintext incrementally.
+        size = len(frame)
         try:
-            message = self._noise.decrypt(frame)
+            if size <= NOISE_MAX_CIPHERTEXT:
+                message = self._noise.decrypt(frame)
+            else:
+                start = 0
+                message = b""
+                while start < size:
+                    ciphertext = frame[start:start + NOISE_MAX_CIPHERTEXT]
+                    message += self._noise.decrypt(ciphertext)
+                    start += NOISE_MAX_CIPHERTEXT
         except NoiseInvalidMessage as e:
             # if this happens during tests, flunk the test
             log.err(e, "bad inbound noise frame")
@@ -455,7 +468,19 @@ def add_and_unframe(self, data):
 
     def send_record(self, r):
         message = encode_record(r)
-        frame = self._noise.encrypt(message)
+        if len(message) <= NOISE_MAX_PAYLOAD:
+            frame = self._noise.encrypt(message)
+        else:
+            # we want to put all the encrypted bytes into one "frame",
+            # but there are more bytes than we can fit in a Noise
+            # message .. so we chop them up
+            start = 0
+            frame = b""
+            while start < len(message):
+                this_msg = message[start:start + NOISE_MAX_PAYLOAD]
+                cip = self._noise.encrypt(this_msg)
+                frame += cip
+                start += NOISE_MAX_PAYLOAD
         self._framer.send_frame(frame)
 
 


Test patch:
diff --git a/src/wormhole/test/dilate/test_record.py b/src/wormhole/test/dilate/test_record.py
index a3278de8..33e48fc0 100644
--- a/src/wormhole/test/dilate/test_record.py
+++ b/src/wormhole/test/dilate/test_record.py
@@ -2,11 +2,14 @@
 from unittest import mock
 from zope.interface import alsoProvides
 from twisted.trial import unittest
+from twisted.internet.interfaces import ITransport
 from ..._dilation._noise import NoiseInvalidMessage
 from ..._dilation.connection import (IFramer, Frame, Prologue,
-                                     _Record, Handshake,
-                                     Disconnect, Ping)
-from ..._dilation.roles import LEADER
+                                     _Record, Handshake, KCM,
+                                     Disconnect, Ping, _Framer, Data)
+from ..._dilation.connector import build_noise
+from ..._dilation.roles import LEADER, FOLLOWER
+from zope.interface import implementer
 
 
 def make_record():
@@ -126,13 +129,13 @@ def test_send_record(self):
         f = mock.Mock()
         alsoProvides(f, IFramer)
         n = mock.Mock()
-        f1 = object()
+        f1 = b"some bytes"
         n.encrypt = mock.Mock(return_value=f1)
         r1 = Ping(b"pingid")
         r = _Record(f, n, LEADER)
         r.set_role_leader()
         self.assertEqual(f.mock_calls, [])
-        m1 = object()
+        m1 = b"some other bytes"
         with mock.patch("wormhole._dilation.connection.encode_record",
                         return_value=m1) as er:
             r.send_record(r1)
@@ -273,3 +276,120 @@ def test_good(self):
             self.assertEqual(pr.mock_calls, [mock.call(kcm), mock.call(msg1)])
         n.mock_calls[:] = []
         f.mock_calls[:] = []
+
+    def test_large_frame(self):
+        """
+        Noise only allows 64KiB message, but the API allows up to 4GiB
+        frames
+        """
+        # XXX could really benefit from some Hypothesis style
+        # exploration of more cases .. but we don't already depend on
+        # that library, so a future improvement
+
+        @implementer(ITransport)
+        class FakeTransport:
+            """
+            Record which write()s happen
+            """
+            def __init__(self):
+                self.data = []  # list of bytes
+
+            def write(self, data):
+                self.data.append(data)
+
+        # we build both sides of a connection so that underlying Noise
+        # structures can be set up and paired properly. Essentially
+        # this test is acting like the L2 Protocol object, and can
+        # feed bytes to / from either side
+        pake_secret = b"\x00" * 32
+        transport0 = FakeTransport()
+        transport1 = FakeTransport()
+        noise0 = build_noise()
+        noise0.set_psks(pake_secret)
+        noise0.set_as_initiator()  # leader
+        noise1 = build_noise()
+        noise1.set_psks(pake_secret)
+        noise1.set_as_responder()  # follower
+        framer0 = _Framer(transport0, b"out prolog", b"in prolog")
+        record0 = _Record(framer0, noise0, LEADER)
+        record0.set_role_leader()
+        record0.connectionMade()
+        self.assertEqual(
+            list(record0.add_and_unframe(b"in prolog")),
+            []
+        )
+
+        # note that in connector the prologues flip around depending
+        # on who is the leader or follower
+        framer1 = _Framer(transport1, b"in prolog", b"out prolog")
+        record1 = _Record(framer1, noise1, FOLLOWER)
+        record1.set_role_follower()
+        record1.connectionMade()
+
+        # the leader has now sent the prolog and the opening handshake
+        # message, so we consume them on the follower side
+
+        self.assertEqual(
+            list(record1.add_and_unframe(transport0.data[0])),  # b"out prolog"
+            []
+        )
+        self.assertEqual(
+            list(record1.add_and_unframe(transport0.data[1])),
+            [Handshake()]
+        )
+
+        # the follower sends the first KCM; the leader is waiting for
+        # this and will complete the handshake after that
+        record1.send_record(KCM())
+        self.assertEqual(
+            list(record0.add_and_unframe(transport1.data[1])),
+            [Handshake()]
+        )
+        record0.send_record(KCM())
+        self.assertEqual(
+            list(record1.add_and_unframe(transport0.data[2])),
+            [KCM()]
+        )
+        # both sides are now done their handshakes
+
+        # Now, we send a message that's definitely bigger than a
+        # single Noise message can deal with
+        input_plaintext = b"\xff" * 65537
+        record0.send_record(
+            Data(
+                seqnum=123,
+                scid=456,
+                data=input_plaintext,
+            )
+        )
+
+        # ...despite its size the message should still be sent out as
+        # a single Data, because _our_ framing handles 4-byte lengths
+        self.assertEqual(
+            len(transport0.data[3]),
+            4 + 65537 + (16 * 2) + 1 + 4 + 4
+        )
+        #   ^-- frame-length
+        #       ^-- plaintext size
+        #               ^-- Noise associated data, x2 noise packets
+        #                          ^-- "data" kind, 0x04
+        #                              ^-- subchannel-id
+        #                                  ^-- sequence number
+        #
+        # (maybe we don't need the above assert, since if any of that
+        # isn't true the next step will fail)
+
+        # round-trip the data to the other side and ensure we get the
+        # plaintext back
+        outputs = list(
+            record1.add_and_unframe(transport0.data[3])
+        )
+        self.assertEqual(len(outputs), 1)
+        self.assertEqual(
+            outputs[0],
+            Data(
+                seqnum=123,
+                scid=456,
+                data=input_plaintext,
+            )
+        )