update: never execute VCS e.g. git

Package repos come from untrusted sources, in terms of the buildserver. They
should be handled in VMs and containers as much as possible to avoid
vulnerabilities.  As far as I could tell, `fdroid update` only has a single
place where it executes any VCS system: if there is .fdroid.yml present in
a package repo, then it will fetch the commit ID using git.

For better security properties, this implements a simple function to just
read the files to get that commit ID.  The function that executes git to do
the same thing is relabeled "unsafe".  That is used for status JSON
everywhere, but that runs on fdroiddata.git and fdroidserver.git, which are
trusted repos.

The unsafe version is also used in places where git.Repo() is needed for
other things.

fdroidserver/build.py

@@ -721,8 +721,7 @@ def build_local(app, build, vcs, build_dir, output_dir, log_dir, srclib_dir, ext
         bindir = os.path.join(root_dir, 'bin')
 
     if os.path.isdir(os.path.join(build_dir, '.git')):
-        import git
+        commit_id = str(common.get_head_commit_id(build_dir))
-        commit_id = common.get_head_commit_id(git.repo.Repo(build_dir))
     else:
         commit_id = build.commit
 

fdroidserver/common.py

@@ -1286,12 +1286,29 @@ def write_status_json(output, pretty=False, name=None):
         rsync_status_file_to_repo(path, repo_subdir='status')
 
 
-def get_head_commit_id(git_repo):
+def get_head_commit_id(git_repo_dir):
-    """Get git commit ID for HEAD as a str."""
+    """Get git commit ID for HEAD as a str.
+
+    This only reads files, so it should be safe to use on untrusted
+    repos.  It was created to avoid running the git executable, no
+    matter what. It uses a tiny subset of the git.Repo class to avoid
+    setting up the git executable.
+
+    """
     try:
-        return git_repo.head.commit.hexsha
+        if type(git_repo_dir) is git.Repo:
-    except ValueError:
+            d = git_repo_dir.git_dir
-        return "None"
+        else:
+            d = os.path.join(git_repo_dir, '.git')
+        repo = type(
+            'Repo',
+            (object,),
+            {'common_dir': d, 'git_dir': d, 're_hexsha_only': git.Repo.re_hexsha_only},
+        )()
+        return git.refs.symbolic.SymbolicReference.dereference_recursive(repo, 'HEAD')
+    except (FileNotFoundError, ValueError) as e:
+        msg = _("Cannot read {path}: {error}").format(path=os.getcwd(), error=str(e))
+        logging.debug(msg)
 
 
 def setup_vcs(app):

fdroidserver/import_subcommand.py

@@ -342,7 +342,7 @@ def main():
 
     app.AutoUpdateMode = 'Version'
     app.UpdateCheckMode = 'Tags'
-    build.commit = common.get_head_commit_id(git_repo)
+    build.commit = common.get_head_commit_id(tmp_importer_dir)
 
     # Extract some information...
     paths = get_all_gradle_and_manifests(tmp_importer_dir)

fdroidserver/metadata.py

@@ -18,7 +18,6 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import git
 from pathlib import Path
 import math
 import platform
@@ -658,14 +657,12 @@ def parse_metadata(metadatapath):
         build_dir = common.get_build_dir(app)
         metadata_in_repo = build_dir / '.fdroid.yml'
         if metadata_in_repo.is_file():
-            try:
+            commit_id = common.get_head_commit_id(build_dir)
-                commit_id = common.get_head_commit_id(git.Repo(build_dir))
+            if commit_id is not None:
                 logging.debug(
                     _('Including metadata from %s@%s') % (metadata_in_repo, commit_id)
                 )
-            # See https://github.com/PyCQA/pylint/issues/2856 .
+            else:
-            # pylint: disable-next=no-member
-            except git.exc.InvalidGitRepositoryError:
                 logging.debug(
                     _('Including metadata from {path}').format(path=metadata_in_repo)
                 )

tests/test_common.py

@@ -3470,3 +3470,53 @@ class UnsafePermissionsTest(SetUpTearDownMixin, unittest.TestCase):
             fdroidserver.common.read_config()
             self.assertTrue('unsafe' not in lw.output[0])
             self.assertEqual(1, len(lw.output))
+
+
+class GetHeadCommitIdTest(unittest.TestCase):
+    """Test and compare two methods of getting the commit ID."""
+
+    def setUp(self):
+        self._td = mkdtemp()
+        self.testdir = self._td.name
+        os.chdir(self.testdir)
+        logging.getLogger('git.cmd').setLevel(logging.INFO)
+
+    def tearDown(self):
+        os.chdir(basedir)
+        self._td.cleanup()
+
+    @unittest.skipUnless((basedir.parent / '.git').exists(), 'Needs a working git repo')
+    def test_get_head_commit_id_compare(self):
+        """Run on this git repo to get some real world noise in there."""
+        git_dir = basedir.parent
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_dir))
+
+    def test_get_head_commit_id_error_bare_repo(self):
+        """Error because it is an empty, bare git repo."""
+        git_repo = git.Repo.init(self.testdir)
+        self.assertIsNone(fdroidserver.common.get_head_commit_id(git_repo))
+
+    def test_get_head_commit_id_error_no_repo(self):
+        """Error because there is no .git/ dir."""
+        with self.assertLogs('root', level=logging.DEBUG):
+            self.assertIsNone(fdroidserver.common.get_head_commit_id(self.testdir))
+
+    def test_get_head_commit_id_detached_and_branch(self):
+        """Fetching commit ID must work from detached HEADs and branches."""
+        git_repo = git.Repo.init(self.testdir)
+        Path('README').write_text('this is just a test')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("add README")
+        Path('LICENSE').write_text('free!')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("add LICENSE")
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_repo))
+        # detached HEAD
+        git_repo.git.checkout('HEAD^')
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_repo))
+        # on a branch with a new commits
+        git_repo.git.checkout('test', b=True)
+        Path('foo.py').write_text('print("code!")')
+        git_repo.git.add(all=True)
+        git_repo.index.commit("add code")
+        self.assertIsNotNone(fdroidserver.common.get_head_commit_id(git_repo))