Skip to content

Commit 9118a58

Browse files
[PR #8079/1c335944 backport][3.9] Validate static paths (#8080)
**This is a backport of PR #8079 as merged into master (1c33594).**
1 parent 435ad46 commit 9118a58

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
@@ -595,9 +595,14 @@ def url_for( # type: ignore[override]
595595
url = url / filename
596596

597597
if append_version:
598+
unresolved_path = self._directory.joinpath(filename)
598599
try:
599-
filepath = self._directory.joinpath(filename).resolve()
600-
if not self._follow_symlinks:
600+
if self._follow_symlinks:
601+
normalized_path = Path(os.path.normpath(unresolved_path))
602+
normalized_path.relative_to(self._directory)
603+
filepath = normalized_path.resolve()
604+
else:
605+
filepath = unresolved_path.resolve()
601606
filepath.relative_to(self._directory)
602607
except (ValueError, FileNotFoundError):
603608
# ValueError for case when path point to symlink
@@ -662,8 +667,13 @@ async def _handle(self, request: Request) -> StreamResponse:
662667
# /static/\\machine_name\c$ or /static/D:\path
663668
# where the static dir is totally different
664669
raise HTTPForbidden()
665-
filepath = self._directory.joinpath(filename).resolve()
666-
if not self._follow_symlinks:
670+
unresolved_path = self._directory.joinpath(filename)
671+
if self._follow_symlinks:
672+
normalized_path = Path(os.path.normpath(unresolved_path))
673+
normalized_path.relative_to(self._directory)
674+
filepath = normalized_path.resolve()
675+
else:
676+
filepath = unresolved_path.resolve()
667677
filepath.relative_to(self._directory)
668678
except (ValueError, FileNotFoundError) as error:
669679
# relatively safe

docs/web_advanced.rst

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

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

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

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

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

docs/web_reference.rst

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

1878-
:param bool follow_symlinks: flag for allowing to follow symlinks from
1879-
a directory, by default it's not allowed and
1880-
HTTP/404 will be returned on access.
1878+
:param bool follow_symlinks: flag for allowing to follow symlinks that lead
1879+
outside the static root directory, by default it's not allowed and
1880+
HTTP/404 will be returned on access. Enabling ``follow_symlinks``
1881+
can be a security risk, and may lead to a directory transversal attack.
1882+
You do NOT need this option to follow symlinks which point to somewhere
1883+
else within the static directory, this option is only used to break out
1884+
of the security sandbox. Enabling this option is highly discouraged,
1885+
and only expected to be used for edge cases in a local development
1886+
setting where remote users do not have access to the server.
18811887

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

tests/test_web_urldispatcher.py

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

132132

133+
async def test_follow_symlink_directory_traversal(
134+
tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
135+
) -> None:
136+
# Tests that follow_symlinks does not allow directory transversal
137+
data = "private"
138+
139+
private_file = tmp_path / "private_file"
140+
private_file.write_text(data)
141+
142+
safe_path = tmp_path / "safe_dir"
143+
safe_path.mkdir()
144+
145+
app = web.Application()
146+
147+
# Register global static route:
148+
app.router.add_static("/", str(safe_path), follow_symlinks=True)
149+
client = await aiohttp_client(app)
150+
151+
await client.start_server()
152+
# We need to use a raw socket to test this, as the client will normalize
153+
# the path before sending it to the server.
154+
reader, writer = await asyncio.open_connection(client.host, client.port)
155+
writer.write(b"GET /../private_file HTTP/1.1\r\n\r\n")
156+
response = await reader.readuntil(b"\r\n\r\n")
157+
assert b"404 Not Found" in response
158+
writer.close()
159+
await writer.wait_closed()
160+
await client.close()
161+
162+
163+
async def test_follow_symlink_directory_traversal_after_normalization(
164+
tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
165+
) -> None:
166+
# Tests that follow_symlinks does not allow directory transversal
167+
# after normalization
168+
#
169+
# Directory structure
170+
# |-- secret_dir
171+
# | |-- private_file (should never be accessible)
172+
# | |-- symlink_target_dir
173+
# | |-- symlink_target_file (should be accessible via the my_symlink symlink)
174+
# | |-- sandbox_dir
175+
# | |-- my_symlink -> symlink_target_dir
176+
#
177+
secret_path = tmp_path / "secret_dir"
178+
secret_path.mkdir()
179+
180+
# This file is below the symlink target and should not be reachable
181+
private_file = secret_path / "private_file"
182+
private_file.write_text("private")
183+
184+
symlink_target_path = secret_path / "symlink_target_dir"
185+
symlink_target_path.mkdir()
186+
187+
sandbox_path = symlink_target_path / "sandbox_dir"
188+
sandbox_path.mkdir()
189+
190+
# This file should be reachable via the symlink
191+
symlink_target_file = symlink_target_path / "symlink_target_file"
192+
symlink_target_file.write_text("readable")
193+
194+
my_symlink_path = sandbox_path / "my_symlink"
195+
pathlib.Path(str(my_symlink_path)).symlink_to(str(symlink_target_path), True)
196+
197+
app = web.Application()
198+
199+
# Register global static route:
200+
app.router.add_static("/", str(sandbox_path), follow_symlinks=True)
201+
client = await aiohttp_client(app)
202+
203+
await client.start_server()
204+
# We need to use a raw socket to test this, as the client will normalize
205+
# the path before sending it to the server.
206+
reader, writer = await asyncio.open_connection(client.host, client.port)
207+
writer.write(b"GET /my_symlink/../private_file HTTP/1.1\r\n\r\n")
208+
response = await reader.readuntil(b"\r\n\r\n")
209+
assert b"404 Not Found" in response
210+
writer.close()
211+
await writer.wait_closed()
212+
213+
reader, writer = await asyncio.open_connection(client.host, client.port)
214+
writer.write(b"GET /my_symlink/symlink_target_file HTTP/1.1\r\n\r\n")
215+
response = await reader.readuntil(b"\r\n\r\n")
216+
assert b"200 OK" in response
217+
response = await reader.readuntil(b"readable")
218+
assert response == b"readable"
219+
writer.close()
220+
await writer.wait_closed()
221+
await client.close()
222+
223+
133224
@pytest.mark.parametrize(
134225
"dir_name,filename,data",
135226
[

0 commit comments

Comments
 (0)