1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
|
From 1c335944d6a8b1298baf179b7c0b3069f10c514b
From: Sam Bull <git@sambull.org>
Date: Sun Jan 28 18:13:06 2024 +0000
Subject: [PATCH] python3-aiohttp: Validate static paths (#8079)
Co-authored-by: J. Nick Koston <nick@koston.org>
CVE: CVE-2024-23334
Upstream-Status: Backport [https://github.com/aio-libs/aiohttp/commit/1c335944d6a8b1298baf179b7c0b3069f10c514b]
Signed-off-by: Rahul Janani Pandi <RahulJanani.Pandi@windriver.com>
---
CHANGES/8079.bugfix.rst | 1 +
aiohttp/web_urldispatcher.py | 18 +++++--
docs/web_advanced.rst | 16 ++++--
docs/web_reference.rst | 12 +++--
tests/test_web_urldispatcher.py | 91 +++++++++++++++++++++++++++++++++
5 files changed, 128 insertions(+), 10 deletions(-)
create mode 100644 CHANGES/8079.bugfix.rst
diff --git a/CHANGES/8079.bugfix.rst b/CHANGES/8079.bugfix.rst
new file mode 100644
index 0000000..57bc8bf
--- /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 5942e35..e8a8023 100644
--- a/aiohttp/web_urldispatcher.py
+++ b/aiohttp/web_urldispatcher.py
@@ -593,9 +593,14 @@ class StaticResource(PrefixResource):
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
@@ -660,8 +665,13 @@ class StaticResource(PrefixResource):
# /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
diff --git a/docs/web_advanced.rst b/docs/web_advanced.rst
index 3a98b78..5129397 100644
--- a/docs/web_advanced.rst
+++ b/docs/web_advanced.rst
@@ -136,12 +136,22 @@ 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``
diff --git a/docs/web_reference.rst b/docs/web_reference.rst
index a156f47..b100676 100644
--- a/docs/web_reference.rst
+++ b/docs/web_reference.rst
@@ -1836,9 +1836,15 @@ Router is any object that implements :class:`~aiohttp.abc.AbstractRouter` interf
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
diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py
index f24f451..f40f6a5 100644
--- a/tests/test_web_urldispatcher.py
+++ b/tests/test_web_urldispatcher.py
@@ -123,6 +123,97 @@ async def test_follow_symlink(tmp_dir_path, aiohttp_client) -> None:
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",
[
--
2.40.0
|
