Skip to content

Commit 002951b

Browse files
rouiljrouilj
committed
I realized that the __came_from and __redirect_to url parameters I
added to handle issues with the LoginAction and NewItemAction could be used for XSS or other purposes. So I check them using a new clean_url(url) function. This tries to validate that the url is under the tracker's base url and that the components of the url are properly url encoded. If it thinks something is wrong with the url, it will raise a ValueError. I decided to not attempt to fix the url's if there is an issue, better to bring it to the tracker admin's attention. Changed the code paths in NewItemAction and LoginAction that deal with the form parameters to use the clean_url function on the form input first.
1 parent 122d991 commit 002951b

File tree

4 files changed

+216
-8
lines changed

4 files changed

+216
-8
lines changed

‎doc/customizing.txt‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1718,10 +1718,13 @@ None of the above (ie. just a simple form value)
17181718
Any of the form variables may be prefixed with a classname or
17191719
designator.
17201720

1721-
Setting the form variable: ``__redirect_to=`` to a url when @action=new
1722-
redirects the user to the specified url after successfully creating
1723-
the new item. This is useful if you want the user to create another
1724-
item rather than edit the newly created item.
1721+
Setting the form variable: ``__redirect_to=`` to a url when
1722+
@action=new redirects the user to the specified url after successfully
1723+
creating the new item. This is useful if you want the user to create
1724+
another item rather than edit the newly created item. Note that the
1725+
url assigned to ``__redirect_to`` must be url encoded/quoted and be
1726+
under the tracker's base url. If the base_url uses http, you can set
1727+
the url to https.
17251728

17261729
Two special form values are supported for backwards compatibility:
17271730

‎doc/upgrading.txt‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,10 @@ Login from a search or after logout works better
130130
The login form has been improved to work with some back end code
131131
changes. Now when a user logs in they stay on the same page where they
132132
started the login. To make this work, you must change the tal that is
133-
used to set the ``__came_from`` form variable.
133+
used to set the ``__came_from`` form variable. Note that the url
134+
assigned to __came_from must be url encoded/quoted and be under the
135+
tracker's base url. If the base_url uses http, you can set the url to
136+
https.
134137

135138
Replace the existing code in the tracker's html/page.html page that
136139
looks similar to (look for name="__came_from")::
@@ -200,7 +203,9 @@ The key component here is support for the '__redirect_to' query
200203
property. It is a url which can be used when creating any new item
201204
(issue, user, keyword ....). It controls the next page displayed after
202205
creating the item. If '__redirect_to' is not set, then you end up on
203-
the page for the newly created item.
206+
the page for the newly created item. The url value assigned to
207+
__redirect_to must be under the tracker's base url and must be properly
208+
url encoded.
204209

205210
html/_generic.404.html in trackers use page template
206211
----------------------------------------------------

‎roundup/cgi/actions.py‎

Lines changed: 98 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,99 @@ def execute(self):
4040
self.permission()
4141
return self.handle()
4242

43+
def clean_url(self, url):
44+
'''Return URL validated to be under self.base and properly escaped
45+
46+
If url not properly escaped or validation fails raise ValueError.
47+
48+
To try to prevent XSS attacks, validate that the url that is
49+
passed in is under self.base for the tracker. This is used to
50+
clean up "__came_from" and "__redirect_to" form variables used
51+
by the LoginAction and NewItemAction actions.
52+
53+
The url that is passed in must be a properly url quoted
54+
argument. I.E. all characters that are not valid according to
55+
RFC3986 must be % encoded. Schema should be lower case.
56+
57+
It parses the passed url into components.
58+
59+
It verifies that the scheme is http or https (so a redirect can
60+
force https even if normal access to the tracker is via http).
61+
Validates that the network component is the same as in self.base.
62+
Validates that the path component in the base url starts the url's
63+
path component. It not it raises ValueError. If everything
64+
validates:
65+
66+
For each component, Appendix A of RFC 3986 says the following
67+
are allowed:
68+
69+
pchar = unreserved / pct-encoded / sub-delims / ":" / "@"
70+
query = *( pchar / "/" / "?" )
71+
unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
72+
pct-encoded = "%" HEXDIG HEXDIG
73+
sub-delims = "!" / "$" / "&" / "'" / "(" / ")"
74+
/ "*" / "+" / "," / ";" / "="
75+
76+
Checks all parts with a regexp that matches any run of 0 or
77+
more allowed characters. If the component doesn't validate,
78+
raise ValueError. Don't attempt to urllib_.quote it. Either
79+
it's correct as it comes in or it's a ValueError.
80+
81+
Finally paste the whole thing together and return the new url.
82+
'''
83+
84+
parsed_url_tuple = urllib_.urlparse(url)
85+
parsed_base_url_tuple = urllib_.urlparse(self.base)
86+
87+
info={ 'url': url,
88+
'base_url': self.base,
89+
'base_scheme': parsed_base_url_tuple.scheme,
90+
'base_netloc': parsed_base_url_tuple.netloc,
91+
'base_path': parsed_base_url_tuple.path,
92+
'url_scheme': parsed_url_tuple.scheme,
93+
'url_netloc': parsed_url_tuple.netloc,
94+
'url_path': parsed_url_tuple.path,
95+
'url_params': parsed_url_tuple.params,
96+
'url_query': parsed_url_tuple.query,
97+
'url_fragment': parsed_url_tuple.fragment }
98+
99+
if parsed_base_url_tuple.scheme == "https":
100+
if parsed_url_tuple.scheme != "https":
101+
raise ValueError(self._("Base url %(base_url)s requires https. Redirect url %(url)s uses http.")%info)
102+
else:
103+
if parsed_url_tuple.scheme not in ('http', 'https'):
104+
raise ValueError(self._("Unrecognized scheme in %(url)s")%info)
105+
106+
if parsed_url_tuple.netloc <> parsed_base_url_tuple.netloc:
107+
raise ValueError(self._("Net location in %(url)s does not match base: %(base_netloc)s")%info)
108+
109+
if parsed_url_tuple.path.find(parsed_base_url_tuple.path) <> 0:
110+
raise ValueError(self._("Base path %(base_path)s is not a prefix for url %(url)s")%info)
111+
112+
# I am not sure if this has to be language sensitive.
113+
# Do ranges depend on the LANG of the user??
114+
# Is there a newer spec for URI's than what I am referencing?
115+
116+
# Also it really should be % HEXDIG HEXDIG that's allowed
117+
# If %%% passes, the roundup server should be able to ignore/
118+
# quote it so it doesn't do anything bad otherwise we have a
119+
# different vector to handle.
120+
allowed_pattern = re.compile(r'''^[A-Za-z0-9@:/?._~%!$&'()*+,;=-]*$''')
121+
122+
if not allowed_pattern.match(parsed_url_tuple.path):
123+
raise ValueError(self._("Path component (%(url_path)s) in %(url)s is not properly escaped")%info)
124+
125+
if not allowed_pattern.match(parsed_url_tuple.params):
126+
raise ValueError(self._("Params component (%(url_params)s) in %(url)s is not properly escaped")%info)
127+
128+
if not allowed_pattern.match(parsed_url_tuple.query):
129+
raise ValueError(self._("Query component (%(url_query)s) in %(url)s is not properly escaped")%info)
130+
131+
if not allowed_pattern.match(parsed_url_tuple.fragment):
132+
raise ValueError(self._("Fragment component (%(url_fragment)s) in %(url)s is not properly escaped")%info)
133+
134+
return(urllib_.urlunparse(parsed_url_tuple))
135+
43136
name = ''
44137
permissionType = None
45138
def permission(self):
@@ -729,7 +822,8 @@ def handle(self):
729822
# Allow an option to stay on the page to create new things
730823
if '__redirect_to' in self.form:
731824
raise exceptions.Redirect('%s&@ok_message=%s'%(
732-
self.form['__redirect_to'].value, urllib_.quote(messages)))
825+
self.clean_url(self.form['__redirect_to'].value),
826+
urllib_.quote(messages)))
733827

734828
# otherwise redirect to the new item's page
735829
raise exceptions.Redirect('%s%s%s?@ok_message=%s&@template=%s' % (
@@ -1046,7 +1140,9 @@ def handle(self):
10461140
# 4. Define a new redirect_url missing the @...message entries.
10471141
# This will be redefined if there is a login error to include
10481142
# a new error message
1049-
redirect_url_tuple = urllib_.urlparse(self.form['__came_from'].value)
1143+
1144+
clean_url = self.clean_url(self.form['__came_from'].value)
1145+
redirect_url_tuple = urllib_.urlparse(clean_url)
10501146
# now I have a tuple form for the __came_from url
10511147
try:
10521148
query=urllib_.parse_qs(redirect_url_tuple.query)

‎test/test_cgi.py‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1173,6 +1173,110 @@ def testrenderContext(self):
11731173
self.assertNotEqual(-1, result.index('ok message'))
11741174
# print result
11751175

1176+
def testclean_url(self):
1177+
''' test the clean_url function '''
1178+
action = actions.Action(self.client)
1179+
clean_url = action.clean_url
1180+
1181+
# Christmas tree url: test of every component that passes
1182+
self.assertEqual(
1183+
clean_url("http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1184+
'http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue')
1185+
1186+
# allow replacing http with https if base is http
1187+
self.assertEqual(
1188+
clean_url("https://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1189+
'https://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue')
1190+
1191+
1192+
# change base to use https and make sure we don't redirect to http
1193+
saved_base = action.base
1194+
action.base = "https://tracker.example/cgi-bin/roundup.cgi/bugs/"
1195+
with self.assertRaises(ValueError) as cm:
1196+
clean_url("http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue")
1197+
self.assertEqual(cm.exception.message, 'Base url https://tracker.example/cgi-bin/roundup.cgi/bugs/ requires https. Redirect url http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue uses http.')
1198+
action.base = saved_base
1199+
1200+
# url doesn't have to be valid to roundup, just has to be contained
1201+
# inside of roundup. No zoik class is defined
1202+
self.assertEqual(clean_url("http://tracker.example/cgi-bin/roundup.cgi/bugs/zoik7;parm=bar?@template=foo&parm=(zot)#issue"), "http://tracker.example/cgi-bin/roundup.cgi/bugs/zoik7;parm=bar?@template=foo&parm=(zot)#issue")
1203+
1204+
# test with wonky schemes
1205+
with self.assertRaises(ValueError) as cm:
1206+
clean_url("email://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1207+
self.assertEqual(cm.exception.message, 'Unrecognized scheme in email://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue')
1208+
1209+
with self.assertRaises(ValueError) as cm:
1210+
clean_url("http%3a//tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1211+
self.assertEqual(cm.exception.message, 'Unrecognized scheme in http%3a//tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue')
1212+
1213+
# test different netloc/path prefix
1214+
# assert port
1215+
with self.assertRaises(ValueError) as cm:
1216+
clean_url("http://tracker.example:1025/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1217+
self.assertEqual(cm.exception.message, 'Net location in http://tracker.example:1025/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue does not match base: tracker.example')
1218+
1219+
#assert user
1220+
with self.assertRaises(ValueError) as cm:
1221+
clean_url("http://[email protected]/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1222+
self.assertEqual(cm.exception.message, 'Net location in http://[email protected]/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue does not match base: tracker.example')
1223+
1224+
#assert user:password
1225+
with self.assertRaises(ValueError) as cm:
1226+
clean_url("http://user:[email protected]/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1227+
self.assertEqual(cm.exception.message, 'Net location in http://user:[email protected]/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue does not match base: tracker.example')
1228+
1229+
# try localhost http scheme
1230+
with self.assertRaises(ValueError) as cm:
1231+
clean_url("http://localhost/cgi-bin/roundup.cgi/bugs/user3")
1232+
self.assertEqual(cm.exception.message, 'Net location in http://localhost/cgi-bin/roundup.cgi/bugs/user3 does not match base: tracker.example')
1233+
1234+
# try localhost https scheme
1235+
with self.assertRaises(ValueError) as cm:
1236+
clean_url("https://localhost/cgi-bin/roundup.cgi/bugs/user3")
1237+
self.assertEqual(cm.exception.message, 'Net location in https://localhost/cgi-bin/roundup.cgi/bugs/user3 does not match base: tracker.example')
1238+
1239+
# try different host
1240+
with self.assertRaises(ValueError) as cm:
1241+
clean_url("http://bad.guys.are.us/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1242+
self.assertEqual(cm.exception.message, 'Net location in http://bad.guys.are.us/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#issue does not match base: tracker.example')
1243+
1244+
# change the base path to .../bug from .../bugs
1245+
with self.assertRaises(ValueError) as cm:
1246+
clean_url("http://tracker.example/cgi-bin/roundup.cgi/bug/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1247+
self.assertEqual(cm.exception.message, 'Base path /cgi-bin/roundup.cgi/bugs/ is not a prefix for url http://tracker.example/cgi-bin/roundup.cgi/bug/user3;parm=bar?@template=foo&parm=(zot)#issue')
1248+
1249+
# change the base path eliminate - in cgi-bin
1250+
with self.assertRaises(ValueError) as cm:
1251+
clean_url("http://tracker.example/cgibin/roundup.cgi/bug/user3;parm=bar?@template=foo&parm=(zot)#issue"),
1252+
self.assertEqual(cm.exception.message, 'Base path /cgi-bin/roundup.cgi/bugs/ is not a prefix for url http://tracker.example/cgibin/roundup.cgi/bug/user3;parm=bar?@template=foo&parm=(zot)#issue')
1253+
1254+
1255+
# scan for unencoded characters
1256+
# we skip schema and net location since unencoded character
1257+
# are allowed only by an explicit match to a reference.
1258+
#
1259+
# break components with unescaped character '<'
1260+
# path component
1261+
with self.assertRaises(ValueError) as cm:
1262+
clean_url("http://tracker.example/cgi-bin/roundup.cgi/bugs/<user3;parm=bar?@template=foo&parm=(zot)#issue"),
1263+
self.assertEqual(cm.exception.message, 'Path component (/cgi-bin/roundup.cgi/bugs/<user3) in http://tracker.example/cgi-bin/roundup.cgi/bugs/<user3;parm=bar?@template=foo&parm=(zot)#issue is not properly escaped')
1264+
1265+
# params component
1266+
with self.assertRaises(ValueError) as cm:
1267+
clean_url("http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=b<ar?@template=foo&parm=(zot)#issue"),
1268+
self.assertEqual(cm.exception.message, 'Params component (parm=b<ar) in http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=b<ar?@template=foo&parm=(zot)#issue is not properly escaped')
1269+
1270+
# query component
1271+
with self.assertRaises(ValueError) as cm:
1272+
clean_url("http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=<foo>&parm=(zot)#issue"),
1273+
self.assertEqual(cm.exception.message, 'Query component (@template=<foo>&parm=(zot)) in http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=<foo>&parm=(zot)#issue is not properly escaped')
1274+
1275+
# fragment component
1276+
with self.assertRaises(ValueError) as cm:
1277+
clean_url("http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#iss<ue"),
1278+
self.assertEqual(cm.exception.message, 'Fragment component (iss<ue) in http://tracker.example/cgi-bin/roundup.cgi/bugs/user3;parm=bar?@template=foo&parm=(zot)#iss<ue is not properly escaped')
1279+
11761280
class TemplateTestCase(unittest.TestCase):
11771281
''' Test the template resolving code, i.e. what can be given to @template
11781282
'''

0 commit comments

Comments
 (0)