[checkmk-commits] Check_MK Git: check_mk: FIX Changed transid implemtation to work as CSRF protection ( Fixes CVE-2014-2330)

Module: check_mk Branch: master Commit: 50bb17166b31a46a53716f9d238d9b009906827f URL: http://git.mathias-kettner.de/git/?p=check_mk.git;a=commit;h=50bb17166b31a4… Author: Lars Michelsen &lt;lm(a)mathias-kettner.de&gt; Date: Mon Mar 31 12:09:30 2014 +0200 FIX Changed transid implemtation to work as CSRF protection (Fixes CVE-2014-2330) This change fixes possible attacks against Check_MK Multisite users. In previous versions a possible attacker could try to make the browsers of authenticated users open URLs of the Check_MK Multisite GUI to execute actions e.g. within WATO without knowledge of the attacked user. To make such an attack possible, there are several things needed: The user must be authenticated with multisite and have enough permission within multisite to execute the actions the attacker wants to use, the attacker needs to know the exact URL to the Multisite GUI. Then the attacker needs to make the user either click on a manipulated link or open a manipulated webpage which makes the browser of the user, where the user is authenticated with multisite, open the URL the attacker wants to make it open. The multisite GUI makes use of transids (transaction ids) when processing form submissions or actions. The transids were mainly used to prevent double execution of actions when reloading the page which performed the action in the browser. Now we changed internal handling of the transid to make it also prevent CSRF attacks. The transid is now some kind of shared secret between the webserver and the browser of the user. This ensures a form submission is intended by a previously requested page. This change impicates an incompatible change: In case you use a script which opens multisite pages to perform an action, e.g. set a downtime and use this with a regular user account which authenticates by username/password, the script won't work anymore after this change. The way to go is to adapt the script and change the user to authenticate with an automation secret instead of a password. For this kind of authentication, you will need to user other URL parameters (_username=... and _secret=...). --- .werks/766 | 33 +++++++++++++++++++++++ ChangeLog | 1 + web/htdocs/htmllib.py | 69 +++++++++++++++++++++++++++++++++++-------------- web/htdocs/login.py | 2 ++ 4 files changed, 86 insertions(+), 19 deletions(-) diff --git a/.werks/766 b/.werks/766 new file mode 100644 index 0000000..21d8748 --- /dev/null +++ b/.werks/766 @@ -0,0 +1,33 @@ +Title: Changed transid implemtation to work as CSRF protection (Fixes CVE-2014-2330) +Level: 3 +Component: multisite +Version: 1.2.5i2 +Date: 1396259365 +Class: fix + +This change fixes possible attacks against Check_MK Multisite users. In previous +versions a possible attacker could try to make the browsers of authenticated users +open URLs of the Check_MK Multisite GUI to execute actions e.g. within WATO without +knowledge of the attacked user. + +To make such an attack possible, there are several things needed: The user must be +authenticated with multisite and have enough permission within multisite to execute +the actions the attacker wants to use, the attacker needs to know the exact URL to the +Multisite GUI. Then the attacker needs to make the user either click on a manipulated +link or open a manipulated webpage which makes the browser of the user, where the user +is authenticated with multisite, open the URL the attacker wants to make it open. + +The multisite GUI makes use of transids (transaction ids) when processing form +submissions or actions. The transids were mainly used to prevent double execution +of actions when reloading the page which performed the action in the browser. +Now we changed internal handling of the transid to make it also prevent CSRF attacks. +The transid is now some kind of shared secret between the webserver and the browser +of the user. This ensures a form submission is intended by a previously requested page. + +This change impicates an incompatible change: In case you use a script which opens +multisite pages to perform an action, e.g. set a downtime and use this with a regular +user account which authenticates by username/password, the script won't work anymore +after this change. +The way to go is to adapt the script and change the user to authenticate with an +automation secret instead of a password. For this kind of authentication, you will +need to user other URL parameters (_username=... and _secret=...). diff --git a/ChangeLog b/ChangeLog index 5c95b6f..4c9d690 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,6 +8,7 @@ Multisite: * 0765 NagVis-Maps-Snapin: Now visualizes downtime / acknowledgment states of maps... + * 0766 FIX: Changed transid implemtation to work as CSRF protection (Fixes CVE-2014-2330)... 1.2.5i1: diff --git a/web/htdocs/htmllib.py b/web/htdocs/htmllib.py index 8c31d42..6a9c84c 100644 --- a/web/htdocs/htmllib.py +++ b/web/htdocs/htmllib.py @@ -74,6 +74,8 @@ class html: self.treestates = {} self.treestates_for_id = None self.caches = {} + self.new_transids = [] + self.ignore_transids = False RETURN = 13 SHIFT = 16 @@ -801,6 +803,9 @@ class html: self.bottom_footer() self.body_end() + # Hopefully this is the correct place to performe some "finalization" tasks. + self.store_new_transids() + def add_status_icon(self, img, tooltip, url = None): if url: self.status_icons[img] = tooltip, url @@ -938,35 +943,61 @@ class html: if not self.has_var("_ajaxid"): self.javascript("if(parent && parent.frames[0]) parent.frames[0].location.reload();"); - # Compute a (hopefully) unique transaction id + def set_ignore_transids(self): + self.ignore_transids = True + + # Compute a (hopefully) unique transaction id. This is generated during rendering + # of a form or an action link, stored in a user specific file for later validation, + # sent to the users browser via HTML code, then submitted by the user together + # with the action (link / form) and then validated if it is a known transid. When + # it is a known transid, it will be used and invalidated. If the id is not known, + # the action will not be processed. def fresh_transid(self): - return "%d/%d" % (int(time.time()), random.getrandbits(32)) + transid = "%d/%d" % (int(time.time()), random.getrandbits(32)) + self.new_transids.append(transid) + return transid # Marks a transaction ID as used. This is done by saving # it in a user specific settings file "transids.mk". At this # time we remove all entries from that list that are older - # then one week. - def invalidate_transid(self, id): - used_ids = self.load_transids() - new_ids = [] + # than one week. + def store_new_transids(self): + valid_ids = self.load_transids() + + cleared_ids = [] now = time.time() - for used_id in used_ids: - timestamp, rand = used_id.split("/") + for valid_id in valid_ids: + timestamp, rand = valid_id.split("/") if now - int(timestamp) < 604800: # 7 * 24 hours - new_ids.append(used_id) - used_ids.append(id) - self.save_transids(used_ids) - - # Checks, if the current transaction is valid, i.e. now - # browser reload. The HTML variable _transid must be present. - # If it is empty or -1, then it's always valid (this is used - # for webservice calls). + cleared_ids.append(valid_id) + + self.save_transids(cleared_ids + self.new_transids) + + # Remove the used transid from the list of valid ones + def invalidate_transid(self, used_id): + valid_ids = self.load_transids() + try: + valid_ids.remove(used_id) + except ValueError: + return + self.save_transids(valid_ids) + + # Checks, if the current transaction is valid, i.e. in case of + # browser reload a browser reload, the form submit should not + # be handled a second time.. The HTML variable _transid must be present. + # + # In case of automation users (authed by _secret in URL): If it is empty + # or -1, then it's always valid (this is used for webservice calls). + # This was also possible for normal users, but has been removed to preven + # security related issues. def transaction_valid(self): if not self.has_var("_transid"): return False id = self.var("_transid") - if not id or id == "-1": + if not id or self.ignore_transids: return True # automation + + # Normal user/password auth user handling timestamp, rand = id.split("/") # If age is too old (one week), it is always @@ -975,8 +1006,8 @@ class html: if now - int(timestamp) >= 604800: # 7 * 24 hours return False - # Now check, if this id is not yet invalidated - return id not in self.load_transids() + # Now check, if this id is a valid one + return id in self.load_transids() # Checks, if the current page is a transation, i.e. something # that is secured by a transid (such as a submitted form) diff --git a/web/htdocs/login.py b/web/htdocs/login.py index c3a06e0..55452c0 100644 --- a/web/htdocs/login.py +++ b/web/htdocs/login.py @@ -128,6 +128,8 @@ def check_auth_automation(): if secret and user and "/" not in user: path = defaults.var_dir + "/web/" + user + "/automation.secret" if os.path.isfile(path) and file(path).read().strip() == secret: + # Auth with automation secret succeeded - mark transid as unneeded in this case + html.set_ignore_transids() return user raise MKAuthException(_("Invalid automation secret for user %s") % user)