CVE-2024-23334: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: aiohttp is an asynchronous HTTP client/server framework for asyncio and Python. When using aiohttp as a web server and configuring static routes, it is necessary to specify the root path for static files. Additionally, the option 'follow_symlinks' can be used to determine whether to follow symbolic links outside the static root directory. When 'follow_symlinks' is set to True, there is no validation to check if reading a file is within the root directory. This can lead to directory traversal vulnerabilities, resulting in unauthorized access to arbitrary files on the system, even when symlinks are not present.  Disabling follow_symlinks and using a reverse proxy are encouraged mitigations.  Version 3.9.2 fixes this issue.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 7.5
Published: 2024-01-29T23:15Z
Repository: https://github.com/aio-libs/aiohttp
Language: Other
Files changed: CHANGES/8079.bugfix.rst, aiohttp/web_urldispatcher.py, docs/web_advanced.rst, docs/web_reference.rst, tests/test_web_urldispatcher.py
Commit date: 2024-01-28 18:13:06 +0000
Commit message: Validate static paths (#8079)

Co-authored-by: J. Nick Koston <nick@koston.org>
Diff: diff --git a/CHANGES/8079.bugfix.rst b/CHANGES/8079.bugfix.rst
new file mode 100644
index 000000000..57bc8bfeb
--- /dev/null
+++ b/CHANGES/8079.bugfix.rst
@@ -0,0 +1 @@
+Improved validation of paths for static resources -- by :user:`bdraco`.
diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py
index fee4f61a1..a05ac6cc3 100644
--- a/aiohttp/web_urldispatcher.py
+++ b/aiohttp/web_urldispatcher.py
@@ -571,13 +571,18 @@ class StaticResource(PrefixResource):
             url = url / filename.replace("%", "%25")
         else:
             url = url / filename
 
         if append_version:
+            unresolved_path = self._directory.joinpath(filename)
             try:
-                filepath = self._directory.joinpath(filename).resolve()
-                if not self._follow_symlinks:
+                if self._follow_symlinks:
+                    normalized_path = Path(os.path.normpath(unresolved_path))
+                    normalized_path.relative_to(self._directory)
+                    filepath = normalized_path.resolve()
+                else:
+                    filepath = unresolved_path.resolve()
                     filepath.relative_to(self._directory)
             except (ValueError, FileNotFoundError):
                 # ValueError for case when path point to symlink
                 # with follow_symlinks is False
                 return url  # relatively safe
@@ -638,12 +643,17 @@ class StaticResource(PrefixResource):
             if filename.anchor:
                 # rel_url is an absolute name like
                 # /static/\\machine_name\c$ or /static/D:\path
                 # where the static dir is totally different
                 raise HTTPForbidden()
-            filepath = self._directory.joinpath(filename).resolve()
-            if not self._follow_symlinks:
+            unresolved_path = self._directory.joinpath(filename)
+            if self._follow_symlinks:
+                normalized_path = Path(os.path.normpath(unresolved_path))
+                normalized_path.relative_to(self._directory)
+                filepath = normalized_path.resolve()
+            else:
+                filepath = unresolved_path.resolve()
                 filepath.relative_to(self._directory)
         except (ValueError, FileNotFoundError) as error:
             # relatively safe
             raise HTTPNotFound() from error
         except HTTPForbidden:
diff --git a/docs/web_advanced.rst b/docs/web_advanced.rst
index 6b1ad1997..5ba3cd334 100644
--- a/docs/web_advanced.rst
+++ b/docs/web_advanced.rst
@@ -260,16 +260,26 @@ When a directory is accessed within a static route then the server responses
 to client with ``HTTP/403 Forbidden`` by default. Displaying folder index
 instead could be enabled with ``show_index`` parameter set to ``True``::
 
    web.static('/prefix', path_to_static_folder, show_index=True)
 
-When a symlink from the static directory is accessed, the server responses to
-client with ``HTTP/404 Not Found`` by default. To allow the server to follow
-symlinks, parameter ``follow_symlinks`` should be set to ``True``::
+When a symlink that leads outside the static directory is accessed, the server
+responds to the client with ``HTTP/404 Not Found`` by default. To allow the server to
+follow symlinks that lead outside the static root, the parameter ``follow_symlinks``
+should be set to ``True``::
 
    web.static('/prefix', path_to_static_folder, follow_symlinks=True)
 
+.. caution::
+
+   Enabling ``follow_symlinks`` can be a security risk, and may lead to
+   a directory transversal attack. You do NOT need this option to follow symlinks
+   which point to somewhere else within the static directory, this option is only
+   used to break out of the security sandbox. Enabling this option is highly
+   discouraged, and only expected to be used for edge cases in a local
+   development setting where remote users do not have access to the server.
+
 When you want to enable cache busting,
 parameter ``append_version`` can be set to ``True``
 
 Cache busting is the process of appending some form of file version hash
 to the filename of resources like JavaScript and CSS files.
diff --git a/docs/web_reference.rst b/docs/web_reference.rst
index 60dc723c4..127d075ca 100644
--- a/docs/web_reference.rst
+++ b/docs/web_reference.rst
@@ -1782,13 +1782,19 @@ Application and Router
 
       :param bool show_index: flag for allowing to show indexes of a directory,
                               by default it's not allowed and HTTP/403 will
                               be returned on directory access.
 
-      :param bool follow_symlinks: flag for allowing to follow symlinks from
-                              a directory, by default it's not allowed and
-                              HTTP/404 will be returned on access.
+      :param bool follow_symlinks: flag for allowing to follow symlinks that lead
+                              outside the static root directory, by default it's not allowed and
+                              HTTP/404 will be returned on access.  Enabling ``follow_symlinks``
+                              can be a security risk, and may lead to a directory transversal attack.
+                              You do NOT need this option to follow symlinks which point to somewhere
+                              else within the static directory, this option is only used to break out
+                              of the security sandbox. Enabling this option is highly discouraged,
+                              and only expected to be used for edge cases in a local development
+                              setting where remote users do not have access to the server.
 
       :param bool append_version: flag for adding file version (hash)
                               to the url query string, this value will
                               be used as default when you call to
                               :meth:`~aiohttp.web.AbstractRoute.url` and
diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py
index 716fa78fd..c3caa4ffc 100644
--- a/tests/test_web_urldispatcher.py
+++ b/tests/test_web_urldispatcher.py
@@ -106,10 +106,101 @@ async def test_follow_symlink(
     r = await client.get("/my_symlink/my_file_in_dir")
     assert r.status == 200
     assert (await r.text()) == data
 
 
+async def test_follow_symlink_directory_traversal(
+    tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
+) -> None:
+    # Tests that follow_symlinks does not allow directory transversal
+    data = "private"
+
+    private_file = tmp_path / "private_file"
+    private_file.write_text(data)
+
+    safe_path = tmp_path / "safe_dir"
+    safe_path.mkdir()
+
+    app = web.Application()
+
+    # Register global static route:
+    app.router.add_static("/", str(safe_path), follow_symlinks=True)
+    client = await aiohttp_client(app)
+
+    await client.start_server()
+    # We need to use a raw socket to test this, as the client will normalize
+    # the path before sending it to the server.
+    reader, writer = await asyncio.open_connection(client.host, client.port)
+    writer.write(b"GET /../private_file HTTP/1.1\r\n\r\n")
+    response = await reader.readuntil(b"\r\n\r\n")
+    assert b"404 Not Found" in response
+    writer.close()
+    await writer.wait_closed()
+    await client.close()
+
+
+async def test_follow_symlink_directory_traversal_after_normalization(
+    tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
+) -> None:
+    # Tests that follow_symlinks does not allow directory transversal
+    # after normalization
+    #
+    # Directory structure
+    # |-- secret_dir
+    # |   |-- private_file (should never be accessible)
+    # |   |-- symlink_target_dir
+    # |       |-- symlink_target_file (should be accessible via the my_symlink symlink)
+    # |       |-- sandbox_dir
+    # |           |-- my_symlink -> symlink_target_dir
+    #
+    secret_path = tmp_path / "secret_dir"
+    secret_path.mkdir()
+
+    # This file is below the symlink target and should not be reachable
+    private_file = secret_path / "private_file"
+    private_file.write_text("private")
+
+    symlink_target_path = secret_path / "symlink_target_dir"
+    symlink_target_path.mkdir()
+
+    sandbox_path = symlink_target_path / "sandbox_dir"
+    sandbox_path.mkdir()
+
+    # This file should be reachable via the symlink
+    symlink_target_file = symlink_target_path / "symlink_target_file"
+    symlink_target_file.write_text("readable")
+
+    my_symlink_path = sandbox_path / "my_symlink"
+    pathlib.Path(str(my_symlink_path)).symlink_to(str(symlink_target_path), True)
+
+    app = web.Application()
+
+    # Register global static route:
+    app.router.add_static("/", str(sandbox_path), follow_symlinks=True)
+    client = await aiohttp_client(app)
+
+    await client.start_server()
+    # We need to use a raw socket to test this, as the client will normalize
+    # the path before sending it to the server.
+    reader, writer = await asyncio.open_connection(client.host, client.port)
+    writer.write(b"GET /my_symlink/../private_file HTTP/1.1\r\n\r\n")
+    response = await reader.readuntil(b"\r\n\r\n")
+    assert b"404 Not Found" in response
+    writer.close()
+    await writer.wait_closed()
+
+    reader, writer = await asyncio.open_connection(client.host, client.port)
+    writer.write(b"GET /my_symlink/symlink_target_file HTTP/1.1\r\n\r\n")
+    response = await reader.readuntil(b"\r\n\r\n")
+    assert b"200 OK" in response
+    response = await reader.readuntil(b"readable")
+    assert response == b"readable"
+    writer.close()
+    await writer.wait_closed()
+    await client.close()
+
+
 @pytest.mark.parametrize(
     "dir_name,filename,data",
     [
         ("", "test file.txt", "test text"),
         ("test dir name", "test dir file .txt", "test text file folder"),

Vulnerable code:                 filepath = self._directory.joinpath(filename).resolve()
                if not self._follow_symlinks:
            filepath = self._directory.joinpath(filename).resolve()
            if not self._follow_symlinks:
When a symlink from the static directory is accessed, the server responses to
client with ``HTTP/404 Not Found`` by default. To allow the server to follow
symlinks, parameter ``follow_symlinks`` should be set to ``True``::
      :param bool follow_symlinks: flag for allowing to follow symlinks from
                              a directory, by default it's not allowed and
                              HTTP/404 will be returned on access.
Fixed code: Improved validation of paths for static resources -- by :user:`bdraco`.
            unresolved_path = self._directory.joinpath(filename)
                if self._follow_symlinks:
                    normalized_path = Path(os.path.normpath(unresolved_path))
                    normalized_path.relative_to(self._directory)
                    filepath = normalized_path.resolve()
                else:
                    filepath = unresolved_path.resolve()
            unresolved_path = self._directory.joinpath(filename)
            if self._follow_symlinks:
                normalized_path = Path(os.path.normpath(unresolved_path))
                normalized_path.relative_to(self._directory)
                filepath = normalized_path.resolve()
            else:
                filepath = unresolved_path.resolve()
When a symlink that leads outside the static directory is accessed, the server
responds to the client with ``HTTP/404 Not Found`` by default. To allow the server to
follow symlinks that lead outside the static root, the parameter ``follow_symlinks``
should be set to ``True``::
.. caution::

   Enabling ``follow_symlinks`` can be a security risk, and may lead to
   a directory transversal attack. You do NOT need this option to follow symlinks
   which point to somewhere else within the static directory, this option is only
   used to break out of the security sandbox. Enabling this option is highly
   discouraged, and only expected to be used for edge cases in a local
   development setting where remote users do not have access to the server.

      :param bool follow_symlinks: flag for allowing to follow symlinks that lead
                              outside the static root directory, by default it's not allowed and
                              HTTP/404 will be returned on access.  Enabling ``follow_symlinks``
                              can be a security risk, and may lead to a directory transversal attack.
                              You do NOT need this option to follow symlinks which point to somewhere
                              else within the static directory, this option is only used to break out
                              of the security sandbox. Enabling this option is highly discouraged,
                              and only expected to be used for edge cases in a local development
                              setting where remote users do not have access to the server.
async def test_follow_symlink_directory_traversal(
    tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
) -> None:
    # Tests that follow_symlinks does not allow directory transversal
    data = "private"

    private_file = tmp_path / "private_file"
    private_file.write_text(data)

    safe_path = tmp_path / "safe_dir"
    safe_path.mkdir()

    app = web.Application()

    # Register global static route:
    app.router.add_static("/", str(safe_path), follow_symlinks=True)
    client = await aiohttp_client(app)

    await client.start_server()
    # We need to use a raw socket to test this, as the client will normalize
    # the path before sending it to the server.
    reader, writer = await asyncio.open_connection(client.host, client.port)
    writer.write(b"GET /../private_file HTTP/1.1\r\n\r\n")
    response = await reader.readuntil(b"\r\n\r\n")
    assert b"404 Not Found" in response
    writer.close()
    await writer.wait_closed()
    await client.close()


async def test_follow_symlink_directory_traversal_after_normalization(
    tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
) -> None:
    # Tests that follow_symlinks does not allow directory transversal
    # after normalization
    #
    # Directory structure
    # |-- secret_dir
    # |   |-- private_file (should never be accessible)
    # |   |-- symlink_target_dir
    # |       |-- symlink_target_file (should be accessible via the my_symlink symlink)
    # |       |-- sandbox_dir
    # |           |-- my_symlink -> symlink_target_dir
    #
    secret_path = tmp_path / "secret_dir"
    secret_path.mkdir()

    # This file is below the symlink target and should not be reachable
    private_file = secret_path / "private_file"
    private_file.write_text("private")

    symlink_target_path = secret_path / "symlink_target_dir"
    symlink_target_path.mkdir()

    sandbox_path = symlink_target_path / "sandbox_dir"
    sandbox_path.mkdir()

    # This file should be reachable via the symlink
    symlink_target_file = symlink_target_path / "symlink_target_file"
    symlink_target_file.write_text("readable")

    my_symlink_path = sandbox_path / "my_symlink"
    pathlib.Path(str(my_symlink_path)).symlink_to(str(symlink_target_path), True)

    app = web.Application()

    # Register global static route:
    app.router.add_static("/", str(sandbox_path), follow_symlinks=True)
    client = await aiohttp_client(app)

    await client.start_server()
    # We need to use a raw socket to test this, as the client will normalize
    # the path before sending it to the server.
    reader, writer = await asyncio.open_connection(client.host, client.port)
    writer.write(b"GET /my_symlink/../private_file HTTP/1.1\r\n\r\n")
    response = await reader.readuntil(b"\r\n\r\n")
    assert b"404 Not Found" in response
    writer.close()
    await writer.wait_closed()

    reader, writer = await asyncio.open_connection(client.host, client.port)
    writer.write(b"GET /my_symlink/symlink_target_file HTTP/1.1\r\n\r\n")
    response = await reader.readuntil(b"\r\n\r\n")
    assert b"200 OK" in response
    response = await reader.readuntil(b"readable")
    assert response == b"readable"
    writer.close()
    await writer.wait_closed()
    await client.close()