From 6038fdf8617319a13b0b42f3283ec2066d54b283 Mon Sep 17 00:00:00 2001

From: "Bryn M. Reeves" <bmr@redhat.com>

Date: Thu, 19 Nov 2015 18:46:36 +0000

Subject: [PATCH] [policies] move hash determination to policies

It's crazy having the Policy classes call a function in the

utilities module only to have that function then load the Policy

module, call policy.get_preferred_hash_algorithm() and then test

the result.

Get rid of the get_hash_name() function in the utilities module

and simplify the calls in the policies module to obtain the

preferred hash name.

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

---

 sos/policies/__init__.py | 22 +++++++++++++++-------

 sos/utilities.py         | 12 ------------

 tests/utilities_tests.py |  2 +-

 3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/sos/policies/__init__.py b/sos/policies/__init__.py

index a403bb9..bf9afde 100644

--- a/sos/policies/__init__.py

+++ b/sos/policies/__init__.py

@@ -10,7 +10,6 @@ from os import environ

 from sos.utilities import (ImporterHelper,

                            import_module,

-                           get_hash_name,

                            shell_out)

 from sos.plugins import IndependentPlugin

 from sos import _sos as _

@@ -286,17 +285,17 @@ No changes will be made to system configuration.

         considered to be a superuser"""

         return (os.getuid() == 0)

-    def _create_checksum(self, final_filename=None):

+    def _create_checksum(self, hash_name, final_filename=None):

         if not final_filename:

             return False

         archive_fp = open(final_filename, 'rb')

-        digest = hashlib.new(get_hash_name())

+        digest = hashlib.new(hash_name)

         digest.update(archive_fp.read())

         archive_fp.close()

         return digest.hexdigest()

-    def get_preferred_hash_algorithm(self):

+    def get_preferred_hash_name(self):

         """Returns the string name of the hashlib-supported checksum algorithm

         to use"""

         return "md5"

@@ -309,10 +308,11 @@ No changes will be made to system configuration.

         self._print()

+        hash_name = self.get_preferred_hash_name()

         if not build:

             # store checksum into file

-            fp = open(final_filename + "." + get_hash_name(), "w")

-            checksum = self._create_checksum(final_filename)

+            fp = open(final_filename + "." + hash_name, "w")

+            checksum = self._create_checksum(hash_name, final_filename)

             if checksum:

                 fp.write(checksum + "\n")

             fp.close()

@@ -373,20 +373,28 @@ class LinuxPolicy(Policy):

     vendor = "None"

     PATH = "/bin:/sbin:/usr/bin:/usr/sbin"

+    _preferred_hash_name = None

+

     def __init__(self, sysroot=None):

         super(LinuxPolicy, self).__init__(sysroot=sysroot)

-    def get_preferred_hash_algorithm(self):

+    def get_preferred_hash_name(self):

+

+        if self._preferred_hash_name:

+            return self._preferred_hash_name

+

         checksum = "md5"

         try:

             fp = open("/proc/sys/crypto/fips_enabled", "r")

         except:

+            self._preferred_hash_name = checksum

             return checksum

         fips_enabled = fp.read()

         if fips_enabled.find("1") >= 0:

             checksum = "sha256"

         fp.close()

+        self._preferred_hash_name = checksum

         return checksum

     def default_runlevel(self):

diff --git a/sos/utilities.py b/sos/utilities.py

index d3a1048..63e7ee4 100644

--- a/sos/utilities.py

+++ b/sos/utilities.py

@@ -52,18 +52,6 @@ def fileobj(path_or_file, mode='r'):

         return closing(path_or_file)

-def get_hash_name():

-    """Returns the algorithm used when computing a hash"""

-    import sos.policies

-    policy = sos.policies.load()

-    try:

-        name = policy.get_preferred_hash_algorithm()

-        hashlib.new(name)

-        return name

-    except:

-        return 'sha256'

-

-

 def convert_bytes(bytes_, K=1 << 10, M=1 << 20, G=1 << 30, T=1 << 40):

     """Converts a number of bytes to a shorter, more human friendly format"""

     fn = float(bytes_)

diff --git a/tests/utilities_tests.py b/tests/utilities_tests.py

index c464692..f1b60e2 100644

--- a/tests/utilities_tests.py

+++ b/tests/utilities_tests.py

@@ -5,7 +5,7 @@ import unittest

 import six

 from six import StringIO

-from sos.utilities import grep, get_hash_name, is_executable, sos_get_command_output, find, tail, shell_out

+from sos.utilities import grep, is_executable, sos_get_command_output, find, tail, shell_out

 import sos

 TEST_DIR = os.path.dirname(__file__)

-- 

2.4.3

From 7f2727749d0c37095a20c5d4cf6f9a2e086a2375 Mon Sep 17 00:00:00 2001

From: "Bryn M. Reeves" <bmr@redhat.com>

Date: Thu, 19 Nov 2015 19:07:50 +0000

Subject: [PATCH] [policies] refactor Policy.display_results() args

Pass explicit archive and build directory arguments to the

Policy.display_results() method rather than a single path name

argument and a boolean to indicate whether it is an archive file

or a build directory path.

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

---

 sos/policies/__init__.py | 24 ++++++++++++++----------

 sos/sosreport.py         | 12 +++++++++---

 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/sos/policies/__init__.py b/sos/policies/__init__.py

index bf9afde..5b0b706 100644

--- a/sos/policies/__init__.py

+++ b/sos/policies/__init__.py

@@ -285,11 +285,11 @@ No changes will be made to system configuration.

         considered to be a superuser"""

         return (os.getuid() == 0)

-    def _create_checksum(self, hash_name, final_filename=None):

-        if not final_filename:

+    def _create_checksum(self, hash_name, archive=None):

+        if not archive:

             return False

-        archive_fp = open(final_filename, 'rb')

+        archive_fp = open(archive, 'rb')

         digest = hashlib.new(hash_name)

         digest.update(archive_fp.read())

         archive_fp.close()

@@ -300,29 +300,33 @@ No changes will be made to system configuration.

         to use"""

         return "md5"

-    def display_results(self, final_filename=None, build=False):

+    def display_results(self, archive, directory):

+        # Display results is called from the tail of SoSReport.final_work()

+        #

+        # Logging is already shutdown and all terminal output must use the

+        # print() call.

         # make sure a report exists

-        if not final_filename:

+        if not archive and not directory:

             return False

         self._print()

         hash_name = self.get_preferred_hash_name()

-        if not build:

+        if archive:

             # store checksum into file

-            fp = open(final_filename + "." + hash_name, "w")

-            checksum = self._create_checksum(hash_name, final_filename)

+            fp = open(archive + "." + hash_name, "w")

+            checksum = self._create_checksum(hash_name, archive)

             if checksum:

                 fp.write(checksum + "\n")

             fp.close()

             self._print(_("Your sosreport has been generated and saved "

-                        "in:\n  %s") % final_filename)

+                        "in:\n  %s") % archive)

         else:

             checksum = None

             self._print(_("sosreport build tree is located at : %s" %

-                        final_filename))

+                        directory))

         self._print()

         if checksum:

diff --git a/sos/sosreport.py b/sos/sosreport.py

index a1f1b96..f3e0f34 100644

--- a/sos/sosreport.py

+++ b/sos/sosreport.py

@@ -1436,6 +1436,10 @@ class SoSReport(object):

         # this must come before archive creation to ensure that log

         # files are closed and cleaned up at exit.

         self._finish_logging()

+

+        archive = None    # archive path

+        directory = None  # report directory path (--build)

+

         # package up the results for the support organization

         if not self.opts.build:

             old_umask = os.umask(0o077)

@@ -1443,7 +1447,7 @@ class SoSReport(object):

                 print(_("Creating compressed archive..."))

             # compression could fail for a number of reasons

             try:

-                final_filename = self.archive.finalize(

+                archive = self.archive.finalize(

                     self.opts.compression_type)

             except (OSError, IOError) as e:

                 if e.errno in fatal_fs_errors:

@@ -1460,8 +1464,10 @@ class SoSReport(object):

             finally:

                 os.umask(old_umask)

         else:

-            final_filename = self.archive.get_archive_path()

-        self.policy.display_results(final_filename, build=self.opts.build)

+            directory = self.archive.get_archive_path()

+

+        self.policy.display_results(archive, directory)

+

         self.tempfile_util.clean()

         return True

-- 

2.4.3

From 19e2bbccb6a86d6ea94f5c82860bed4d2276bbf3 Mon Sep 17 00:00:00 2001

From: "Bryn M. Reeves" <bmr@redhat.com>

Date: Thu, 19 Nov 2015 19:19:42 +0000

Subject: [PATCH] [sosreport] move archive checksumming to sosreport

Although the digest algorithm is policy controlled the actual

mechanism to checksum the archive does not belong in the policies

module: historically this was done to keep the code that calculates

the checksum close to the UI code that reports it.

Move the calculation to the main SoSReport class's final_work()

method and add a 'checksum' argument to the display_results()

method so that the value can be reported.

In future it may make sense to push the checksum code directly into

the archive class.

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

---

 sos/policies/__init__.py | 22 +---------------------

 sos/sosreport.py         | 26 +++++++++++++++++++++++++-

 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/sos/policies/__init__.py b/sos/policies/__init__.py

index 5b0b706..20e0e3b 100644

--- a/sos/policies/__init__.py

+++ b/sos/policies/__init__.py

@@ -13,7 +13,6 @@ from sos.utilities import (ImporterHelper,

                            shell_out)

 from sos.plugins import IndependentPlugin

 from sos import _sos as _

-import hashlib

 from textwrap import fill

 from six import print_

 from six.moves import input

@@ -285,22 +284,12 @@ No changes will be made to system configuration.

         considered to be a superuser"""

         return (os.getuid() == 0)

-    def _create_checksum(self, hash_name, archive=None):

-        if not archive:

-            return False

-

-        archive_fp = open(archive, 'rb')

-        digest = hashlib.new(hash_name)

-        digest.update(archive_fp.read())

-        archive_fp.close()

-        return digest.hexdigest()

-

     def get_preferred_hash_name(self):

         """Returns the string name of the hashlib-supported checksum algorithm

         to use"""

         return "md5"

-    def display_results(self, archive, directory):

+    def display_results(self, archive, directory, checksum):

         # Display results is called from the tail of SoSReport.final_work()

         #

         # Logging is already shutdown and all terminal output must use the

@@ -312,19 +301,10 @@ No changes will be made to system configuration.

         self._print()

-        hash_name = self.get_preferred_hash_name()

         if archive:

-            # store checksum into file

-            fp = open(archive + "." + hash_name, "w")

-            checksum = self._create_checksum(hash_name, archive)

-            if checksum:

-                fp.write(checksum + "\n")

-            fp.close()

-

             self._print(_("Your sosreport has been generated and saved "

                         "in:\n  %s") % archive)

         else:

-            checksum = None

             self._print(_("sosreport build tree is located at : %s" %

                         directory))

diff --git a/sos/sosreport.py b/sos/sosreport.py

index f3e0f34..f7a5f11 100644

--- a/sos/sosreport.py

+++ b/sos/sosreport.py

@@ -33,6 +33,7 @@ from stat import ST_UID, ST_GID, ST_MODE, ST_CTIME, ST_ATIME, ST_MTIME, S_IMODE

 from time import strftime, localtime

 from collections import deque

 import tempfile

+import hashlib

 from sos import _sos as _

 from sos import __version__

@@ -1432,6 +1433,18 @@ class SoSReport(object):

                     raise

                 self._log_plugin_exception(plugname, "postproc")

+    def _create_checksum(self, archive=None):

+        if not archive:

+            return False

+

+        hash_name = self.policy.get_preferred_hash_name()

+

+        archive_fp = open(archive, 'rb')

+        digest = hashlib.new(hash_name)

+        digest.update(archive_fp.read())

+        archive_fp.close()

+        return digest.hexdigest()

+

     def final_work(self):

         # this must come before archive creation to ensure that log

         # files are closed and cleaned up at exit.

@@ -1466,7 +1479,18 @@ class SoSReport(object):

         else:

             directory = self.archive.get_archive_path()

-        self.policy.display_results(archive, directory)

+        hash_name = self.policy.get_preferred_hash_name()

+        checksum = None

+

+        if hash_name and not self.opts.build:

+            # store checksum into file

+            fp = open(archive + "." + hash_name, "w")

+            checksum = self._create_checksum(archive)

+            if checksum:

+                fp.write(checksum + "\n")

+            fp.close()

+

+        self.policy.display_results(archive, directory, checksum)

         self.tempfile_util.clean()

         return True

-- 

2.4.3

From 4a9b919a7f1b9542a23982e49cc9035e84551e13 Mon Sep 17 00:00:00 2001

From: "Bryn M. Reeves" <bmr@redhat.com>

Date: Fri, 20 Nov 2015 18:48:33 +0000

Subject: [PATCH] [sosreport] prepare report in a private subdirectory

To avoid file creation races in shared temporary directories like

/tmp and /var/tmp use a private (0700) subdirectory to build the

FileCacheArchive and subsequent archive and compressed archive

files: only create a file in the containing directory when it can

be done as a single atomic rename.

This prevents sos from writing to an arbitrary location under the

control of another user: a malicious user could steal data or over

write files in /etc resulting in a local privilege escalation.

There remains a further race since once the archive name is known

the checksum file name becomes predictable: as the checksum file

is also prepared in the subdirectory and moved into place the

result is always either success or an error that is reported to

the user.

The correct checksum value is still reported to the user via the

terminal.

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

---

 sos/sosreport.py | 100 ++++++++++++++++++++++++++++++++++++++++++-------------

 1 file changed, 77 insertions(+), 23 deletions(-)

diff --git a/sos/sosreport.py b/sos/sosreport.py

index f7a5f11..fe251ed 100644

--- a/sos/sosreport.py

+++ b/sos/sosreport.py

@@ -32,6 +32,7 @@ from sos.utilities import ImporterHelper

 from stat import ST_UID, ST_GID, ST_MODE, ST_CTIME, ST_ATIME, ST_MTIME, S_IMODE

 from time import strftime, localtime

 from collections import deque

+from shutil import rmtree

 import tempfile

 import hashlib

@@ -678,6 +679,7 @@ class SoSReport(object):

         self.tempfile_util = None

         self._args = args

         self.sysroot = "/"

+        self.sys_tmp = None

         try:

             import signal

@@ -678,15 +678,22 @@ class SoSReport(object):

         self._is_root = self.policy.is_root()

-        self.tmpdir = os.path.abspath(

-            self.policy.get_tmp_dir(self.opts.tmp_dir))

-        if not os.path.isdir(self.tmpdir) \

-                or not os.access(self.tmpdir, os.W_OK):

+        # system temporary directory to use

+        tmp = os.path.abspath(self.policy.get_tmp_dir(self.opts.tmp_dir))

+

+        if not os.path.isdir(tmp) \

+                or not os.access(tmp, os.W_OK):

             # write directly to stderr as logging is not initialised yet

-            sys.stderr.write("temporary directory %s " % self.tmpdir

+            sys.stderr.write("temporary directory %s " % tmp

                              + "does not exist or is not writable\n")

             self._exit(1)

+

+        self.sys_tmp = tmp

+

+        # our (private) temporary directory

+        self.tmpdir = tempfile.mkdtemp(prefix="sos.", dir=self.sys_tmp)

         self.tempfile_util = TempFileUtil(self.tmpdir)

+

         self._set_directories()

         self._setup_logging()

@@ -1433,27 +1442,34 @@ class SoSReport(object):

                     raise

                 self._log_plugin_exception(plugname, "postproc")

-    def _create_checksum(self, archive=None):

+    def _create_checksum(self, archive, hash_name):

         if not archive:

             return False

-        hash_name = self.policy.get_preferred_hash_name()

-

         archive_fp = open(archive, 'rb')

         digest = hashlib.new(hash_name)

         digest.update(archive_fp.read())

         archive_fp.close()

         return digest.hexdigest()

+    def _write_checksum(self, archive, hash_name, checksum):

+            # store checksum into file

+            fp = open(archive + "." + hash_name, "w")

+            if checksum:

+                fp.write(checksum + "\n")

+            fp.close()

+

     def final_work(self):

-        # this must come before archive creation to ensure that log

+        # This must come before archive creation to ensure that log

         # files are closed and cleaned up at exit.

+        #

+        # All subsequent terminal output must use print().

         self._finish_logging()

         archive = None    # archive path

         directory = None  # report directory path (--build)

-        # package up the results for the support organization

+        # package up and compress the results

         if not self.opts.build:

             old_umask = os.umask(0o077)

             if not self.opts.quiet:

@@ -1464,10 +1480,9 @@ class SoSReport(object):

                     self.opts.compression_type)

             except (OSError, IOError) as e:

                 if e.errno in fatal_fs_errors:

-                    self.ui_log.error("")

-                    self.ui_log.error(" %s while finalizing archive"

-                                      % e.strerror)

-                    self.ui_log.error("")

+                    print("")

+                    print(_(" %s while finalizing archive" % e.strerror))

+                    print("")

                     self._exit(1)

             except:

                 if self.opts.debug:

@@ -1477,18 +1492,55 @@ class SoSReport(object):

             finally:

                 os.umask(old_umask)

         else:

+            # move the archive root out of the private tmp directory.

             directory = self.archive.get_archive_path()

+            dir_name = os.path.basename(directory)

+            try:

+                os.rename(directory, os.path.join(self.sys_tmp, dir_name))

+            except (OSError, IOError):

+                    print(_("Error moving directory: %s" % directory))

+                    return False

-        hash_name = self.policy.get_preferred_hash_name()

         checksum = None

-        if hash_name and not self.opts.build:

-            # store checksum into file

-            fp = open(archive + "." + hash_name, "w")

-            checksum = self._create_checksum(archive)

-            if checksum:

-                fp.write(checksum + "\n")

-            fp.close()

+        if not self.opts.build:

+            # compute and store the archive checksum

+            hash_name = self.policy.get_preferred_hash_name()

+            checksum = self._create_checksum(archive, hash_name)

+            self._write_checksum(archive, hash_name, checksum)

+

+            # output filename is in the private tmpdir - move it to the

+            # containing directory.

+            final_name = os.path.join(self.sys_tmp, os.path.basename(archive))

+

+            archive_hash = archive + "." + hash_name

+            final_hash = final_name + "." + hash_name

+

+            # move the archive and checksum file

+            try:

+                    os.rename(archive, final_name)

+                    archive = final_name

+            except (OSError, IOError):

+                    print(_("Error moving archive file: %s" % archive))

+                    return False

+

+            # There is a race in the creation of the final checksum file:

+            # since the archive has already been published and the checksum

+            # file name is predictable once the archive name is known a

+            # malicious user could attempt to create a symbolic link in order

+            # to misdirect writes to a file of the attacker's choosing.

+            #

+            # To mitigate this we write the checksum inside the private tmp

+            # directory and use an atomic rename that is guaranteed to either

+            # succeed or fail: at worst the move will fail and be reported to

+            # the user. The correct checksum value is still written to the

+            # terminal and nothing is written to a location under the control

+            # of the user creating the link.

+            try:

+                    os.rename(archive_hash, final_hash)

+            except (OSError, IOError):

+                    print(_("Error moving checksum file: %s" % archive_hash))

+                    return False

         self.policy.display_results(archive, directory, checksum)

@@ -1546,8 +1598,10 @@ class SoSReport(object):

                     self.archive.cleanup()

                 if self.tempfile_util:

                     self.tempfile_util.clean()

+                if self.tmpdir:

+                    rmtree(self.tmpdir)

             except:

-                pass

+                raise

         return False

-- 

2.4.3

From 08121d877741e33333a1ae01280f6898d7d4ca15 Mon Sep 17 00:00:00 2001

From: "Bryn M. Reeves" <bmr@redhat.com>

Date: Thu, 10 Dec 2015 11:50:49 +0000

Subject: [PATCH] [sosreport] clean up private temporary directory

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

---

 sos/sosreport.py | 7 ++++++-

 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sos/sosreport.py b/sos/sosreport.py

index fe251ed..bc7792a 100644

--- a/sos/sosreport.py

+++ b/sos/sosreport.py

@@ -1544,7 +1544,12 @@ class SoSReport(object):

         self.policy.display_results(archive, directory, checksum)

-        self.tempfile_util.clean()

+        # clean up

+        if self.tempfile_util:

+            self.tempfile_util.clean()

+        if self.tmpdir:

+            rmtree(self.tmpdir)

+

         return True

     def verify_plugins(self):

-- 

2.4.3

From a7a2dd8eb1791257f80fd2ae2993b06472690aea Mon Sep 17 00:00:00 2001

From: "Bryn M. Reeves" <bmr@redhat.com>

Date: Wed, 13 Jan 2016 10:57:59 +0000

Subject: [PATCH] [sosreport] report correct final path with --build

Ensure the correct path (in the system temporary directory) is

reported when the --build option is used.

Signed-off-by: Bryn M. Reeves <bmr@redhat.com>

---

 sos/sosreport.py | 4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sos/sosreport.py b/sos/sosreport.py

index bc7792a..f61ea4a 100644

--- a/sos/sosreport.py

+++ b/sos/sosreport.py

@@ -1496,7 +1496,9 @@ class SoSReport(object):

             directory = self.archive.get_archive_path()

             dir_name = os.path.basename(directory)

             try:

-                os.rename(directory, os.path.join(self.sys_tmp, dir_name))

+                final_dir = os.path.join(self.sys_tmp, dir_name)

+                os.rename(directory, final_dir)

+                directory = final_dir

             except (OSError, IOError):

                     print(_("Error moving directory: %s" % directory))

                     return False

-- 

2.4.3