Use cryptographically secure boundaries for multipart file uploads

stripe/_multipart_data_generator.py

@@ -1,13 +1,29 @@
-import random
 import io
+import secrets
 
 from stripe._encode import _api_encode
 
+# Number of random bytes used to build the boundary, matching stripe-ruby's
+# `SecureRandom.hex(30)` and Go's mime/multipart. This must come from a CSPRNG:
+# multipart/form-data is only safe if the delimiter cannot be guessed by anyone
+# able to influence the content, since a value containing the delimiter gets
+# parsed as additional parts.
+BOUNDARY_BYTES = 30
+
+
+def _escape_header_value(value: str) -> str:
+    """Make a value safe to interpolate into a part header.
+
+    An unescaped quote would end the quoted-string early, and CR/LF would
+    introduce additional header lines or parts.
+    """
+    return value.replace('"', "%22").replace("\r", " ").replace("\n", " ")
+
 
 class MultipartDataGenerator(object):
     data: io.BytesIO
     line_break: str
-    boundary: int
+    boundary: str
     chunk_size: int
 
     def __init__(self, chunk_size: int = 1028):
@@ -36,9 +52,9 @@ def add_params(self, params):
                     filename = str(value.name)
 
                 self._write('Content-Disposition: form-data; name="')
-                self._write(key)
+                self._write(_escape_header_value(key))
                 self._write('"; filename="')
-                self._write(filename)
+                self._write(_escape_header_value(filename))
                 self._write('"')
                 self._write(self.line_break)
                 self._write("Content-Type: application/octet-stream")
@@ -48,7 +64,7 @@ def add_params(self, params):
                 self._write_file(value)
             else:
                 self._write('Content-Disposition: form-data; name="')
-                self._write(key)
+                self._write(_escape_header_value(key))
                 self._write('"')
                 self._write(self.line_break)
                 self._write(self.line_break)
@@ -83,5 +99,5 @@ def _write_file(self, f):
                 break
             self._write(file_contents)
 
-    def _initialize_boundary(self):
-        return random.randint(0, 2**63)
+    def _initialize_boundary(self) -> str:
+        return secrets.token_hex(BOUNDARY_BYTES)

tests/api_resources/test_file.py

@@ -32,8 +32,14 @@ def test_is_retrievable(self, http_client_mock):
         )
         assert isinstance(resource, stripe.File)
 
-    def test_is_creatable(self, setup_upload_api_base, http_client_mock):
-        MultipartDataGenerator._initialize_boundary = lambda self: 1234567890
+    def test_is_creatable(
+        self, setup_upload_api_base, http_client_mock, monkeypatch
+    ):
+        monkeypatch.setattr(
+            MultipartDataGenerator,
+            "_initialize_boundary",
+            lambda self: "abc123",
+        )
         test_file = tempfile.TemporaryFile()
         resource = stripe.File.create(
             purpose="dispute_evidence",
@@ -44,7 +50,7 @@ def test_is_creatable(self, setup_upload_api_base, http_client_mock):
             "post",
             path="/v1/files",
             api_base=stripe.upload_api_base,
-            content_type="multipart/form-data; boundary=1234567890",
+            content_type="multipart/form-data; boundary=abc123",
         )
         assert isinstance(resource, stripe.File)
 

tests/api_resources/test_file_upload.py

@@ -33,8 +33,14 @@ def test_is_retrievable(self, http_client_mock):
         )
         assert isinstance(resource, File)
 
-    def test_is_creatable(self, setup_upload_api_base, http_client_mock):
-        MultipartDataGenerator._initialize_boundary = lambda self: 1234567890
+    def test_is_creatable(
+        self, setup_upload_api_base, http_client_mock, monkeypatch
+    ):
+        monkeypatch.setattr(
+            MultipartDataGenerator,
+            "_initialize_boundary",
+            lambda self: "abc123",
+        )
         test_file = tempfile.TemporaryFile()
         resource = File.create(
             purpose="dispute_evidence",
@@ -45,7 +51,7 @@ def test_is_creatable(self, setup_upload_api_base, http_client_mock):
             "post",
             api_base=stripe.upload_api_base,
             path="/v1/files",
-            content_type="multipart/form-data; boundary=1234567890",
+            content_type="multipart/form-data; boundary=abc123",
         )
         assert isinstance(resource, File)
 

tests/services/test_file_upload.py

@@ -28,8 +28,13 @@ def test_is_creatable(
         self,
         file_stripe_mock_stripe_client,
         http_client_mock,
+        monkeypatch,
     ):
-        MultipartDataGenerator._initialize_boundary = lambda self: 1234567890
+        monkeypatch.setattr(
+            MultipartDataGenerator,
+            "_initialize_boundary",
+            lambda self: "abc123",
+        )
         test_file = tempfile.TemporaryFile()
 
         # We create a new client here instead of re-using the stripe_mock_stripe_client fixture
@@ -46,6 +51,6 @@ def test_is_creatable(
             "post",
             api_base=stripe.upload_api_base,
             path="/v1/files",
-            content_type="multipart/form-data; boundary=1234567890",
+            content_type="multipart/form-data; boundary=abc123",
         )
         assert isinstance(resource, File)

tests/test_multipart_data_generator.py

@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 
 
+import random
 import re
 import io
 
@@ -87,3 +88,51 @@ def test_multipart_data_unicode_file_name(self):
         string = io.StringIO("foo")
         string.name = "паспорт.png"
         self.run_test_multipart_data_with_file(string)
+
+    def test_boundary_is_not_derived_from_the_random_module(self):
+        # Seeding the `random` module must not determine the boundary. A
+        # boundary an attacker can predict lets a caller-influenced value
+        # (including file bytes) inject additional parts.
+        random.seed(0)
+        first = MultipartDataGenerator().boundary
+        random.seed(0)
+        second = MultipartDataGenerator().boundary
+
+        assert first != second
+        assert re.fullmatch(r"[0-9a-f]{60}", first)
+        assert re.fullmatch(r"[0-9a-f]{60}", second)
+
+    @staticmethod
+    def lines_starting_with(http_body, prefix):
+        return [
+            line for line in http_body.split("\r\n") if line.startswith(prefix)
+        ]
+
+    def test_escapes_quotes_and_crlf_in_param_names(self):
+        injected = 'a\r\nContent-Disposition: form-data; name="purpose'
+        generator = MultipartDataGenerator()
+        generator.add_params({injected: "value"})
+        http_body = generator.get_post_data().decode("utf-8")
+
+        # The injected CRLF must not begin a second header line, and the
+        # injected quote must not end the quoted-string early.
+        assert self.lines_starting_with(http_body, "Content-Disposition:") == [
+            'Content-Disposition: form-data; name="a  Content-Disposition: '
+            'form-data; name=%22purpose"'
+        ]
+        # One opening delimiter and one closing delimiter: a single part.
+        assert http_body.count("--%s" % generator.boundary) == 2
+
+    def test_escapes_quotes_and_crlf_in_file_names(self):
+        test_file = io.StringIO("foo")
+        test_file.name = 'a\r\nX-Injected: yes"b.png'
+        generator = MultipartDataGenerator()
+        generator.add_params({"file": test_file})
+        http_body = generator.get_post_data().decode("utf-8")
+
+        assert self.lines_starting_with(http_body, "Content-Disposition:") == [
+            'Content-Disposition: form-data; name="file"; '
+            'filename="a  X-Injected: yes%22b.png"'
+        ]
+        assert self.lines_starting_with(http_body, "X-Injected:") == []
+        assert http_body.count("--%s" % generator.boundary) == 2