Skip to content

Commit 1c33594

Browse files
Validate static paths (#8079)
Co-authored-by: J. Nick Koston <[email protected]>
1 parent 33ccdfb commit 1c33594

File tree

5 files changed

+128
-10
lines changed

5 files changed

+128
-10
lines changed

CHANGES/8079.bugfix.rst

+1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Improved validation of paths for static resources -- by :user:`bdraco`.

aiohttp/web_urldispatcher.py

+14-4
Original file line numberDiff line numberDiff line change
@@ -573,9 +573,14 @@ def url_for( # type: ignore[override]
573573
url = url / filename
574574

575575
if append_version:
576+
unresolved_path = self._directory.joinpath(filename)
576577
try:
577-
filepath = self._directory.joinpath(filename).resolve()
578-
if not self._follow_symlinks:
578+
if self._follow_symlinks:
579+
normalized_path = Path(os.path.normpath(unresolved_path))
580+
normalized_path.relative_to(self._directory)
581+
filepath = normalized_path.resolve()
582+
else:
583+
filepath = unresolved_path.resolve()
579584
filepath.relative_to(self._directory)
580585
except (ValueError, FileNotFoundError):
581586
# ValueError for case when path point to symlink
@@ -640,8 +645,13 @@ async def _handle(self, request: Request) -> StreamResponse:
640645
# /static/\\machine_name\c$ or /static/D:\path
641646
# where the static dir is totally different
642647
raise HTTPForbidden()
643-
filepath = self._directory.joinpath(filename).resolve()
644-
if not self._follow_symlinks:
648+
unresolved_path = self._directory.joinpath(filename)
649+
if self._follow_symlinks:
650+
normalized_path = Path(os.path.normpath(unresolved_path))
651+
normalized_path.relative_to(self._directory)
652+
filepath = normalized_path.resolve()
653+
else:
654+
filepath = unresolved_path.resolve()
645655
filepath.relative_to(self._directory)
646656
except (ValueError, FileNotFoundError) as error:
647657
# relatively safe

docs/web_advanced.rst

+13-3
Original file line numberDiff line numberDiff line change
@@ -262,12 +262,22 @@ instead could be enabled with ``show_index`` parameter set to ``True``::
262262

263263
web.static('/prefix', path_to_static_folder, show_index=True)
264264

265-
When a symlink from the static directory is accessed, the server responses to
266-
client with ``HTTP/404 Not Found`` by default. To allow the server to follow
267-
symlinks, parameter ``follow_symlinks`` should be set to ``True``::
265+
When a symlink that leads outside the static directory is accessed, the server
266+
responds to the client with ``HTTP/404 Not Found`` by default. To allow the server to
267+
follow symlinks that lead outside the static root, the parameter ``follow_symlinks``
268+
should be set to ``True``::
268269

269270
web.static('/prefix', path_to_static_folder, follow_symlinks=True)
270271

272+
.. caution::
273+
274+
Enabling ``follow_symlinks`` can be a security risk, and may lead to
275+
a directory transversal attack. You do NOT need this option to follow symlinks
276+
which point to somewhere else within the static directory, this option is only
277+
used to break out of the security sandbox. Enabling this option is highly
278+
discouraged, and only expected to be used for edge cases in a local
279+
development setting where remote users do not have access to the server.
280+
271281
When you want to enable cache busting,
272282
parameter ``append_version`` can be set to ``True``
273283

docs/web_reference.rst

+9-3
Original file line numberDiff line numberDiff line change
@@ -1784,9 +1784,15 @@ Application and Router
17841784
by default it's not allowed and HTTP/403 will
17851785
be returned on directory access.
17861786

1787-
:param bool follow_symlinks: flag for allowing to follow symlinks from
1788-
a directory, by default it's not allowed and
1789-
HTTP/404 will be returned on access.
1787+
:param bool follow_symlinks: flag for allowing to follow symlinks that lead
1788+
outside the static root directory, by default it's not allowed and
1789+
HTTP/404 will be returned on access. Enabling ``follow_symlinks``
1790+
can be a security risk, and may lead to a directory transversal attack.
1791+
You do NOT need this option to follow symlinks which point to somewhere
1792+
else within the static directory, this option is only used to break out
1793+
of the security sandbox. Enabling this option is highly discouraged,
1794+
and only expected to be used for edge cases in a local development
1795+
setting where remote users do not have access to the server.
17901796

17911797
:param bool append_version: flag for adding file version (hash)
17921798
to the url query string, this value will

tests/test_web_urldispatcher.py

+91
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,97 @@ async def test_follow_symlink(
108108
assert (await r.text()) == data
109109

110110

111+
async def test_follow_symlink_directory_traversal(
112+
tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
113+
) -> None:
114+
# Tests that follow_symlinks does not allow directory transversal
115+
data = "private"
116+
117+
private_file = tmp_path / "private_file"
118+
private_file.write_text(data)
119+
120+
safe_path = tmp_path / "safe_dir"
121+
safe_path.mkdir()
122+
123+
app = web.Application()
124+
125+
# Register global static route:
126+
app.router.add_static("/", str(safe_path), follow_symlinks=True)
127+
client = await aiohttp_client(app)
128+
129+
await client.start_server()
130+
# We need to use a raw socket to test this, as the client will normalize
131+
# the path before sending it to the server.
132+
reader, writer = await asyncio.open_connection(client.host, client.port)
133+
writer.write(b"GET /../private_file HTTP/1.1\r\n\r\n")
134+
response = await reader.readuntil(b"\r\n\r\n")
135+
assert b"404 Not Found" in response
136+
writer.close()
137+
await writer.wait_closed()
138+
await client.close()
139+
140+
141+
async def test_follow_symlink_directory_traversal_after_normalization(
142+
tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
143+
) -> None:
144+
# Tests that follow_symlinks does not allow directory transversal
145+
# after normalization
146+
#
147+
# Directory structure
148+
# |-- secret_dir
149+
# | |-- private_file (should never be accessible)
150+
# | |-- symlink_target_dir
151+
# | |-- symlink_target_file (should be accessible via the my_symlink symlink)
152+
# | |-- sandbox_dir
153+
# | |-- my_symlink -> symlink_target_dir
154+
#
155+
secret_path = tmp_path / "secret_dir"
156+
secret_path.mkdir()
157+
158+
# This file is below the symlink target and should not be reachable
159+
private_file = secret_path / "private_file"
160+
private_file.write_text("private")
161+
162+
symlink_target_path = secret_path / "symlink_target_dir"
163+
symlink_target_path.mkdir()
164+
165+
sandbox_path = symlink_target_path / "sandbox_dir"
166+
sandbox_path.mkdir()
167+
168+
# This file should be reachable via the symlink
169+
symlink_target_file = symlink_target_path / "symlink_target_file"
170+
symlink_target_file.write_text("readable")
171+
172+
my_symlink_path = sandbox_path / "my_symlink"
173+
pathlib.Path(str(my_symlink_path)).symlink_to(str(symlink_target_path), True)
174+
175+
app = web.Application()
176+
177+
# Register global static route:
178+
app.router.add_static("/", str(sandbox_path), follow_symlinks=True)
179+
client = await aiohttp_client(app)
180+
181+
await client.start_server()
182+
# We need to use a raw socket to test this, as the client will normalize
183+
# the path before sending it to the server.
184+
reader, writer = await asyncio.open_connection(client.host, client.port)
185+
writer.write(b"GET /my_symlink/../private_file HTTP/1.1\r\n\r\n")
186+
response = await reader.readuntil(b"\r\n\r\n")
187+
assert b"404 Not Found" in response
188+
writer.close()
189+
await writer.wait_closed()
190+
191+
reader, writer = await asyncio.open_connection(client.host, client.port)
192+
writer.write(b"GET /my_symlink/symlink_target_file HTTP/1.1\r\n\r\n")
193+
response = await reader.readuntil(b"\r\n\r\n")
194+
assert b"200 OK" in response
195+
response = await reader.readuntil(b"readable")
196+
assert response == b"readable"
197+
writer.close()
198+
await writer.wait_closed()
199+
await client.close()
200+
201+
111202
@pytest.mark.parametrize(
112203
"dir_name,filename,data",
113204
[

0 commit comments

Comments
 (0)