Merge lp:~abentley/bzr-builder/safe into lp:bzr-builder

[Aaron Bentley]

Add --safe option to prevent arbitrary code execution.

This branch adds the --safe option, as we discussed.
It changes forbidden_instructions to permitted_instructions.
It removes the period from the problem, because a period is supplied by
RecipeParseError.
It fixes the error I was getting with an unchanged trunk.

=== modified file 'cmds.py'
--- cmds.py	2010-07-09 20:54:37 +0000
+++ cmds.py	2010-08-25 19:33:04 +0000
@@ -46,6 +46,7 @@
         DEBUPSTREAM_VAR,
         RecipeParser,
         resolve_revisions,
+        SAFE_INSTRUCTIONS,
         )
 
 
@@ -68,7 +69,13 @@
         manifest_f.close()
 
 
-def get_branch_from_recipe_file(recipe_file):
+def get_branch_from_recipe_file(recipe_file, safe=False):
+    """Return the base branch for the specified recipe.
+
+    :param recipe_file: The URL of the recipe file to retrieve.
+    :param safe: if True, reject recipes that would cause arbitrary code
+        execution.
+    """
     recipe_transport = transport.get_transport(os.path.dirname(recipe_file))
     try:
         recipe_contents = recipe_transport.get_bytes(
@@ -76,8 +83,12 @@
     except errors.NoSuchFile:
         raise errors.BzrCommandError("Specified recipe does not exist: "
                 "%s" % recipe_file)
+    if safe:
+        permitted_instructions = SAFE_INSTRUCTIONS
+    else:
+        permitted_instructions = None
     parser = RecipeParser(recipe_contents, filename=recipe_file)
-    return parser.parse()
+    return parser.parse(permitted_instructions=permitted_instructions)
 
 
 def get_old_recipe(if_changed_from):
@@ -318,16 +329,18 @@
                     ]
 
     def _get_prepared_branch_from_recipe(self, recipe_file,
-            if_changed_from=None):
+            if_changed_from=None, safe=False):
         """Common code to prepare a branch and do substitutions.
 
         :param recipe_file: a path to a recipe file to work from.
         :param if_changed_from: an optional path to a manifest to
             compare the recipe against.
+        :param safe: if True, reject recipes that would cause arbitrary code
+            execution.
         :return: A tuple with (retcode, base_branch). If retcode is None
             then the command execution should continue.
         """
-        base_branch = get_branch_from_recipe_file(recipe_file)
+        base_branch = get_branch_from_recipe_file(recipe_file, safe=safe)
         time = datetime.datetime.utcnow()
         base_branch.substitute_time(time)
         old_recipe = None
@@ -388,6 +401,8 @@
                 Option("append-version", type=str, help="Append the "
                         "specified string to the end of the version used "
                         "in debian/changelog."),
+                Option("safe", help="Error if the recipe would cause"
+                       " arbitrary code execution.")
             ]
 
     takes_args = ["recipe_file", "working_basedir?"]
@@ -395,7 +410,7 @@
     def run(self, recipe_file, working_basedir=None, manifest=None,
             if_changed_from=None, package=None, distribution=None,
             dput=None, key_id=None, no_build=None, watch_ppa=False,
-            append_version=None):
+            append_version=None, safe=False):
 
         if dput is not None and key_id is None:
             raise errors.BzrCommandError("You must specify --key-id if you "
@@ -409,7 +424,7 @@
                 target_from_dput(dput)
 
         result, base_branch = self._get_prepared_branch_from_recipe(recipe_file,
-            if_changed_from=if_changed_from)
+            if_changed_from=if_changed_from, safe=safe)
         if result is not None:
             return result
         if working_basedir is None:

=== modified file 'recipe.py'
--- recipe.py	2010-08-16 20:07:15 +0000
+++ recipe.py	2010-08-25 19:33:04 +0000
@@ -56,6 +56,9 @@
 NEST_INSTRUCTION = "nest"
 RUN_INSTRUCTION = "run"
 
+SAFE_INSTRUCTIONS = [
+    MERGE_INSTRUCTION, NEST_PART_INSTRUCTION, NEST_INSTRUCTION]
+
 TIME_VAR = "{time}"
 DATE_VAR = "{date}"
 REVNO_VAR = "{revno}"
@@ -770,12 +773,12 @@
         if filename is None:
             self.filename = "recipe"
 
-    def parse(self, forbidden_instructions=None):
+    def parse(self, permitted_instructions=None):
         """Parse the recipe.
 
-        :param forbidden_instructions: a list of instructions that you
-            don't want to allow. Defaults to None allowing them all.
-        :type forbidden_instructions: list(str) or None
+        :param permitted_instructions: a list of instructions that you
+            want to allow. Defaults to None allowing them all.
+        :type permitted_instructions: list(str) or None
         :return: a RecipeBranch representing the recipe.
         """
         self.lines = self.text.split("\n")
@@ -819,7 +822,7 @@
                 last_instruction = ""
             else:
                 instruction = self.parse_instruction(
-                    forbidden_instructions=forbidden_instructions)
+                    permitted_instructions=permitted_instructions)
                 if instruction == RUN_INSTRUCTION:
                     self.parse_whitespace("the command")
                     command = self.take_to_newline().strip()
@@ -868,7 +871,7 @@
         self.new_line()
         return version, deb_version
 
-    def parse_instruction(self, forbidden_instructions=None):
+    def parse_instruction(self, permitted_instructions=None):
         if self.version < 0.2:
             options = (MERGE_INSTRUCTION, NEST_INSTRUCTION)
             options_str = "'%s' or '%s'" % options
@@ -884,10 +887,10 @@
             self.throw_parse_error("End of line while looking for %s"
                     % options_str)
         if instruction in options:
-            if forbidden_instructions is not None:
-                if instruction in forbidden_instructions:
+            if permitted_instructions is not None:
+                if instruction not in permitted_instructions:
                     self.throw_parse_error("The '%s' instruction is "
-                            "forbidden." % instruction,
+                            "forbidden" % instruction,
                             cls=ForbiddenInstructionError,
                             instruction_name=instruction)
             self.take_chars(len(instruction))

=== modified file 'tests/test_blackbox.py'
--- tests/test_blackbox.py	2010-07-09 15:42:23 +0000
+++ tests/test_blackbox.py	2010-08-25 19:33:04 +0000
@@ -1,5 +1,5 @@
-# bzr-builter: a bzr plugin to constuct trees based on recipes
-# Copyright 2009 Canonical Ltd.
+# bzr-builder: a bzr plugin to constuct trees based on recipes
+# Copyright 2009-2010 Canonical Ltd.
 
 # This program is free software: you can redistribute it and/or modify it 
 # under the terms of the GNU General Public License version 3, as published 
@@ -274,5 +274,13 @@
                     "deb-version $\nsource 1\n")])
         err = self.run_bzr("dailydeb -q test.recipe working --package foo",
                 retcode=3)[1]
-        self.assertEqual("bzr: ERROR: Invalid deb-version: $: "
-                "Could not parse version: $\n", err)
+        self.assertContainsRe(err, "bzr: ERROR: Invalid deb-version: \$: ")
+
+    def test_cmd_dailydeb_with_safe(self):
+        self.make_simple_package()
+        self.build_tree_contents([("test.recipe", "# bzr-builder format 0.3 "
+                    "deb-version 1\nsource 1\nrun something bad")])
+        out, err = self.run_bzr("dailydeb -q test.recipe working --safe",
+            retcode=3)
+        self.assertContainsRe(err, "The 'run' instruction is forbidden.$")
+

=== modified file 'tests/test_recipe.py'
--- tests/test_recipe.py	2010-08-12 14:04:50 +0000
+++ tests/test_recipe.py	2010-08-25 19:33:04 +0000
@@ -35,7 +35,7 @@
         RecipeBranch,
         RecipeParseError,
         resolve_revisions,
-        RUN_INSTRUCTION,
+        SAFE_INSTRUCTIONS,
         )
 
 
@@ -438,7 +438,7 @@
         exc = self.assertParseError(3, 1, "The 'run' instruction is "
                 "forbidden.", self.get_recipe, self.basic_header_and_branch
                 + "run touch test\n",
-                forbidden_instructions=[RUN_INSTRUCTION])
+                permitted_instructions=[SAFE_INSTRUCTIONS])
         self.assertTrue(isinstance(exc, ForbiddenInstructionError))
         self.assertEqual("run", exc.instruction_name)