Mercurial > p > roundup > code

comparison roundup/cgi/actions.py @ 5161:12190efa30d4

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
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.
author John Rouillard <rouilj@ieee.org>
date Sat, 23 Jul 2016 14:00:49 -0400
parents 63294ed25e84
children 3ee79a2d95d4
comparison
equal deleted inserted replaced
5160:f8a32b7331f1 5161:12190efa30d4
37 37
38 def execute(self): 38 def execute(self):
39 """Execute the action specified by this object.""" 39 """Execute the action specified by this object."""
40 self.permission() 40 self.permission()
41 return self.handle() 41 return self.handle()
42
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))
42 135
43 name = '' 136 name = ''
44 permissionType = None 137 permissionType = None
45 def permission(self): 138 def permission(self):
46 """Check whether the user has permission to execute this action. 139 """Check whether the user has permission to execute this action.
727 self.db.commit() 820 self.db.commit()
728 821
729 # Allow an option to stay on the page to create new things 822 # Allow an option to stay on the page to create new things
730 if '__redirect_to' in self.form: 823 if '__redirect_to' in self.form:
731 raise exceptions.Redirect('%s&@ok_message=%s'%( 824 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)))
733 827
734 # otherwise redirect to the new item's page 828 # otherwise redirect to the new item's page
735 raise exceptions.Redirect('%s%s%s?@ok_message=%s&@template=%s' % ( 829 raise exceptions.Redirect('%s%s%s?@ok_message=%s&@template=%s' % (
736 self.base, self.classname, self.nodeid, urllib_.quote(messages), 830 self.base, self.classname, self.nodeid, urllib_.quote(messages),
737 urllib_.quote(self.template))) 831 urllib_.quote(self.template)))
1044 # 2. Split the query string into parts. 1138 # 2. Split the query string into parts.
1045 # 3. Delete @error_message and @ok_message if present. 1139 # 3. Delete @error_message and @ok_message if present.
1046 # 4. Define a new redirect_url missing the @...message entries. 1140 # 4. Define a new redirect_url missing the @...message entries.
1047 # This will be redefined if there is a login error to include 1141 # This will be redefined if there is a login error to include
1048 # a new error message 1142 # 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)
1050 # now I have a tuple form for the __came_from url 1146 # now I have a tuple form for the __came_from url
1051 try: 1147 try:
1052 query=urllib_.parse_qs(redirect_url_tuple.query) 1148 query=urllib_.parse_qs(redirect_url_tuple.query)
1053 if "@error_message" in query: 1149 if "@error_message" in query:
1054 del query["@error_message"] 1150 del query["@error_message"]
Roundup Issue Tracker: http://roundup-tracker.org/