fix: add authorization checks to XML-RPC stop() endpoints

The XML-RPC methods jobs.stop(), recipesets.stop(), recipes.stop(),
and recipetasks.stop() only required authentication (not_anonymous)
but did not verify that the caller owns the job or has the stop_task
permission.  This allowed any authenticated user to cancel any other
user's job by calling these endpoints directly, bypassing the
can_stop() check enforced by taskactions.stop().

Add can_stop(identity.current.user) checks to all four endpoints,
matching the authorization already present in taskactions.stop() and
the REST API (POST /jobs/<id>/status).  Lab controllers are
unaffected because they authenticate with the stop_task permission,
which can_stop() already permits.

Made-with: Cursor

Author: Zinkelburger

IntegrationTests/src/bkr/inttest/server/selenium/test_job_cancel_authz.py

@@ -0,0 +1,109 @@
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+
+"""
+Regression tests for missing authorization checks on the XML-RPC
+jobs.stop(), recipesets.stop(), recipes.stop(), and recipetasks.stop()
+endpoints.
+"""
+
+import xmlrpclib
+from turbogears.database import session
+from bkr.inttest.server.selenium import XmlRpcTestCase
+from bkr.inttest import data_setup
+from bkr.server.model import TaskStatus
+
+
+class UnauthorizedJobCancelViaJobsStopTest(XmlRpcTestCase):
+
+    def setUp(self):
+        with session.begin():
+            self.victim = data_setup.create_user(password=u'victim')
+            self.attacker = data_setup.create_user(password=u'attacker')
+            self.job = data_setup.create_running_job(owner=self.victim)
+            self.job_id = self.job.id
+        self.server = self.get_server()
+        self.server.auth.login_password(self.attacker.user_name, 'attacker')
+
+    def test_taskactions_stop_rejects_unauthorized_user(self):
+        """Baseline: the protected path correctly denies the attacker."""
+        try:
+            self.server.taskactions.stop(
+                'J:%s' % self.job_id, 'cancel', 'authz test')
+            self.fail('taskactions.stop should have denied the attacker')
+        except xmlrpclib.Fault, e:
+            self.assertIn("don't have permission", e.faultString)
+
+    def test_jobs_stop_rejects_unauthorized_user(self):
+        try:
+            self.server.jobs.stop(self.job_id, 'cancel', 'authz test')
+            self.fail('jobs.stop should have denied the attacker')
+        except xmlrpclib.Fault, e:
+            self.assertIn("don't have permission", e.faultString)
+        with session.begin():
+            session.refresh(self.job)
+            self.assertNotEqual(self.job.status, TaskStatus.cancelled,
+                'Job was cancelled despite the attacker not owning it')
+
+
+class UnauthorizedCancelViaRecipeSetsStopTest(XmlRpcTestCase):
+
+    def setUp(self):
+        with session.begin():
+            self.victim = data_setup.create_user(password=u'victim')
+            self.attacker = data_setup.create_user(password=u'attacker')
+            self.job = data_setup.create_running_job(owner=self.victim)
+            self.recipeset_id = self.job.recipesets[0].id
+        self.server = self.get_server()
+        self.server.auth.login_password(self.attacker.user_name, 'attacker')
+
+    def test_recipesets_stop_rejects_unauthorized_user(self):
+        try:
+            self.server.recipesets.stop(
+                self.recipeset_id, 'cancel', 'authz test')
+            self.fail('recipesets.stop should have denied the attacker')
+        except xmlrpclib.Fault, e:
+            self.assertIn("don't have permission", e.faultString)
+
+
+class UnauthorizedCancelViaRecipesStopTest(XmlRpcTestCase):
+
+    def setUp(self):
+        with session.begin():
+            self.victim = data_setup.create_user(password=u'victim')
+            self.attacker = data_setup.create_user(password=u'attacker')
+            self.job = data_setup.create_running_job(owner=self.victim)
+            self.recipe_id = self.job.recipesets[0].recipes[0].id
+        self.server = self.get_server()
+        self.server.auth.login_password(self.attacker.user_name, 'attacker')
+
+    def test_recipes_stop_rejects_unauthorized_user(self):
+        try:
+            self.server.recipes.stop(
+                self.recipe_id, 'cancel', 'authz test')
+            self.fail('recipes.stop should have denied the attacker')
+        except xmlrpclib.Fault, e:
+            self.assertIn("don't have permission", e.faultString)
+
+
+class UnauthorizedCancelViaRecipeTasksStopTest(XmlRpcTestCase):
+
+    def setUp(self):
+        with session.begin():
+            self.victim = data_setup.create_user(password=u'victim')
+            self.attacker = data_setup.create_user(password=u'attacker')
+            self.job = data_setup.create_running_job(owner=self.victim)
+            self.task_id = self.job.recipesets[0].recipes[0].tasks[0].id
+        self.server = self.get_server()
+        self.server.auth.login_password(self.attacker.user_name, 'attacker')
+
+    def test_recipetasks_stop_rejects_unauthorized_user(self):
+        try:
+            self.server.recipetasks.stop(
+                self.task_id, 'cancel', 'authz test')
+            self.fail('recipetasks.stop should have denied the attacker')
+        except xmlrpclib.Fault, e:
+            self.assertIn("don't have permission", e.faultString)

Server/bkr/server/jobs.py

@@ -849,6 +849,8 @@ def stop(self, job_id, stop_type, msg=None):
             job = Job.by_id(job_id)
         except InvalidRequestError:
             raise BX('Invalid job ID: %s' % job_id)
+        if not job.can_stop(identity.current.user):
+            raise BX("You don't have permission to stop job %s" % job_id)
         if stop_type not in job.stop_types:
             raise BX('Invalid stop_type: %s, must be one of %s' %
                              (stop_type, job.stop_types))

Server/bkr/server/recipes.py

@@ -211,6 +211,9 @@ def stop(self, recipe_id, stop_type, msg=None):
             recipe = Recipe.by_id(recipe_id)
         except InvalidRequestError:
             raise BX('Invalid recipe ID: %s' % recipe_id)
+        if not recipe.recipeset.can_stop(identity.current.user):
+            raise BX("You don't have permission to stop recipe %s"
+                     % recipe_id)
         if stop_type not in recipe.stop_types:
             raise BX('Invalid stop_type: %s, must be one of %s' %
                              (stop_type, recipe.stop_types))

Server/bkr/server/recipesets.py

@@ -253,6 +253,9 @@ def stop(self, recipeset_id, stop_type, msg=None):
             recipeset = RecipeSet.by_id(recipeset_id)
         except InvalidRequestError:
             raise BX('Invalid recipeset ID: %s' % recipeset_id)
+        if not recipeset.can_stop(identity.current.user):
+            raise BX("You don't have permission to stop recipeset %s"
+                     % recipeset_id)
         if stop_type not in recipeset.stop_types:
             raise BX('Invalid stop_type: %s, must be one of %s' %
                              (stop_type, recipeset.stop_types))

Server/bkr/server/recipetasks.py

@@ -201,6 +201,9 @@ def stop(self, task_id, stop_type, msg=None):
             task = RecipeTask.by_id(task_id)
         except InvalidRequestError:
             raise BX('Invalid task ID: %s' % task_id)
+        if not task.can_stop(identity.current.user):
+            raise BX("You don't have permission to stop task %s"
+                     % task_id)
         if stop_type not in task.stop_types:
             raise BX('Invalid stop_type: %s, must be one of %s' %
                              (stop_type, task.stop_types))