From 4e7bda736c096e62bfd4fe0a36ccb44cb1f40cfc Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Fri, 28 Mar 2025 14:28:37 +0100 Subject: [PATCH] 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 | 3 +- fdroidserver/common.py | 27 +++++++++++++---- fdroidserver/import_subcommand.py | 2 +- fdroidserver/metadata.py | 9 ++---- tests/test_common.py | 50 +++++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 14 deletions(-) diff --git a/fdroidserver/build.py b/fdroidserver/build.py index 41df2c1c..e5ee9663 100644 --- a/fdroidserver/build.py +++ b/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 = common.get_head_commit_id(git.repo.Repo(build_dir)) + commit_id = str(common.get_head_commit_id(build_dir)) else: commit_id = build.commit diff --git a/fdroidserver/common.py b/fdroidserver/common.py index 4a30bb6b..ce5dc195 100644 --- a/fdroidserver/common.py +++ b/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): - """Get git commit ID for HEAD as a str.""" +def get_head_commit_id(git_repo_dir): + """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 - except ValueError: - return "None" + if type(git_repo_dir) is git.Repo: + d = git_repo_dir.git_dir + 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): diff --git a/fdroidserver/import_subcommand.py b/fdroidserver/import_subcommand.py index b9f9a4c4..345c2891 100644 --- a/fdroidserver/import_subcommand.py +++ b/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) diff --git a/fdroidserver/metadata.py b/fdroidserver/metadata.py index f5fcffac..47fbdca7 100644 --- a/fdroidserver/metadata.py +++ b/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 . -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(git.Repo(build_dir)) + commit_id = common.get_head_commit_id(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 . - # pylint: disable-next=no-member - except git.exc.InvalidGitRepositoryError: + else: logging.debug( _('Including metadata from {path}').format(path=metadata_in_repo) ) diff --git a/tests/test_common.py b/tests/test_common.py index 522367a4..f6cdb0cb 100755 --- a/tests/test_common.py +++ b/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))