From c7bcedb6baab81cc94e76fea0bc93f1c5062155f Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 07 2019 15:35:10 +0000 Subject: [PATCH 1/6] Add static security analysis tool Signed-off-by: Patrick Uiterwijk --- diff --git a/Makefile b/Makefile index df6da65..0b54d37 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ RPMBUILD = $(PWD)/dist/rpmbuild -all: testdeps lint pep8 test +all: testdeps lint pep8 test security echo "All tests passed" testdeps: @@ -52,6 +52,11 @@ pep8: # Check style consistency pep8 ipsilon +security: + # Run a static analyzer aimed at security (OpenStack Bandit) + # Level MEDIUM and above are blocking + bandit -r ipsilon -ll + # Requires NodeJS less and clear-css packages ui-node: less/ipsilon.less less/admin.less @@ -175,7 +180,7 @@ containertest-fedora29: container-fedora29 containertest-lint: container-fedora29 @echo "Starting code lint tests ..." - @docker run -v `pwd`:/code -t --rm --entrypoint /usr/bin/make ipsilon-fedora29 lint pep8 + @docker run -v `pwd`:/code -t --rm --entrypoint /usr/bin/make ipsilon-fedora29 lint pep8 security @echo "Code lint tests passed" containertest: containertest-lint containertest-centos6 containertest-centos7 containertest-fedora28 containertest-fedora29 diff --git a/tests/containers/Dockerfile-fedora b/tests/containers/Dockerfile-fedora index c519795..599a4f3 100644 --- a/tests/containers/Dockerfile-fedora +++ b/tests/containers/Dockerfile-fedora @@ -1 +1 @@ -RUN yum install -y etcd python2-python-etcd dbus-python python2-ipalib +RUN yum install -y etcd python2-python-etcd dbus-python python2-ipalib bandit From 223286e119eccdaca2cd606dba69e36e2c20c66e Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 07 2019 15:35:10 +0000 Subject: [PATCH 2/6] Skip security analysis in unit test Signed-off-by: Patrick Uiterwijk --- diff --git a/ipsilon/providers/saml2/sessions.py b/ipsilon/providers/saml2/sessions.py index 6e2b1bf..2cdf83f 100644 --- a/ipsilon/providers/saml2/sessions.py +++ b/ipsilon/providers/saml2/sessions.py @@ -290,7 +290,7 @@ if __name__ == '__main__': # temporary values to simulate cherrypy cherrypy_config['tools.sessions.timeout'] = 60 - factory = SAMLSessionFactory('/tmp/saml2sessions.sqlite') + factory = SAMLSessionFactory('/tmp/saml2sessions.sqlite') # nosec factory.wipe_data() sess1 = factory.add_session('_123456', provider1, "admin", From 4e039c3c8cc8f2195cbf431ad5d0c0708f200bee Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 07 2019 15:35:10 +0000 Subject: [PATCH 3/6] Setup dbupgrade template_env up like normal Signed-off-by: Patrick Uiterwijk --- diff --git a/ipsilon/tools/dbupgrade.py b/ipsilon/tools/dbupgrade.py index 46cdfcd..59b00c7 100644 --- a/ipsilon/tools/dbupgrade.py +++ b/ipsilon/tools/dbupgrade.py @@ -78,9 +78,12 @@ def execute_upgrade(cfgfile): cherrypy.config[option] = admin_config[option] # Initialize a minimal env - template_env = Environment(loader=FileSystemLoader( - os.path.join(cherrypy.config['base.dir'], - 'templates'))) + template_env = Environment( + loader=FileSystemLoader( + os.path.join(cherrypy.config['base.dir'], + 'templates')), + autoescape=True, + extensions=['jinja2.ext.autoescape']) root = Root('default', template_env) # Handle the session store if that is Sql From 4ef518051488aaf9650c6dbf145f8b74c59c72c4 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 07 2019 15:35:10 +0000 Subject: [PATCH 4/6] Store sessions as json Signed-off-by: Patrick Uiterwijk --- diff --git a/ipsilon/util/sessions.py b/ipsilon/util/sessions.py index 1b1c0be..1b723e8 100644 --- a/ipsilon/util/sessions.py +++ b/ipsilon/util/sessions.py @@ -11,10 +11,6 @@ except ImportError: etcd = None import json import time -try: - import cPickle as pickle -except ImportError: - import pickle SESSION_TABLE = {'columns': ['id', 'data', 'expiration_time'], 'primary_key': ('id', ), @@ -85,13 +81,20 @@ class SqlSession(Session): r = result.fetchone() if r: data = str(base64.b64decode(r[1])) - return pickle.loads(data) + if not data.startswith('['): + # This is a pre-upgrade pickle'd session. Just invalidate. + self._delete() + return + value, exp_time = json.loads(data) + exp_dt = datetime.datetime.utcfromtimestamp(exp_time) + return value, exp_dt def _save(self, expiration_time): + expiration_time = int(time.mktime(expiration_time.timetuple())) q = SqlQuery(self._db, 'sessions', SESSION_TABLE, trans=True) with q: q.delete({'id': self.id}) - data = pickle.dumps((self._data, expiration_time), self._proto) + data = json.dumps((self._data, expiration_time)) q.insert((self.id, base64.b64encode(data), expiration_time)) def _delete(self): From f97b890a3d06356866b9ab8ec18b2aac8b4927e9 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 07 2019 15:49:29 +0000 Subject: [PATCH 5/6] Skip false SQL injection positive Signed-off-by: Patrick Uiterwijk --- diff --git a/ipsilon/util/data.py b/ipsilon/util/data.py index 3454429..70364e6 100644 --- a/ipsilon/util/data.py +++ b/ipsilon/util/data.py @@ -983,7 +983,9 @@ class Store(Log): kvfilter['option'] = opt q.delete(kvfilter) except Exception as e: # pylint: disable=broad-except - self.error("Failed to delete from %s: [%s]" % (table, e)) + # nosec: This line is not a SQL line + self.error( + "Failed to delete from %s: [%s]" % (table, e)) # nosec raise def new_unique_data(self, table, data, ttl=None, expiration_time=None): From df821c1b857bd48c6b923c1d9588e54dae2d72c2 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 07 2019 15:49:29 +0000 Subject: [PATCH 6/6] Allow SHA1 for image names for now: they're admin-provided Once non-admins can upload images, we should look at migrating over to another hash. Signed-off-by: Patrick Uiterwijk --- diff --git a/ipsilon/util/config.py b/ipsilon/util/config.py index 9e21d19..365003d 100644 --- a/ipsilon/util/config.py +++ b/ipsilon/util/config.py @@ -14,7 +14,7 @@ def name_from_image(image): return None fext = imghdr.what(None, base64.b64decode(image)) - m = hashlib.sha1() + m = hashlib.sha1() # nosec: This file is admin-provided m.update(base64.b64decode(image)) return '%s.%s' % (m.hexdigest(), fext)