Mercurial > p > roundup > code
comparison roundup/cgi/client.py @ 5210:7da56980754d
Remove csrf keys used with get
The original code didn't delete keys used with a get. Detect this and
invalidate the keys. Get keys are more likely to leak (in urls etc.)
so they have to be removed once used.
Also a little code cleanup and added testing.
| author | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Sun, 19 Mar 2017 15:32:14 -0400 |
| parents | 47bd81998ddc |
| children | f4b6a2a3e605 |
comparison
equal
deleted
inserted
replaced
| 5209:729e703a6c5a | 5210:7da56980754d |
|---|---|
| 927 given the newness of this code notifying the | 927 given the newness of this code notifying the |
| 928 user and having them notify the admins for | 928 user and having them notify the admins for |
| 929 debugging seems to be an advantage. | 929 debugging seems to be an advantage. |
| 930 | 930 |
| 931 ''' | 931 ''' |
| 932 # Create the otks handle here as we need it almost immediately. | |
| 933 # If this is perf issue, set to None here and check below | |
| 934 # once all header checks have passed if it needs to be opened. | |
| 935 otks=self.db.getOTKManager() | |
| 936 | |
| 932 # Assume: never allow changes via GET | 937 # Assume: never allow changes via GET |
| 933 if self.env['REQUEST_METHOD'] not in ['POST', 'PUT', 'DELETE']: | 938 if self.env['REQUEST_METHOD'] not in ['POST', 'PUT', 'DELETE']: |
| 939 if "@csrf" in self.form: | |
| 940 # We have a nonce being used with a method it should | |
| 941 # not be. If the nonce exists, report to admin so they | |
| 942 # can fix the nonce leakage and destroy it. (nonces | |
| 943 # used in a get are more exposed than those used in a | |
| 944 # post.) Note, I don't attempt to validate here since | |
| 945 # existance here is the sign of a failure. If nonce | |
| 946 # exists try to report the referer header to try to | |
| 947 # find where this comes from so it can be fixed. If | |
| 948 # nonce doesn't exist just ignore it. Maybe we should | |
| 949 # report, but somebody could spam us with a ton of | |
| 950 # invalid keys and fill up the logs. | |
| 951 if 'HTTP_REFERER' in self.env: | |
| 952 referer = self.env['HTTP_REFERER'] | |
| 953 else: | |
| 954 referer = self._("Referer header not available.") | |
| 955 key=self.form['@csrf'].value | |
| 956 if otks.exists(key): | |
| 957 logger.error( | |
| 958 self._("csrf key used with wrong method from: %s"), | |
| 959 referer) | |
| 960 otks.destroy(key) | |
| 961 self.db.commit() | |
| 934 return True | 962 return True |
| 963 | |
| 935 config=self.instance.config | 964 config=self.instance.config |
| 936 user=self.db.getuid() | 965 user=self.db.getuid() |
| 937 | 966 |
| 938 # the HTTP headers we check. | 967 # the HTTP headers we check. |
| 939 # Note that the xmlrpc header is missing. Its enforcement is | 968 # Note that the xmlrpc header is missing. Its enforcement is |
| 1043 if 'HTTP_X-REQUESTED-WITH' not in self.env: | 1072 if 'HTTP_X-REQUESTED-WITH' not in self.env: |
| 1044 self.add_error_message("Required Header Missing") | 1073 self.add_error_message("Required Header Missing") |
| 1045 logger.error(self._("csrf X-REQUESED-WITH xmlrpc required header check failed for user%s."), user) | 1074 logger.error(self._("csrf X-REQUESED-WITH xmlrpc required header check failed for user%s."), user) |
| 1046 return False | 1075 return False |
| 1047 | 1076 |
| 1048 otks=self.db.getOTKManager() | |
| 1049 enforce=config['WEB_CSRF_ENFORCE_TOKEN'] | 1077 enforce=config['WEB_CSRF_ENFORCE_TOKEN'] |
| 1050 if "@csrf" not in self.form: | 1078 if "@csrf" not in self.form: |
| 1051 if enforce == 'required': | 1079 if enforce == 'required': |
| 1052 self.add_error_message(self._("No csrf token found")) | 1080 self.add_error_message(self._("No csrf token found")) |
| 1053 logger.error(self._("required csrf field missing for user%s"), user) | 1081 logger.error(self._("required csrf field missing for user%s"), user) |
| 1059 return True | 1087 return True |
| 1060 | 1088 |
| 1061 key=self.form['@csrf'].value | 1089 key=self.form['@csrf'].value |
| 1062 uid = otks.get(key, 'uid', default=None) | 1090 uid = otks.get(key, 'uid', default=None) |
| 1063 sid = otks.get(key, 'sid', default=None) | 1091 sid = otks.get(key, 'sid', default=None) |
| 1092 if __debug__: | |
| 1093 ts = otks.get(key, '__timestamp', default=None) | |
| 1094 print("Found key %s for user%s sess: %s, ts %s, time %s"%(key, uid, sid, ts, time.time())) | |
| 1064 current_session = self.session_api._sid | 1095 current_session = self.session_api._sid |
| 1065 # validate against user and session | 1096 |
| 1097 # The key has been used or compromised. Delete it to prevent replay. | |
| 1098 otks.destroy(key) | |
| 1099 self.db.commit() | |
| 1066 | 1100 |
| 1067 ''' | 1101 ''' |
| 1068 # I think now that LogoutAction redirects to | 1102 # I think now that LogoutAction redirects to |
| 1069 # self.base ([tracker] web parameter in config.ini), | 1103 # self.base ([tracker] web parameter in config.ini), |
| 1070 # this code is not needed. However I am keeping it | 1104 # this code is not needed. However I am keeping it |
| 1098 return True | 1132 return True |
| 1099 else: | 1133 else: |
| 1100 self.add_error_message("Reload window before logging in.") | 1134 self.add_error_message("Reload window before logging in.") |
| 1101 ''' | 1135 ''' |
| 1102 | 1136 |
| 1103 # The key has been used or compromised. Delete it to prevent replay. | 1137 # validate against user and session |
| 1104 otks.destroy(key) | |
| 1105 self.db.commit() | |
| 1106 | |
| 1107 if uid != user: | 1138 if uid != user: |
| 1108 if enforce in ('required', "yes"): | 1139 if enforce in ('required', "yes"): |
| 1109 self.add_error_message(self._("Invalid csrf token found: %s")%key) | 1140 self.add_error_message(self._("Invalid csrf token found: %s")%key) |
| 1141 logger.error(self._("csrf uid mismatch for user%s."), user) | |
| 1110 return False | 1142 return False |
| 1111 elif enforce == 'logfailure': | 1143 elif enforce == 'logfailure': |
| 1112 logger.warning(self._("csrf uid mismatch for user%s."), user) | 1144 logger.warning(self._("csrf uid mismatch for user%s."), user) |
| 1113 if current_session != sid: | 1145 if current_session != sid: |
| 1114 if enforce in ('required', "yes"): | 1146 if enforce in ('required', "yes"): |
| 1115 self.add_error_message(self._( | 1147 self.add_error_message(self._( |
| 1116 "Invalid csrf session found: %s")%key) | 1148 "Invalid csrf session found: %s")%key) |
| 1149 logger.error(self._("csrf session mismatch for user%s."), user) | |
| 1117 return False | 1150 return False |
| 1118 elif enforce == 'logfailure': | 1151 elif enforce == 'logfailure': |
| 1119 logger.warning(self._("csrf session mismatch for user%s."), user) | 1152 logger.warning(self._("csrf session mismatch for user%s."), user) |
| 1120 return True | 1153 return True |
| 1121 | 1154 |
