Skip to content

Commit 000ed85

Browse files
zzzeekzzzeek
committed
Fix path traversal via drive designator URI on Windows in TemplateLookup
The directory traversal check in Template.__init__ normalized the URI using os.path, which on Windows is ntpath, while TemplateLookup. get_template() resolves the URI to a file using posixpath. The two modules disagree about a drive designator: posixpath has no notion of one and treats "C:" as an ordinary path component, so the ".." segments of a URI such as "C:/../../secret.txt" escape the template root, while ntpath splits the drive off, treats the remainder as rooted and absorbs those same ".." segments, leaving nothing for the check to inspect. The check therefore could not fire for any URI beginning with a drive designator, and arbitrary readable files outside of the template directory could be loaded. Normalization in Template.__init__ is now performed with posixpath, the same module get_template() already commits to, so that both sides of the operation agree on what the URI means. This is a third instance of the same posixpath/os.path mismatch behind the fixes for sqlalchemy#434 and sqlalchemy#435; both of those remain effective on their own terms and neither covers this form, as each normalized a separator spelling rather than the drive designator. The per-form traversal tests added for sqlalchemy#434 and sqlalchemy#435 are consolidated into a single parametrized test in each of test_lookup.py and test_template.py, running every spelling of the traversal against both posixpath and ntpath. All three forms are then covered by one matrix, and reverting any one of the three fixes fails only the rows belonging to it. Fixes: sqlalchemy#441 Change-Id: I7ad8cc85d0514ff4120b5ace28010a6770070a8a
1 parent 411b4ac commit 000ed85

4 files changed

Lines changed: 148 additions & 85 deletions

File tree

‎doc/build/unreleased/441.rst‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
.. change::
2+
:tags: bug, template
3+
:tickets: 441
4+
5+
Fixed issue in :class:`.TemplateLookup` where a URI beginning with a drive
6+
designator (e.g. ``C:/../../secret.txt``) could bypass the directory
7+
traversal check on Windows, allowing reads of arbitrary files outside of
8+
the template directory. The check in :class:`.Template` normalized the URI
9+
using ``os.path``, which on Windows is ``ntpath``; as ``ntpath`` splits the
10+
drive designator off and treats the remainder as rooted, the ``..``
11+
segments were absorbed before the check could inspect them. Normalization
12+
is now performed with ``posixpath``, which is the same module used by
13+
:meth:`.TemplateLookup.get_template` to resolve the URI to a file.

‎mako/template.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
from importlib import machinery
1313
import json
1414
import os
15+
import posixpath
1516
import re
1617
import shutil
1718
import stat
@@ -265,7 +266,12 @@ def __init__(
265266
self.uri = self.module_id
266267

267268
u_norm = self.uri.replace("\\", "/").lstrip("/")
268-
u_norm = os.path.normpath(u_norm)
269+
# use posixpath, not os.path, to normalize; this is the same
270+
# module used by TemplateLookup to resolve the URI to a file,
271+
# and unlike ntpath it has no notion of a drive designator, so
272+
# a URI like "C:/../../secret.txt" retains its ".." segments
273+
# here rather than having them absorbed into a drive root.
274+
u_norm = posixpath.normpath(u_norm)
269275
if u_norm.startswith(".."):
270276
raise exceptions.TemplateLookupException(
271277
'Template uri "%s" is invalid - '

‎test/test_lookup.py‎

Lines changed: 66 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,18 @@
1+
import contextlib
2+
import ntpath
13
import os
4+
import posixpath
25
import tempfile
36

7+
import pytest
8+
49
from mako import exceptions
510
from mako import lookup
611
from mako import runtime
712
from mako.template import Template
813
from mako.testing.assertions import assert_raises_message
914
from mako.testing.assertions import assert_raises_with_given_cause
15+
from mako.testing.assertions import eq_
1016
from mako.testing.config import config
1117
from mako.testing.helpers import file_with_template_code
1218
from mako.testing.helpers import replace_file_with_dir
@@ -127,50 +133,64 @@ def test_dont_accept_relative_outside_of_root(self):
127133
# this is OK since the .. cancels out
128134
runtime._lookup_template(ctx, "foo/../index.html", index.uri)
129135

130-
def test_dont_accept_relative_outside_of_root_via_double_slash(self):
131-
"""test that double-slash URI prefix can't bypass the
132-
path traversal check"""
133-
with tempfile.TemporaryDirectory() as base:
134-
tmpl_dir = os.path.join(base, "app", "templates")
135-
os.makedirs(tmpl_dir)
136-
with open(os.path.join(tmpl_dir, "index.html"), "w") as f:
137-
f.write("Hello")
138-
139-
secret = os.path.join(base, "secrets", "creds.txt")
140-
os.makedirs(os.path.dirname(secret))
141-
with open(secret, "w") as f:
142-
f.write("SECRET_KEY=supersecret123")
143-
136+
@pytest.mark.parametrize(
137+
"ospath", [posixpath, ntpath], ids=["posix", "windows"]
138+
)
139+
@pytest.mark.parametrize(
140+
"uri",
141+
[
142+
# plain relative traversal
143+
"../../secrets/creds.txt",
144+
"/../../secrets/creds.txt",
145+
# leading slash prefixes; #434
146+
"//../../secrets/creds.txt",
147+
"///../../secrets/creds.txt",
148+
# backslash separators; #435
149+
"..\\..\\secrets\\creds.txt",
150+
"\\..\\..\\secrets\\creds.txt",
151+
# drive designators; #441. the drive designator itself
152+
# consumes one level of the posixpath resolution, hence the
153+
# extra ".." relative to the forms above
154+
"C:/../../../secrets/creds.txt",
155+
"c:/../../../secrets/creds.txt",
156+
"C:\\..\\..\\..\\secrets\\creds.txt",
157+
"/C:/../../../secrets/creds.txt",
158+
"//C:/../../../secrets/creds.txt",
159+
],
160+
)
161+
def test_dont_accept_traversal_outside_of_root(self, uri, ospath):
162+
"""test that no spelling of a traversal URI can bypass the
163+
path traversal check, on either platform.
164+
165+
The URIs above are all spellings of the same traversal, and all
166+
must be refused whether ``os.path`` is ``posixpath`` or
167+
``ntpath``; the backslash and drive designator forms are only
168+
distinguishable from a plain traversal under ``ntpath``.
169+
170+
"""
171+
with self._traversal_fixture() as tmpl_dir:
144172
tl = lookup.TemplateLookup(directories=[tmpl_dir])
145-
rel = os.path.relpath(secret, tmpl_dir)
146-
147-
# single-slash prefix should also be blocked
148-
assert_raises_message(
149-
exceptions.TemplateLookupException,
150-
"cannot be relative outside of the root path",
151-
tl.get_template,
152-
"/" + rel,
153-
)
154173

155-
# double-slash prefix must not bypass the check
156-
assert_raises_message(
157-
exceptions.TemplateLookupException,
158-
"cannot be relative outside of the root path",
159-
tl.get_template,
160-
"//" + rel,
161-
)
174+
current_path = os.path
175+
os.path = ospath
176+
try:
177+
assert_raises_message(
178+
exceptions.TemplateLookupException,
179+
"cannot be relative outside of the root path",
180+
tl.get_template,
181+
uri,
182+
)
183+
184+
# a template within the root still resolves
185+
eq_(tl.get_template("index.html").render(), "Hello")
186+
finally:
187+
os.path = current_path
188+
189+
@contextlib.contextmanager
190+
def _traversal_fixture(self):
191+
"""set up a template directory with a file to be reached outside
192+
of it, laid out to match the literal URIs used above."""
162193

163-
# triple-slash prefix must not bypass the check
164-
assert_raises_message(
165-
exceptions.TemplateLookupException,
166-
"cannot be relative outside of the root path",
167-
tl.get_template,
168-
"///" + rel,
169-
)
170-
171-
def test_dont_accept_relative_outside_of_root_via_backslash(self):
172-
"""test that backslash traversal URI can't bypass the
173-
path traversal check"""
174194
with tempfile.TemporaryDirectory() as base:
175195
tmpl_dir = os.path.join(base, "app", "templates")
176196
os.makedirs(tmpl_dir)
@@ -182,31 +202,13 @@ def test_dont_accept_relative_outside_of_root_via_backslash(self):
182202
with open(secret, "w") as f:
183203
f.write("SECRET_KEY=supersecret123")
184204

185-
tl = lookup.TemplateLookup(directories=[tmpl_dir])
186-
rel = os.path.relpath(secret, tmpl_dir).replace("/", "\\")
187-
188-
assert_raises_message(
189-
exceptions.TemplateLookupException,
190-
"cannot be relative outside of the root path",
191-
tl.get_template,
192-
rel,
205+
# keep the literal URIs above honest
206+
eq_(
207+
os.path.relpath(secret, tmpl_dir).replace(os.sep, "/"),
208+
"../../secrets/creds.txt",
193209
)
194210

195-
# with leading backslash
196-
assert_raises_message(
197-
exceptions.TemplateLookupException,
198-
"cannot be relative outside of the root path",
199-
tl.get_template,
200-
"\\" + rel,
201-
)
202-
203-
# with leading forward slash
204-
assert_raises_message(
205-
exceptions.TemplateLookupException,
206-
"cannot be relative outside of the root path",
207-
tl.get_template,
208-
"/" + rel,
209-
)
211+
yield tmpl_dir
210212

211213
def test_checking_against_bad_filetype(self):
212214
with tempfile.TemporaryDirectory() as tempdir:

‎test/test_template.py‎

Lines changed: 62 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1+
import ntpath
12
import os
3+
import posixpath
4+
import re
25

36
import pytest
47

@@ -1463,28 +1466,67 @@ def _compile_from_file(self, path, filename):
14631466
finally:
14641467
os.path = current_path
14651468

1466-
def test_dont_accept_relative_outside_of_root(self):
1467-
assert_raises_message(
1468-
exceptions.TemplateLookupException,
1469-
'Template uri "../../foo.html" is invalid - it '
1470-
"cannot be relative outside of the root path",
1471-
Template,
1472-
"test",
1473-
uri="../../foo.html",
1474-
)
1469+
@pytest.mark.parametrize(
1470+
"ospath", [posixpath, ntpath], ids=["posix", "windows"]
1471+
)
1472+
@pytest.mark.parametrize(
1473+
"uri",
1474+
[
1475+
# plain relative traversal
1476+
"../../foo.html",
1477+
"/../../foo.html",
1478+
# leading slash prefixes; #434
1479+
"//../../foo.html",
1480+
"///../../foo.html",
1481+
# backslash separators; #435
1482+
"..\\..\\foo.html",
1483+
"\\..\\..\\foo.html",
1484+
# drive designators; #441
1485+
"C:/../../foo.html",
1486+
"c:/../../foo.html",
1487+
"C:\\..\\..\\foo.html",
1488+
"/C:/../../foo.html",
1489+
"//C:/../../foo.html",
1490+
],
1491+
)
1492+
def test_dont_accept_relative_outside_of_root(self, uri, ospath):
1493+
"""test that no spelling of a traversal URI passes the check in
1494+
:class:`.Template`, on either platform."""
14751495

1476-
assert_raises_message(
1477-
exceptions.TemplateLookupException,
1478-
'Template uri "/../../foo.html" is invalid - it '
1479-
"cannot be relative outside of the root path",
1480-
Template,
1481-
"test",
1482-
uri="/../../foo.html",
1483-
)
1496+
current_path = os.path
1497+
os.path = ospath
1498+
try:
1499+
# assert_raises_message matches as a regexp, so the
1500+
# backslash forms have to be escaped
1501+
assert_raises_message(
1502+
exceptions.TemplateLookupException,
1503+
re.escape(
1504+
'Template uri "%s" is invalid - it '
1505+
"cannot be relative outside of the root path" % uri
1506+
),
1507+
Template,
1508+
"test",
1509+
uri=uri,
1510+
)
1511+
finally:
1512+
os.path = current_path
14841513

1485-
# normalizes in the root is OK
1486-
t = Template("test", uri="foo/bar/../../foo.html")
1487-
eq_(t.uri, "foo/bar/../../foo.html")
1514+
@pytest.mark.parametrize(
1515+
"ospath", [posixpath, ntpath], ids=["posix", "windows"]
1516+
)
1517+
@pytest.mark.parametrize(
1518+
"uri", ["foo/bar/../../foo.html", "C:/foo/../foo.html"]
1519+
)
1520+
def test_accept_relative_normalizing_inside_root(self, uri, ospath):
1521+
"""test that a traversal which cancels out within the root is
1522+
still accepted, including below a drive designator."""
1523+
1524+
current_path = os.path
1525+
os.path = ospath
1526+
try:
1527+
eq_(Template("test", uri=uri).uri, uri)
1528+
finally:
1529+
os.path = current_path
14881530

14891531

14901532
class ModuleTemplateTest(TemplateTest):

0 commit comments

Comments
 (0)