#1745 Resolve issues reported by bandit
Merged 2 years ago by breilly. Opened 2 years ago by breilly.
breilly/fm-orchestrator remediation-8826  into  master

@@ -160,7 +160,8 @@ 

          fmt = sep.join(["%%{%s}" % tag for tag in tags])

          cmd = "/bin/rpm -qa --qf '{0}\n'".format(fmt)

          with open("/dev/null", "r+") as devnull:

-             p = subprocess.Popen(

+             # subprocess call does not take user input, thus risk is low

+             p = subprocess.Popen(  # nosec

                  cmd, shell=True, stdin=devnull, stdout=subprocess.PIPE, stderr=devnull)

  

              (stdout, stderr) = p.communicate()
@@ -361,7 +362,7 @@ 

                  mmd = load_mmd(data)

                  ret["filename"] = mmd_filename

                  ret["filesize"] = len(raw_data)

-                 ret["checksum"] = hashlib.md5(raw_data).hexdigest()

+                 ret["checksum"] = hashlib.md5(raw_data, usedforsecurity=False).hexdigest()

          except IOError:

              if arch == "src":

                  # This might happen in case the Module is submitted directly
@@ -403,7 +404,7 @@ 

          try:

              log_path = os.path.join(output_path, "build.log")

              with open(log_path, "rb") as build_log:

-                 checksum = hashlib.md5(build_log.read()).hexdigest()

+                 checksum = hashlib.md5(build_log.read(), usedforsecurity=False).hexdigest()

              stat = os.stat(log_path)

              ret.append(

                  {

@@ -414,7 +414,8 @@ 

          if len(nsvc_tag) + len("-build") > max_length:

              # Fallback to the old format of 'module-<hash>' if the generated koji tag

              # name is longer than max_length

-             nsvc_hash = hashlib.sha1(".".join(nsvc_list).encode("utf-8")).hexdigest()[:16]

+             nsvc_hash = hashlib.sha1(".".join(nsvc_list).encode("utf-8"),

+                                      usedforsecurity=False).hexdigest()[:16]

              return prefix + nsvc_hash + suffix

          return nsvc_tag

  

@@ -459,7 +459,9 @@ 

                  config_opts = {}

                  code = compile(f.read(), infile, "exec")

                  # pylint: disable=exec-used

-                 exec(code)

+                 # exec is not being called with user input

+                 # only used for local builds, never on the server

+                 exec(code)  # nosec

  

                  self.groups = config_opts["chroot_setup_cmd"].split(" ")[1:]

                  self.yum_conf = config_opts["yum.conf"]

@@ -190,7 +190,7 @@ 

          str(module_build.version),

          str(module_build.context),

      ]).encode("utf-8")

-     dist_hash = hashlib.sha1(dist_str).hexdigest()[:8]

+     dist_hash = hashlib.sha1(dist_str, usedforsecurity=False).hexdigest()[:8]

  

      # We need to share the same auto-incrementing index in dist tag between all MSE builds.

      # We can achieve that by using the lowest build ID of all the MSE siblings including

@@ -35,7 +35,7 @@ 

          os.getcwd(), "module_build_service.db"))

      SQLALCHEMY_TRACK_MODIFICATIONS = True

      # Where we should run when running "manage.py run" directly.

-     HOST = "0.0.0.0"

+     HOST = None  # Flask will default to 127.0.0.1

      PORT = 5000

  

  

@@ -548,7 +548,7 @@ 

              if dep not in deps_to_filter

          }

          property_json = json.dumps(OrderedDict(sorted(mmd_formatted_buildrequires.items())))

-         return hashlib.sha1(property_json.encode("utf-8")).hexdigest()

+         return hashlib.sha1(property_json.encode("utf-8"), usedforsecurity=False).hexdigest()

  

      @staticmethod

      def calculate_runtime_context(mmd_dependencies):
@@ -567,7 +567,7 @@ 

          # Sort the streams for each module name and also sort the module names.

          mmd_requires = {dep: sorted(list(streams)) for dep, streams in mmd_requires.items()}

          property_json = json.dumps(OrderedDict(sorted(mmd_requires.items())))

-         return hashlib.sha1(property_json.encode("utf-8")).hexdigest()

+         return hashlib.sha1(property_json.encode("utf-8"), usedforsecurity=False).hexdigest()

  

      @staticmethod

      def calculate_module_context(build_context, runtime_context):
@@ -581,7 +581,7 @@ 

          :return: module context hash

          """

          combined_hashes = "{0}:{1}".format(build_context, runtime_context)

-         return hashlib.sha1(combined_hashes.encode("utf-8")).hexdigest()[:8]

+         return hashlib.sha1(combined_hashes.encode("utf-8"), usedforsecurity=False).hexdigest()[:8]

  

      def siblings(self, db_session):

          query = db_session.query(ModuleBuild).filter(

@@ -291,7 +291,7 @@ 

  @console_script_help

  @manager.command

  def run(host=None, port=None, debug=None):

-     """ Runs the Flask app, locally.

+     """ Runs the Flask app, locally. Intended for dev instances, should not be used for production.

      """

      host = host or conf.host

      port = port or conf.port

@@ -58,6 +58,6 @@ 

          if build.build_context and build.runtime_context:

              combined_hashes = '{0}:{1}'.format(

                  build.build_context, build.runtime_context).encode('utf-8')

-             context = hashlib.sha1(combined_hashes).hexdigest()[:8]

+             context = hashlib.sha1(combined_hashes, usedforsecurity=False).hexdigest()[:8]

              connection.execute(

                  modulebuild.update().where(modulebuild.c.id == build.id).values(context=context))

@@ -60,7 +60,7 @@ 

              mmd_formatted_property = {

                  dep: info['ref'] for dep, info in mbs_xmd[xmd_name].items()}

              property_json = json.dumps(OrderedDict(sorted(mmd_formatted_property.items())))

-             contexts[xmd_name] = hashlib.sha1(property_json).hexdigest()

+             contexts[xmd_name] = hashlib.sha1(property_json, usedforsecurity=False).hexdigest()

  

          # Update the database now

          if len(contexts) == 2:

@@ -36,7 +36,7 @@ 

          if build.build_context and build.runtime_context:

              combined_hashes = '{0}:{1}'.format(

                  build.build_context, build.runtime_context).encode('utf-8')

-             context = hashlib.sha1(combined_hashes).hexdigest()[:8]

+             context = hashlib.sha1(combined_hashes, usedforsecurity=False).hexdigest()[:8]

              connection.execute(

                  modulebuild.update().where(modulebuild.c.id == build.id).values(

                      context=context))

@@ -62,7 +62,7 @@ 

          mmd_formatted_buildrequires = {

              dep: info['stream'] for dep, info in mbs_xmd["buildrequires"].items()}

          property_json = json.dumps(OrderedDict(sorted(mmd_formatted_buildrequires.items())))

-         context = hashlib.sha1(property_json).hexdigest()

+         context = hashlib.sha1(property_json, usedforsecurity=False).hexdigest()

  

          # Update the database now

          connection.execute(

no initial comment

1 new commit added

  • update
2 years ago

rebased onto 17ec6a8aed30d59aa09f62f8b56f96231394ed0c

2 years ago

The exec for rpm -qa could be replaced with an rpmlib call. That doesn't have to be done for this issue, but might be a good follow-up.

While the use of md5 in _get_arch_mmd_output and _get_output is indeed not for security, it would still be nice to get rid of it. Koji also supports checksum_type=sha256 now. This would be a good follow-up, though we need to be careful about possible compatibility issues for module consumers.

In _load_mock_config it might also be worth noting that this code is only used for local builds and never on the server.

I think we should probably do something more with this default HOST value.

  • Default to HOST=None causing Flask to use its default, 127.0.0.1
  • Update the manage.py run command to clearly indicate it is a dev server and should not be used for production

See also:
https://stackoverflow.com/questions/7023052/configure-flask-dev-server-to-be-visible-across-the-network

rebased onto 60483f76a63f51895d9642efdb98f08ea2afcf61

2 years ago

Thanks @mikem, updated PR and filed followup issues.

Thanks, looks like that addresses everything

:thumbsup:

rebased onto d4c213d

2 years ago

Commit d676e06 fixes this pull-request

Pull-Request has been merged by breilly

2 years ago

Pull-Request has been merged by breilly

2 years ago