diff --git a/stripe/_multipart_data_generator.py b/stripe/_multipart_data_generator.py index 3151df83e..ec91d22b4 100644 --- a/stripe/_multipart_data_generator.py +++ b/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) diff --git a/tests/api_resources/test_file.py b/tests/api_resources/test_file.py index 4494f543e..fe7037961 100644 --- a/tests/api_resources/test_file.py +++ b/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) diff --git a/tests/api_resources/test_file_upload.py b/tests/api_resources/test_file_upload.py index 8e52cf103..a2896a378 100644 --- a/tests/api_resources/test_file_upload.py +++ b/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) diff --git a/tests/services/test_file_upload.py b/tests/services/test_file_upload.py index cae1c79b7..fa671c1ff 100644 --- a/tests/services/test_file_upload.py +++ b/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) diff --git a/tests/test_multipart_data_generator.py b/tests/test_multipart_data_generator.py index b8f4fa738..0b9d5f93e 100644 --- a/tests/test_multipart_data_generator.py +++ b/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