From d3d48dba5eb3484eb17955f21a349225ad80597a Mon Sep 17 00:00:00 2001 From: Hans-Christoph Steiner Date: Tue, 4 Sep 2018 11:03:05 +0200 Subject: [PATCH] add config.yml as default config file format None of the config options in config.py require Python code. YAML is a common config data format, and it is also used for build metadata. It is also much safer to use since it can be pure data, without anything executable in it. This also reduces the attack surface of the fdroid process by eliminating a guaranteed place to write to get code executed. With config.py, any exploit that can get local write access can turn that into execute access by writing to the config.py, then cleaning up after itself once it has what it needs. Switching to YAML removes that vector entirely. Also, this removes the config_file argument. It is not used in either fdroidserver or repomaker. Also, it probably wouldn't work since so much of the code assumes that the current working dir is the root of the repo. --- fdroidserver/common.py | 36 +++++++++++++++++------ tests/common.TestCase | 65 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 8 deletions(-) diff --git a/fdroidserver/common.py b/fdroidserver/common.py index df9ffe25..48434581 100644 --- a/fdroidserver/common.py +++ b/fdroidserver/common.py @@ -39,6 +39,7 @@ import socket import base64 import urllib.parse import urllib.request +import yaml import zipfile import tempfile import json @@ -284,14 +285,18 @@ def regsub_file(pattern, repl, path): f.write(text) -def read_config(opts, config_file='config.py'): +def read_config(opts): """Read the repository config The config is read from config_file, which is in the current directory when any of the repo management commands are used. If - there is a local metadata file in the git repo, then config.py is + there is a local metadata file in the git repo, then the config is not required, just use defaults. + config.yml is the preferred form because no code is executed when + reading it. config.py is deprecated and supported for backwards + compatibility. + """ global config, options @@ -301,12 +306,23 @@ def read_config(opts, config_file='config.py'): options = opts config = {} + config_file = 'config.yml' + old_config_file = 'config.py' - if os.path.isfile(config_file): + if os.path.exists(config_file) and os.path.exists(old_config_file): + logging.error(_("""Conflicting config files! Using {newfile}, ignoring {oldfile}!""") + .format(oldfile=old_config_file, newfile=config_file)) + + if os.path.exists(config_file): logging.debug(_("Reading '{config_file}'").format(config_file=config_file)) - with io.open(config_file, "rb") as f: - code = compile(f.read(), config_file, 'exec') - exec(code, None, config) # nosec TODO switch to YAML file + with open(config_file) as fp: + config = yaml.safe_load(fp) + elif os.path.exists(old_config_file): + logging.warning(_("""{oldfile} is deprecated, use {newfile}""") + .format(oldfile=old_config_file, newfile=config_file)) + with io.open(old_config_file, "rb") as fp: + code = compile(fp.read(), old_config_file, 'exec') + exec(code, None, config) # nosec TODO automatically migrate else: logging.warning(_("No 'config.py' found, using defaults.")) @@ -329,10 +345,14 @@ def read_config(opts, config_file='config.py'): '-providerArg', 'opensc-fdroid.cfg'] if any(k in config for k in ["keystore", "keystorepass", "keypass"]): - st = os.stat(config_file) + if os.path.exists(config_file): + f = config_file + elif os.path.exists(old_config_file): + f = old_config_file + st = os.stat(f) if st.st_mode & stat.S_IRWXG or st.st_mode & stat.S_IRWXO: logging.warning(_("unsafe permissions on '{config_file}' (should be 0600)!") - .format(config_file=config_file)) + .format(config_file=f)) fill_config_defaults(config) diff --git a/tests/common.TestCase b/tests/common.TestCase index ff7befb1..b49ba10e 100755 --- a/tests/common.TestCase +++ b/tests/common.TestCase @@ -47,6 +47,7 @@ class CommonTest(unittest.TestCase): if not os.path.exists(self.tmpdir): os.makedirs(self.tmpdir) os.chdir(self.basedir) + fdroidserver.common.config = None def test_parse_human_readable_size(self): for k, v in ((9827, 9827), (123.456, 123), ('123b', 123), ('1.2', 1), @@ -1409,6 +1410,70 @@ class CommonTest(unittest.TestCase): self.assertIsNotNone(result) self.assertNotEqual(result, '') + def test_with_no_config(self): + """It should set defaults if no config file is found""" + testdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir) + os.chdir(testdir) + self.assertFalse(os.path.exists('config.yml')) + self.assertFalse(os.path.exists('config.py')) + config = fdroidserver.common.read_config(fdroidserver.common.options) + self.assertEqual(None, config.get('apksigner')) + self.assertIsNotNone(config.get('char_limits')) + + def test_with_config_yml(self): + """Make sure it is possible to use config.yml alone.""" + testdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir) + os.chdir(testdir) + with open('config.yml', 'w') as fp: + fp.write('apksigner: yml') + self.assertTrue(os.path.exists('config.yml')) + self.assertFalse(os.path.exists('config.py')) + config = fdroidserver.common.read_config(fdroidserver.common.options) + self.assertEqual('yml', config.get('apksigner')) + + def test_with_config_py(self): + """Make sure it is still possible to use config.py alone.""" + testdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir) + os.chdir(testdir) + with open('config.py', 'w') as fp: + fp.write('apksigner = "py"') + self.assertFalse(os.path.exists('config.yml')) + self.assertTrue(os.path.exists('config.py')) + config = fdroidserver.common.read_config(fdroidserver.common.options) + self.assertEqual("py", config.get('apksigner')) + + def test_config_perm_warning(self): + """Exercise the code path that issues a warning about unsafe permissions.""" + testdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir) + os.chdir(testdir) + with open('config.yml', 'w') as fp: + fp.write('keystore: foo.jks') + self.assertTrue(os.path.exists(fp.name)) + os.chmod(fp.name, 0o666) + fdroidserver.common.read_config(fdroidserver.common.options) + os.remove(fp.name) + fdroidserver.common.config = None + + with open('config.py', 'w') as fp: + fp.write('keystore = "foo.jks"') + self.assertTrue(os.path.exists(fp.name)) + os.chmod(fp.name, 0o666) + fdroidserver.common.read_config(fdroidserver.common.options) + + def test_with_both_config_yml_py(self): + """If config.yml and config.py are present, config.py should be ignored.""" + testdir = tempfile.mkdtemp(prefix=inspect.currentframe().f_code.co_name, dir=self.tmpdir) + os.chdir(testdir) + with open('config.yml', 'w') as fp: + fp.write('apksigner: yml') + with open('config.py', 'w') as fp: + fp.write('apksigner = "py"') + self.assertTrue(os.path.exists('config.yml')) + self.assertTrue(os.path.exists('config.py')) + config = fdroidserver.common.read_config(fdroidserver.common.options) + self.assertEqual('yml', config.get('apksigner')) + + if __name__ == "__main__": os.chdir(os.path.dirname(__file__))