From 7a8cac949d30c80ad5560bbe5ce1a00c45d0ea20 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 14 Jan 2024 09:22:29 -1000 Subject: [PATCH 01/15] Fix directory traversal when follow_symlinks=True --- aiohttp/web_urldispatcher.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index fee4f61a1aa..28755111ce4 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -573,9 +573,12 @@ def url_for( # type: ignore[override] url = url / filename if append_version: + unresolved_path = self._directory.joinpath(filename) try: - filepath = self._directory.joinpath(filename).resolve() - if not self._follow_symlinks: + filepath = unresolved_path.resolve() + if self._follow_symlinks: + Path(os.path.normpath(unresolved_path)).relative_to(self._directory) + else: filepath.relative_to(self._directory) except (ValueError, FileNotFoundError): # ValueError for case when path point to symlink @@ -640,8 +643,11 @@ async def _handle(self, request: Request) -> StreamResponse: # /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) + filepath = unresolved_path.resolve() + if self._follow_symlinks: + Path(os.path.normpath(unresolved_path)).relative_to(self._directory) + else: filepath.relative_to(self._directory) except (ValueError, FileNotFoundError) as error: # relatively safe From cc2afb901c1136f323b656dfe953f0cfc95885fe Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 14 Jan 2024 09:38:25 -1000 Subject: [PATCH 02/15] coverage --- tests/test_web_urldispatcher.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index 716fa78fd4a..c9476343b2d 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -108,6 +108,30 @@ async def test_follow_symlink( assert (await r.text()) == data +async def test_follow_symlink_directory_transversal( + tmp_path: pathlib.Path, aiohttp_client: AiohttpClient +) -> None: + # Tests that follow_symlinks does not allow directory transversal + app = web.Application() + + # Register global static route: + app.router.add_static("/", str(tmp_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( + "GET /../../../../../../../../../../../../../../../etc/passwd HTTP/1.1\r\n\r\n".encode() + ) + 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() + + @pytest.mark.parametrize( "dir_name,filename,data", [ From b7b339c4f67942b7eb28a6e371f8e0bb72eb6ca2 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 14 Jan 2024 12:37:10 -1000 Subject: [PATCH 03/15] adjust test to work on systems without /etc/passwd --- tests/test_web_urldispatcher.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index c9476343b2d..5f7f1e1c70f 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -112,10 +112,18 @@ async def test_follow_symlink_directory_transversal( 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(tmp_path), follow_symlinks=True) + app.router.add_static("/", str(safe_path), follow_symlinks=True) client = await aiohttp_client(app) await client.start_server() @@ -123,7 +131,7 @@ async def test_follow_symlink_directory_transversal( # the path before sending it to the server. reader, writer = await asyncio.open_connection(client.host, client.port) writer.write( - "GET /../../../../../../../../../../../../../../../etc/passwd HTTP/1.1\r\n\r\n".encode() + "GET /../private_file HTTP/1.1\r\n\r\n".encode() ) response = await reader.readuntil(b"\r\n\r\n") assert b"404 Not Found" in response From 5ef1d243d5e8befabf629007d556759e8a899e0e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 14 Jan 2024 12:37:19 -1000 Subject: [PATCH 04/15] adjust test to work on systems without /etc/passwd --- tests/test_web_urldispatcher.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index 5f7f1e1c70f..b8ba914e007 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -130,9 +130,7 @@ async def test_follow_symlink_directory_transversal( # 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( - "GET /../private_file HTTP/1.1\r\n\r\n".encode() - ) + writer.write("GET /../private_file HTTP/1.1\r\n\r\n".encode()) response = await reader.readuntil(b"\r\n\r\n") assert b"404 Not Found" in response writer.close() From 12a2b51805bc6bff6d2ed6dfafeb071f28e790c4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 14 Jan 2024 14:53:45 -1000 Subject: [PATCH 05/15] adjust --- aiohttp/web_urldispatcher.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 28755111ce4..a05ac6cc3de 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -575,10 +575,12 @@ def url_for( # type: ignore[override] if append_version: unresolved_path = self._directory.joinpath(filename) try: - filepath = unresolved_path.resolve() if self._follow_symlinks: - Path(os.path.normpath(unresolved_path)).relative_to(self._directory) + 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 @@ -644,10 +646,12 @@ async def _handle(self, request: Request) -> StreamResponse: # where the static dir is totally different raise HTTPForbidden() unresolved_path = self._directory.joinpath(filename) - filepath = unresolved_path.resolve() if self._follow_symlinks: - Path(os.path.normpath(unresolved_path)).relative_to(self._directory) + 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 From 11306541a41f724ad4d867ab80b733b94ef44158 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 17 Jan 2024 14:58:55 -1000 Subject: [PATCH 06/15] fix docs, add warning --- docs/web_advanced.rst | 14 +++++++++++--- docs/web_reference.rst | 10 +++++++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/docs/web_advanced.rst b/docs/web_advanced.rst index 6b1ad199758..84e6ceeaf77 100644 --- a/docs/web_advanced.rst +++ b/docs/web_advanced.rst @@ -262,12 +262,20 @@ 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 client with ``HTTP/404 Not Found`` by default. To allow the server to +follow symlinks that lead outside the path, the parameter ``follow_symlinks`` should +be set to ``True``:: web.static('/prefix', path_to_static_folder, follow_symlinks=True) +.. warning:: + + Enabling ``follow_symlinks`` can be a security risk, and may lead to + a directory transversal attack. Enabling this option is highly discouraged. + It is recommended to only enable this option when serving static files from a + trusted directory that is not writable by anyone else. + When you want to enable cache busting, parameter ``append_version`` can be set to ``True`` diff --git a/docs/web_reference.rst b/docs/web_reference.rst index 60dc723c405..3b4710d7227 100644 --- a/docs/web_reference.rst +++ b/docs/web_reference.rst @@ -1784,9 +1784,13 @@ Application and Router 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 path, 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. + Enabling this option is highly discouraged. It is recommended to only + enable this option when serving static files from a trusted directory + that is not writable by anyone else. :param bool append_version: flag for adding file version (hash) to the url query string, this value will From 592317b9476614783e0ec9c6cf9efd1206580530 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 17 Jan 2024 15:15:11 -1000 Subject: [PATCH 07/15] add another test --- aiohttp/web_urldispatcher.py | 2 ++ tests/test_web_urldispatcher.py | 60 +++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index a05ac6cc3de..6f2fc66adb5 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -677,6 +677,8 @@ async def _handle(self, request: Request) -> StreamResponse: elif filepath.is_file(): return FileResponse(filepath, chunk_size=self._chunk_size) else: + import pprint + pprint.pprint(['404', filepath]) raise HTTPNotFound def _directory_as_html(self, filepath: Path) -> str: diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index b8ba914e007..b10edaf0be4 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -137,6 +137,66 @@ async def test_follow_symlink_directory_transversal( await writer.wait_closed() await client.close() +async def test_follow_symlink_directory_transversal_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("GET /my_symlink/../private_file HTTP/1.1\r\n\r\n".encode()) + 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("GET /my_symlink/symlink_target_file HTTP/1.1\r\n\r\n".encode()) + 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", From aadeaaced48e54820c85ff880a61c8093ec32ab3 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 17 Jan 2024 15:15:17 -1000 Subject: [PATCH 08/15] add another test --- tests/test_web_urldispatcher.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index b10edaf0be4..f53d46b94b6 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -137,6 +137,7 @@ async def test_follow_symlink_directory_transversal( await writer.wait_closed() await client.close() + async def test_follow_symlink_directory_transversal_after_normalization( tmp_path: pathlib.Path, aiohttp_client: AiohttpClient ) -> None: From 7e36ff0726398e74a482fb113ebcb5eff6696aec Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 17 Jan 2024 15:16:06 -1000 Subject: [PATCH 09/15] preen debug --- aiohttp/web_urldispatcher.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 6f2fc66adb5..a05ac6cc3de 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -677,8 +677,6 @@ async def _handle(self, request: Request) -> StreamResponse: elif filepath.is_file(): return FileResponse(filepath, chunk_size=self._chunk_size) else: - import pprint - pprint.pprint(['404', filepath]) raise HTTPNotFound def _directory_as_html(self, filepath: Path) -> str: From ad173143312e1b5b6139625f57badc6e811aef7c Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 20 Jan 2024 12:39:11 +0000 Subject: [PATCH 10/15] Update web_advanced.rst --- docs/web_advanced.rst | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/web_advanced.rst b/docs/web_advanced.rst index 84e6ceeaf77..1b2baadc14b 100644 --- a/docs/web_advanced.rst +++ b/docs/web_advanced.rst @@ -263,18 +263,20 @@ instead could be enabled with ``show_index`` parameter set to ``True``:: web.static('/prefix', path_to_static_folder, show_index=True) When a symlink that leads outside the static directory is accessed, the server -responds to client with ``HTTP/404 Not Found`` by default. To allow the server to -follow symlinks that lead outside the path, the parameter ``follow_symlinks`` should -be set to ``True``:: +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) .. warning:: Enabling ``follow_symlinks`` can be a security risk, and may lead to - a directory transversal attack. Enabling this option is highly discouraged. - It is recommended to only enable this option when serving static files from a - trusted directory that is not writable by anyone else. + 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`` From 79db6d8f7e3d54e2a0566c84b12a3caf9221553c Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sat, 20 Jan 2024 12:41:43 +0000 Subject: [PATCH 11/15] Update web_reference.rst --- docs/web_reference.rst | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/web_reference.rst b/docs/web_reference.rst index 3b4710d7227..127d075cad7 100644 --- a/docs/web_reference.rst +++ b/docs/web_reference.rst @@ -1785,12 +1785,14 @@ Application and Router be returned on directory access. :param bool follow_symlinks: flag for allowing to follow symlinks that lead - outside the path, by default it's not allowed and + 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. - Enabling this option is highly discouraged. It is recommended to only - enable this option when serving static files from a trusted directory - that is not writable by anyone else. + 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 From ee255249cc507c788c15dc0b36736fa54ad714e8 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 28 Jan 2024 15:39:31 +0000 Subject: [PATCH 12/15] Update test_web_urldispatcher.py --- tests/test_web_urldispatcher.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index f53d46b94b6..556869c167f 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -108,7 +108,7 @@ async def test_follow_symlink( assert (await r.text()) == data -async def test_follow_symlink_directory_transversal( +async def test_follow_symlink_directory_traversal( tmp_path: pathlib.Path, aiohttp_client: AiohttpClient ) -> None: # Tests that follow_symlinks does not allow directory transversal @@ -138,7 +138,7 @@ async def test_follow_symlink_directory_transversal( await client.close() -async def test_follow_symlink_directory_transversal_after_normalization( +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 From b2c6f445d6d7d23a805beeffe1284bed0c5c3be6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 28 Jan 2024 17:39:42 +0000 Subject: [PATCH 13/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_web_urldispatcher.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index 556869c167f..c3caa4ffcff 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -130,7 +130,7 @@ async def test_follow_symlink_directory_traversal( # 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("GET /../private_file HTTP/1.1\r\n\r\n".encode()) + 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() @@ -182,14 +182,14 @@ async def test_follow_symlink_directory_traversal_after_normalization( # 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("GET /my_symlink/../private_file HTTP/1.1\r\n\r\n".encode()) + 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("GET /my_symlink/symlink_target_file HTTP/1.1\r\n\r\n".encode()) + 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") From 078e9df80f9130f0fc399d67015b9eb6b43502c1 Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 28 Jan 2024 17:40:32 +0000 Subject: [PATCH 14/15] Create 8079.bugfix.rst --- CHANGES/8079.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 CHANGES/8079.bugfix.rst diff --git a/CHANGES/8079.bugfix.rst b/CHANGES/8079.bugfix.rst new file mode 100644 index 00000000000..57bc8bfebcc --- /dev/null +++ b/CHANGES/8079.bugfix.rst @@ -0,0 +1 @@ +Improved validation of paths for static resources -- by :user:`bdraco`. From 64208f77cb9bd8e9cfde1538741eae992e96bb0a Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Sun, 28 Jan 2024 17:48:24 +0000 Subject: [PATCH 15/15] Update docs/web_advanced.rst --- docs/web_advanced.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/web_advanced.rst b/docs/web_advanced.rst index 1b2baadc14b..5ba3cd33442 100644 --- a/docs/web_advanced.rst +++ b/docs/web_advanced.rst @@ -269,7 +269,7 @@ should be set to ``True``:: web.static('/prefix', path_to_static_folder, follow_symlinks=True) -.. warning:: +.. 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