CVE-2023-34457: Improper Input Validation (CWE-20)
value: MechanicalSoup is a Python library for automating interaction with websites. Starting in version 0.2.0 and prior to version 1.3.0, a malicious web server can read arbitrary files on the client using a `<input type="file" ...>` inside HTML form. All users of MechanicalSoup's form submission are affected, unless they took very specific (and manual) steps to reset HTML form field values. Version 1.3.0 contains a patch for this issue.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 7.5
Published: 2023-07-05T20:15Z
Repository: https://github.com/MechanicalSoup/MechanicalSoup
Language: Python
Files changed: mechanicalsoup/browser.py, mechanicalsoup/form.py, mechanicalsoup/utils.py, tests/test_browser.py, tests/test_stateful_browser.py
Commit date: 2023-07-04 11:47:44 -0700
Commit message: Merge pull request from GHSA-x456-3ccm-m6j4

* Add suggested mitigation against malicious HTML form file input. Test cases and docs not updated.

* Make tests pass. Add test specifically for this vulnerability. Fix the `Form.new_control()` method so that it correctly sets the input value if the value is a file object.

* Add better documentation about the vulnerability remediation. Wording fix.

* tests: Add separate test to check for CVE-2023-34457.

* Revert "tests: Add separate test to check for CVE-2023-34457."

This reverts commit bd4c6d92a8a803499a447b511bc46b1ba00841d0.

* Misc cleanup

* No need to raise the error on forms with an enctype that do not
  upload files (e.g. everything besides multipart).

* Make sure that open file inputs are converted to their basename
  regardless of what kind of form it is to avoid leaking local file
  paths.

* Throw error at time when the file input is set incorrectly, rather
  than at submission time.

* Factor complex repeated logic into their own functions (i.e. for
  the multipart file input check and invalid file value check).

* Fix failing tests.

* Avoid excessive whitespace in exception message.

* Reorder 'and' operands so the slightly less expensive check comes first.

---------

Co-authored-by: Dan Hemberger <daniel.hemberger@gmail.com>
Diff: diff --git a/mechanicalsoup/browser.py b/mechanicalsoup/browser.py
index 285f8bb..dda98d9 100644
--- a/mechanicalsoup/browser.py
+++ b/mechanicalsoup/browser.py
@@ -1,5 +1,6 @@
+import io
 import os
 import tempfile
 import urllib
 import weakref
 import webbrowser
@@ -8,11 +9,11 @@ import bs4
 import bs4.dammit
 import requests
 
 from .__version__ import __title__, __version__
 from .form import Form
-from .utils import LinkNotFoundError
+from .utils import LinkNotFoundError, is_multipart_file_upload
 
 
 class Browser:
     """Builds a low-level Browser.
 
@@ -226,22 +227,24 @@ class Browser:
                     # browsers use empty string for inputs with missing values
                     value = tag.get("value", "")
 
                 # If the enctype is not multipart, the filename is put in
                 # the form as a text input and the file is not sent.
-                if tag.get("type", "").lower() == "file" and multipart:
-                    filepath = value
-                    if filepath != "" and isinstance(filepath, str):
-                        content = open(filepath, "rb")
+                if is_multipart_file_upload(form, tag):
+                    if isinstance(value, io.IOBase):
+                        content = value
+                        filename = os.path.basename(getattr(value, "name", ""))
                     else:
                         content = ""
-                    filename = os.path.basename(filepath)
-                    # If value is the empty string, we still pass it
+                        filename = os.path.basename(value)
+                    # If content is the empty string, we still pass it
                     # for consistency with browsers (see
                     # https://github.com/MechanicalSoup/MechanicalSoup/issues/250).
                     files[name] = (filename, content)
                 else:
+                    if isinstance(value, io.IOBase):
+                        value = os.path.basename(getattr(value, "name", ""))
                     data.append((name, value))
 
             elif tag.name == "button":
                 if tag.get("type", "").lower() in ("button", "reset"):
                     continue
diff --git a/mechanicalsoup/form.py b/mechanicalsoup/form.py
index a67195c..42b6949 100644
--- a/mechanicalsoup/form.py
+++ b/mechanicalsoup/form.py
@@ -1,11 +1,12 @@
 import copy
+import io
 import warnings
 
 from bs4 import BeautifulSoup
 
-from .utils import LinkNotFoundError
+from .utils import LinkNotFoundError, is_multipart_file_upload
 
 
 class InvalidFormMethod(LinkNotFoundError):
     """This exception is raised when a method of :class:`Form` is used
     for an HTML element that is of the wrong type (or is malformed).
@@ -66,10 +67,11 @@ class Form:
 
         for (name, value) in data.items():
             i = self.form.find("input", {"name": name})
             if not i:
                 raise InvalidFormMethod("No input field named " + name)
+            self._assert_valid_file_upload(i, value)
             i["value"] = value
 
     def uncheck_all(self, name):
         """Remove the *checked*-attribute of all input elements with
         a *name*-attribute given by ``name``.
@@ -259,16 +261,16 @@ class Form:
             form.set("login", username)
             form.set("password", password)
             form.set("eula-checkbox", True)
 
         Example: uploading a file through a ``<input type="file"
-        name="tagname">`` field (provide the path to the local file,
+        name="tagname">`` field (provide an open file object,
         and its content will be uploaded):
 
         .. code-block:: python
 
-            form.set("tagname", path_to_local_file)
+            form.set("tagname", open(path_to_local_file, "rb"))
 
         """
         for func in ("checkbox", "radio", "input", "textarea", "select"):
             try:
                 getattr(self, "set_" + func)({name: value})
@@ -298,10 +300,11 @@ class Form:
         control['type'] = type
         control['name'] = name
         control['value'] = value
         for k, v in kwargs.items():
             control[k] = v
+        self._assert_valid_file_upload(control, value)
         self.form.append(control)
         return control
 
     def choose_submit(self, submit):
         """Selects the input (or button) element to use for form submission.
@@ -381,5 +384,19 @@ class Form:
             # contains a lot of spaces that we don't want here.
             for subtag in input_copy.find_all() + [input_copy]:
                 if subtag.string:
                     subtag.string = subtag.string.strip()
             print(input_copy)
+
+    def _assert_valid_file_upload(self, tag, value):
+        """Raise an exception if a multipart file input is not an open file."""
+        if (
+            is_multipart_file_upload(self.form, tag) and
+            not isinstance(value, io.IOBase)
+        ):
+            raise ValueError(
+                "From v1.3.0 onwards, you must pass an open file object "
+                'directly, e.g. `form["name"] = open("/path/to/file", "rb")`. '
+                "This change is to remediate a security vulnerability where "
+                "a malicious web server could read arbitrary files from the "
+                "client (CVE-2023-34457)."
+            )
diff --git a/mechanicalsoup/utils.py b/mechanicalsoup/utils.py
index 9ad7ff0..18bef21 100644
--- a/mechanicalsoup/utils.py
+++ b/mechanicalsoup/utils.py
@@ -12,5 +12,12 @@ class LinkNotFoundError(Exception):
     * The user tried to fill-in a field which doesn't exist in a form
       (e.g. browser["name"] = "val" with browser being a
       StatefulBrowser).
     """
     pass
+
+
+def is_multipart_file_upload(form, tag):
+    return (
+        form.get("enctype", "") == "multipart/form-data" and
+        tag.get("type", "").lower() == "file"
+    )
diff --git a/tests/test_browser.py b/tests/test_browser.py
index d3a1a5c..1145e19 100644
--- a/tests/test_browser.py
+++ b/tests/test_browser.py
@@ -162,12 +162,13 @@ def test_enctype_and_file_submit(httpbin, enctype, submit_file, file_field):
             # Correct encoding => send the content
             expected_content = file_content
         else:
             # Encoding doesn't allow sending the content, we expect
             # the filename as a normal text field.
-            expected_content = pic_path.encode()
-        form.find("input", {"name": "pic"})["value"] = pic_path
+            expected_content = os.path.basename(pic_path.encode())
+        tag = form.find("input", {"name": "pic"})
+        tag["value"] = open(pic_path, "rb")
 
     browser = mechanicalsoup.Browser()
     response = browser._request(form)
 
     if enctype not in valid_enctypes_file_submit:
diff --git a/tests/test_stateful_browser.py b/tests/test_stateful_browser.py
index 0461ef8..1f2de87 100644
--- a/tests/test_stateful_browser.py
+++ b/tests/test_stateful_browser.py
@@ -389,36 +389,81 @@ def test_form_multiple():
     assert response.status_code == 200 and response.text == 'Success!'
 
 
 def test_upload_file(httpbin):
     browser = mechanicalsoup.StatefulBrowser()
-    browser.open(httpbin + "/forms/post")
+    url = httpbin + "/post"
+    file_input_form = f"""
+    <form method="post" action="{url}" enctype="multipart/form-data">
+        <input type="file" name="first" />
+    </form>
+    """
 
     # Create two temporary files to upload
     def make_file(content):
         path = tempfile.mkstemp()[1]
         with open(path, "w") as fd:
             fd.write(content)
         return path
-    path1, path2 = (make_file(content) for content in
-                    ("first file content", "second file content"))
+    path1 = make_file("first file content")
+    path2 = make_file("second file content")
 
-    # The form doesn't have a type=file field, but the target action
-    # does show it => add the fields ourselves, and add enctype too.
+    value1 = open(path1, "rb")
+    value2 = open(path2, "rb")
+
+    browser.open_fake_page(file_input_form)
     browser.select_form()
-    browser._StatefulBrowser__state.form.form[
-      "enctype"] = "multipart/form-data"
-    browser.new_control("file", "first", path1)
-    browser.new_control("file", "second", "")
-    browser["second"] = path2
-    browser.form.print_summary()
+
+    # Test filling an existing input and creating a new input
+    browser["first"] = value1
+    browser.new_control("file", "second", value2)
+
     response = browser.submit_selected()
     files = response.json()["files"]
     assert files["first"] == "first file content"
     assert files["second"] == "second file content"
 
 
+def test_upload_file_with_malicious_default(httpbin):
+    """Check for CVE-2023-34457 by setting the form input value directly to a
+    file that the user does not explicitly consent to upload, as a malicious
+    server might do.
+    """
+    browser = mechanicalsoup.StatefulBrowser()
+    sensitive_path = tempfile.mkstemp()[1]
+    with open(sensitive_path, "w") as fd:
+        fd.write("Some sensitive information")
+    url = httpbin + "/post"
+    malicious_html = f"""
+    <form method="post" action="{url}" enctype="multipart/form-data">
+        <input type="file" name="malicious" value="{sensitive_path}" />
+    </form>
+    """
+    browser.open_fake_page(malicious_html)
+    browser.select_form()
+    response = browser.submit_selected()
+    assert response.json()["files"] == {"malicious": ""}
+
+
+def test_upload_file_raise_on_string_input():
+    """Check for use of the file upload API that was modified to remediate
+    CVE-2023-34457. Users must now open files manually to upload them.
+    """
+    browser = mechanicalsoup.StatefulBrowser()
+    file_input_form = """
+    <form enctype="multipart/form-data">
+        <input type="file" name="upload" />
+    </form>
+    """
+    browser.open_fake_page(file_input_form)
+    browser.select_form()
+    with pytest.raises(ValueError, match="CVE-2023-34457"):
+        browser["upload"] = "/path/to/file"
+    with pytest.raises(ValueError, match="CVE-2023-34457"):
+        browser.new_control("file", "upload2", "/path/to/file")
+
+
 def test_with():
     """Test that __enter__/__exit__ properly create/close the browser."""
     with mechanicalsoup.StatefulBrowser() as browser:
         assert browser.session is not None
     assert browser.session is None

Vulnerable code: from .utils import LinkNotFoundError
                if tag.get("type", "").lower() == "file" and multipart:
                    filepath = value
                    if filepath != "" and isinstance(filepath, str):
                        content = open(filepath, "rb")
                    filename = os.path.basename(filepath)
                    # If value is the empty string, we still pass it
from .utils import LinkNotFoundError
        name="tagname">`` field (provide the path to the local file,
            form.set("tagname", path_to_local_file)
            expected_content = pic_path.encode()
        form.find("input", {"name": "pic"})["value"] = pic_path
    browser.open(httpbin + "/forms/post")
    path1, path2 = (make_file(content) for content in
                    ("first file content", "second file content"))
    # The form doesn't have a type=file field, but the target action
    # does show it => add the fields ourselves, and add enctype too.
    browser._StatefulBrowser__state.form.form[
      "enctype"] = "multipart/form-data"
    browser.new_control("file", "first", path1)
    browser.new_control("file", "second", "")
    browser["second"] = path2
    browser.form.print_summary()
Fixed code: import io
from .utils import LinkNotFoundError, is_multipart_file_upload
                if is_multipart_file_upload(form, tag):
                    if isinstance(value, io.IOBase):
                        content = value
                        filename = os.path.basename(getattr(value, "name", ""))
                        filename = os.path.basename(value)
                    # If content is the empty string, we still pass it
                    if isinstance(value, io.IOBase):
                        value = os.path.basename(getattr(value, "name", ""))
import io
from .utils import LinkNotFoundError, is_multipart_file_upload
            self._assert_valid_file_upload(i, value)
        name="tagname">`` field (provide an open file object,
            form.set("tagname", open(path_to_local_file, "rb"))
        self._assert_valid_file_upload(control, value)

    def _assert_valid_file_upload(self, tag, value):
        """Raise an exception if a multipart file input is not an open file."""
        if (
            is_multipart_file_upload(self.form, tag) and
            not isinstance(value, io.IOBase)
        ):
            raise ValueError(
                "From v1.3.0 onwards, you must pass an open file object "
                'directly, e.g. `form["name"] = open("/path/to/file", "rb")`. '
                "This change is to remediate a security vulnerability where "
                "a malicious web server could read arbitrary files from the "
                "client (CVE-2023-34457)."
            )


def is_multipart_file_upload(form, tag):
    return (
        form.get("enctype", "") == "multipart/form-data" and
        tag.get("type", "").lower() == "file"
    )
            expected_content = os.path.basename(pic_path.encode())
        tag = form.find("input", {"name": "pic"})
        tag["value"] = open(pic_path, "rb")
    url = httpbin + "/post"
    file_input_form = f"""
    <form method="post" action="{url}" enctype="multipart/form-data">
        <input type="file" name="first" />
    </form>
    """
    path1 = make_file("first file content")
    path2 = make_file("second file content")
    value1 = open(path1, "rb")
    value2 = open(path2, "rb")

    browser.open_fake_page(file_input_form)

    # Test filling an existing input and creating a new input
    browser["first"] = value1
    browser.new_control("file", "second", value2)

def test_upload_file_with_malicious_default(httpbin):
    """Check for CVE-2023-34457 by setting the form input value directly to a
    file that the user does not explicitly consent to upload, as a malicious
    server might do.
    """
    browser = mechanicalsoup.StatefulBrowser()
    sensitive_path = tempfile.mkstemp()[1]
    with open(sensitive_path, "w") as fd:
        fd.write("Some sensitive information")
    url = httpbin + "/post"
    malicious_html = f"""
    <form method="post" action="{url}" enctype="multipart/form-data">
        <input type="file" name="malicious" value="{sensitive_path}" />
    </form>
    """
    browser.open_fake_page(malicious_html)
    browser.select_form()
    response = browser.submit_selected()
    assert response.json()["files"] == {"malicious": ""}


def test_upload_file_raise_on_string_input():
    """Check for use of the file upload API that was modified to remediate
    CVE-2023-34457. Users must now open files manually to upload them.
    """
    browser = mechanicalsoup.StatefulBrowser()
    file_input_form = """
    <form enctype="multipart/form-data">
        <input type="file" name="upload" />
    </form>
    """
    browser.open_fake_page(file_input_form)
    browser.select_form()
    with pytest.raises(ValueError, match="CVE-2023-34457"):
        browser["upload"] = "/path/to/file"
    with pytest.raises(ValueError, match="CVE-2023-34457"):
        browser.new_control("file", "upload2", "/path/to/file")