#287 Use the static security analysis tool OpenStack Bandit
Closed 5 years ago by puiterwijk. Opened 6 years ago by puiterwijk.
puiterwijk/ipsilon bandit  into  master

file modified
+7 -2
@@ -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 @@ 

  	# 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-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

@@ -290,7 +290,7 @@ 

      # 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",

file modified
+6 -3
@@ -78,9 +78,12 @@ 

          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

file modified
+1 -1
@@ -14,7 +14,7 @@ 

          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)

file modified
+3 -1
@@ -983,7 +983,9 @@ 

                          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):

file modified
+9 -6
@@ -11,10 +11,6 @@ 

      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 @@ 

              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):

@@ -1,1 +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

Do note that this does not currently pass, and I will open another PR to change away from using pickle's for storing sessions.

Looks good to me, though wouldn't you want it to pass before merging this? It's not important to me either way, so merge as you please.

@bowlofeggs Yes, I do want it to pass, which is why I will not yet be merging this. I opened this issue so that using this could be discussed, and we can merge it when ready :).

rebased

6 years ago

The current code passes the full test suite!

rebased onto b25a49b04fa762c0b1e496ddc06aa922d6340bc3

5 years ago

rebased onto a67376ff0f98244eb5469fb5ba4bd45def4b1e6d

5 years ago

Not sure if this is still relevant, because it contains F25 container, but otherwise LGTM

rebased onto c7bcedb

5 years ago

Pull-Request has been closed by puiterwijk

5 years ago