Repository: Azure/WALinuxAgent
Pull request #1485 (2019-03-12 15:58:45)

Add fixes for handling swap file and other nit fixes
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description
The fix addresses a potential vulnerability that exists in the way the Azure Linux Agent creates swap files on resource disks under some specific conditions.

<!-- if any -->
<!--
Please add an informative description that covers that changes made by the pull request. 
This checklist is used to make sure that common issues in a pull request are addressed.
This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
-->

---

### PR information
- [x] The title of the PR is clear and informative.
- [x] There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, [see this page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [x] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [x] If applicable, the PR references the bug/issue that it fixes in the description.
- [x] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [x] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).


Patch:
diff --git a/azurelinuxagent/daemon/resourcedisk/default.py b/azurelinuxagent/daemon/resourcedisk/default.py
index 0f0925d7..cfb76d2b 100644
--- a/azurelinuxagent/daemon/resourcedisk/default.py
+++ b/azurelinuxagent/daemon/resourcedisk/default.py
@@ -17,6 +17,7 @@
 
 import os
 import re
+import stat
 import sys
 import threading
 from time import sleep
@@ -124,12 +125,13 @@ class ResourceDiskHandler(object):
         force_option = 'F'
         if self.fs == 'xfs':
             force_option = 'f'
-        mkfs_string = "mkfs.{0} -{2} {1}".format(self.fs, partition, force_option)
+        mkfs_string = "mkfs.{0} -{2} {1}".format(
+            self.fs, partition, force_option)
 
         if "gpt" in ret[1]:
             logger.info("GPT detected, finding partitions")
             parts = [x for x in ret[1].split("\n") if
-                     re.match("^\s*[0-9]+", x)]
+                     re.match(r"^\s*[0-9]+", x)]
             logger.info("Found {0} GPT partition(s).", len(parts))
             if len(parts) > 1:
                 logger.info("Removing old GPT partitions")
@@ -138,18 +140,23 @@ class ResourceDiskHandler(object):
                     shellutil.run("parted {0} rm {1}".format(device, i))
 
                 logger.info("Creating new GPT partition")
-                shellutil.run("parted {0} mkpart primary 0% 100%".format(device))
+                shellutil.run(
+                    "parted {0} mkpart primary 0% 100%".format(device))
 
                 logger.info("Format partition [{0}]", mkfs_string)
                 shellutil.run(mkfs_string)
         else:
             logger.info("GPT not detected, determining filesystem")
-            ret = self.change_partition_type(suppress_message=True, option_str="{0} 1 -n".format(device))
+            ret = self.change_partition_type(
+                suppress_message=True,
+                option_str="{0} 1 -n".format(device))
             ptype = ret[1].strip()
             if ptype == "7" and self.fs != "ntfs":
                 logger.info("The partition is formatted with ntfs, updating "
                             "partition type to 83")
-                self.change_partition_type(suppress_message=False, option_str="{0} 1 83".format(device))
+                self.change_partition_type(
+                    suppress_message=False,
+                    option_str="{0} 1 83".format(device))
                 self.reread_partition_table(device)
                 logger.info("Format partition [{0}]", mkfs_string)
                 shellutil.run(mkfs_string)
@@ -169,7 +176,8 @@ class ResourceDiskHandler(object):
             attempts -= 1
 
         if not os.path.exists(partition):
-            raise ResourceDiskError("Partition was not created [{0}]".format(partition))
+            raise ResourceDiskError(
+                "Partition was not created [{0}]".format(partition))
 
         logger.info("Mount resource disk [{0}]", mount_string)
         ret, output = shellutil.run_get_output(mount_string, chk_err=False)
@@ -215,14 +223,19 @@ class ResourceDiskHandler(object):
         """
 
         command_to_use = '--part-type'
-        input = "sfdisk {0} {1} {2}".format(command_to_use, '-f' if suppress_message else '', option_str)
-        err_code, output = shellutil.run_get_output(input, chk_err=False, log_cmd=True)
+        input = "sfdisk {0} {1} {2}".format(
+            command_to_use, '-f' if suppress_message else '', option_str)
+        err_code, output = shellutil.run_get_output(
+            input, chk_err=False, log_cmd=True)
 
         # fall back to -c
         if err_code != 0:
-            logger.info("sfdisk with --part-type failed [{0}], retrying with -c", err_code)
+            logger.info(
+                "sfdisk with --part-type failed [{0}], retrying with -c",
+                err_code)
             command_to_use = '-c'
-            input = "sfdisk {0} {1} {2}".format(command_to_use, '-f' if suppress_message else '', option_str)
+            input = "sfdisk {0} {1} {2}".format(
+                command_to_use, '-f' if suppress_message else '', option_str)
             err_code, output = shellutil.run_get_output(input, log_cmd=True)
 
         if err_code == 0:
@@ -245,16 +258,30 @@ class ResourceDiskHandler(object):
         else:
             return 'mount {0} {1}'.format(partition, mount_point)
 
+    @staticmethod
+    def check_existing_swap_file(swapfile, swaplist, size):
+        if swapfile in swaplist and os.path.isfile(
+                swapfile) and os.path.getsize(swapfile) == size:
+            logger.info("Swap already enabled")
+            # restrict access to owner (remove all access from group, others)
+            swapfile_mode = os.stat(swapfile).st_mode
+            if swapfile_mode & (stat.S_IRWXG | stat.S_IRWXO):
+                swapfile_mode = swapfile_mode & ~(stat.S_IRWXG | stat.S_IRWXO)
+                logger.info(
+                    "Changing mode of {0} to {1:o}".format(
+                        swapfile, swapfile_mode))
+                os.chmod(swapfile, swapfile_mode)
+            return True
+
+        return False
+
     def create_swap_space(self, mount_point, size_mb):
         size_kb = size_mb * 1024
         size = size_kb * 1024
         swapfile = os.path.join(mount_point, 'swapfile')
         swaplist = shellutil.run_get_output("swapon -s")[1]
 
-        if swapfile in swaplist \
-                and os.path.isfile(swapfile) \
-                and os.path.getsize(swapfile) == size:
-            logger.info("Swap already enabled")
+        if self.check_existing_swap_file(swapfile, swaplist, size):
             return
 
         if os.path.isfile(swapfile) and os.path.getsize(swapfile) != size:
@@ -296,7 +323,8 @@ class ResourceDiskHandler(object):
             os.remove(filename)
 
         # If file system is xfs, use dd right away as we have been reported that
-        # swap enabling fails in xfs fs when disk space is allocated with fallocate
+        # swap enabling fails in xfs fs when disk space is allocated with
+        # fallocate
         ret = 0
         fn_sh = shellutil.quote((filename,))
         if self.fs != 'xfs':
@@ -305,13 +333,21 @@ class ResourceDiskHandler(object):
                 # Probable errors:
                 #  - OSError: Seen on Cygwin, libc notimpl?
                 #  - AttributeError: What if someone runs this under...
+                fd = None
+
                 try:
-                    with open(filename, 'w') as f:
-                        os.posix_fallocate(f.fileno(), 0, nbytes)
-                        return 0
-                except:
+                    fd = os.open(
+                        filename,
+                        os.O_CREAT | os.O_WRONLY | os.O_EXCL,
+                        stat.S_IRUSR | stat.S_IWUSR)
+                    os.posix_fallocate(fd, 0, nbytes)
+                    return 0
+                except BaseException:
                     # Not confident with this thing, just keep trying...
                     pass
+                finally:
+                    if fd is not None:
+                        os.close(fd)
 
             # fallocate command
             ret = shellutil.run(
diff --git a/azurelinuxagent/daemon/resourcedisk/freebsd.py b/azurelinuxagent/daemon/resourcedisk/freebsd.py
index ece166be..0a78771f 100644
--- a/azurelinuxagent/daemon/resourcedisk/freebsd.py
+++ b/azurelinuxagent/daemon/resourcedisk/freebsd.py
@@ -24,6 +24,7 @@ import azurelinuxagent.common.conf as conf
 from azurelinuxagent.common.exception import ResourceDiskError
 from azurelinuxagent.daemon.resourcedisk.default import ResourceDiskHandler
 
+
 class FreeBSDResourceDiskHandler(ResourceDiskHandler):
     """
     This class handles resource disk mounting for FreeBSD.
@@ -36,6 +37,7 @@ class FreeBSDResourceDiskHandler(ResourceDiskHandler):
     1. MBR: The resource disk partition is /dev/da1s1
     2. GPT: The resource disk partition is /dev/da1p2, /dev/da1p1 is for reserved usage.
     """
+
     def __init__(self):
         super(FreeBSDResourceDiskHandler, self).__init__()
 
@@ -52,25 +54,30 @@ class FreeBSDResourceDiskHandler(ResourceDiskHandler):
     def mount_resource_disk(self, mount_point):
         fs = self.fs
         if fs != 'ufs':
-            raise ResourceDiskError("Unsupported filesystem type:{0}, only ufs is supported.".format(fs))
+            raise ResourceDiskError(
+                "Unsupported filesystem type:{0}, only ufs is supported.".format(fs))
 
         # 1. Detect device
         err, output = shellutil.run_get_output('gpart list')
         if err:
-            raise ResourceDiskError("Unable to detect resource disk device:{0}".format(output))
+            raise ResourceDiskError(
+                "Unable to detect resource disk device:{0}".format(output))
         disks = self.parse_gpart_list(output)
 
         device = self.osutil.device_for_ide_port(1)
-        if device is None or not device in disks:
-        # fallback logic to find device
-            err, output = shellutil.run_get_output('camcontrol periphlist 2:1:0')
+        if device is None or device not in disks:
+            # fallback logic to find device
+            err, output = shellutil.run_get_output(
+                'camcontrol periphlist 2:1:0')
             if err:
                 # try again on "3:1:0"
-                err, output = shellutil.run_get_output('camcontrol periphlist 3:1:0')
+                err, output = shellutil.run_get_output(
+                    'camcontrol periphlist 3:1:0')
                 if err:
-                    raise ResourceDiskError("Unable to detect resource disk device:{0}".format(output))
+                    raise ResourceDiskError(
+                        "Unable to detect resource disk device:{0}".format(output))
 
-        # 'da1:  generation: 4 index: 1 status: MORE\npass2:  generation: 4 index: 2 status: LAST\n'
+            # 'da1:  generation: 4 index: 1 status: MORE\npass2:  generation: 4 index: 2 status: LAST\n'
             for line in output.split('\n'):
                 index = line.find(':')
                 if index > 0:
@@ -91,9 +98,11 @@ class FreeBSDResourceDiskHandler(ResourceDiskHandler):
         elif partition_table_type == 'GPT':
             provider_name = device + 'p2'
         else:
-            raise ResourceDiskError("Unsupported partition table type:{0}".format(output))
+            raise ResourceDiskError(
+                "Unsupported partition table type:{0}".format(output))
 
-        err, output = shellutil.run_get_output('gpart show -p {0}'.format(device))
+        err, output = shellutil.run_get_output(
+            'gpart show -p {0}'.format(device))
         if err or output.find(provider_name) == -1:
             raise ResourceDiskError("Resource disk partition not found.")
 
@@ -112,16 +121,26 @@ class FreeBSDResourceDiskHandler(ResourceDiskHandler):
         mount_cmd = 'mount -t {0} {1} {2}'.format(fs, partition, mount_point)
         err = shellutil.run(mount_cmd, chk_err=False)
         if err:
-            logger.info('Creating {0} filesystem on partition {1}'.format(fs, partition))
-            err, output = shellutil.run_get_output('newfs -U {0}'.format(partition))
+            logger.info(
+                'Creating {0} filesystem on partition {1}'.format(
+                    fs, partition))
+            err, output = shellutil.run_get_output(
+                'newfs -U {0}'.format(partition))
             if err:
-                raise ResourceDiskError("Failed to create new filesystem on partition {0}, error:{1}"
-                                        .format(partition, output))
+                raise ResourceDiskError(
+                    "Failed to create new filesystem on partition {0}, error:{1}" .format(
+                        partition, output))
             err, output = shellutil.run_get_output(mount_cmd, chk_err=False)
             if err:
-                raise ResourceDiskError("Failed to mount partition {0}, error {1}".format(partition, output))
-
-        logger.info("Resource disk partition {0} is mounted at {1} with fstype {2}", partition, mount_point, fs)
+                raise ResourceDiskError(
+                    "Failed to mount partition {0}, error {1}".format(
+                        partition, output))
+
+        logger.info(
+            "Resource disk partition {0} is mounted at {1} with fstype {2}",
+            partition,
+            mount_point,
+            fs)
         return mount_point
 
     def create_swap_space(self, mount_point, size_mb):
@@ -130,10 +149,7 @@ class FreeBSDResourceDiskHandler(ResourceDiskHandler):
         swapfile = os.path.join(mount_point, 'swapfile')
         swaplist = shellutil.run_get_output("swapctl -l")[1]
 
-        if swapfile in swaplist \
-                and os.path.isfile(swapfile) \
-                and os.path.getsize(swapfile) == size:
-            logger.info("Swap already enabled")
+        if self.check_existing_swap_file(swapfile, swaplist, size):
             return
 
         if os.path.isfile(swapfile) and os.path.getsize(swapfile) != size:
@@ -144,20 +160,24 @@ class FreeBSDResourceDiskHandler(ResourceDiskHandler):
         if not os.path.isfile(swapfile):
             logger.info("Create swap file")
             self.mkfile(swapfile, size_kb * 1024)
-        
-        mddevice = shellutil.run_get_output("mdconfig -a -t vnode -f {0}".format(swapfile))[1].rstrip()
+
+        mddevice = shellutil.run_get_output(
+            "mdconfig -a -t vnode -f {0}".format(swapfile))[1].rstrip()
         shellutil.run("chmod 0600 /dev/{0}".format(mddevice))
-        
+
         if conf.get_resourcedisk_enable_swap_encryption():
             shellutil.run("kldload aesni")
             shellutil.run("kldload cryptodev")
             shellutil.run("kldload geom_eli")
-            shellutil.run("geli onetime -e AES-XTS -l 256 -d /dev/{0}".format(mddevice))
+            shellutil.run(
+                "geli onetime -e AES-XTS -l 256 -d /dev/{0}".format(mddevice))
             shellutil.run("chmod 0600 /dev/{0}.eli".format(mddevice))
             if shellutil.run("swapon /dev/{0}.eli".format(mddevice)):
                 raise ResourceDiskError("/dev/{0}.eli".format(mddevice))
-            logger.info("Enabled {0}KB of swap at /dev/{1}.eli ({2})".format(size_kb, mddevice, swapfile))
+            logger.info(
+                "Enabled {0}KB of swap at /dev/{1}.eli ({2})".format(size_kb, mddevice, swapfile))
         else:
             if shellutil.run("swapon /dev/{0}".format(mddevice)):
                 raise ResourceDiskError("/dev/{0}".format(mddevice))
-            logger.info("Enabled {0}KB of swap at /dev/{1} ({2})".format(size_kb, mddevice, swapfile))
+            logger.info(
+                "Enabled {0}KB of swap at /dev/{1} ({2})".format(size_kb, mddevice, swapfile))


Test patch:
diff --git a/tests/distro/test_resourceDisk.py b/tests/distro/test_resourceDisk.py
index d2ce6e16..5f9db0a3 100644
--- a/tests/distro/test_resourceDisk.py
+++ b/tests/distro/test_resourceDisk.py
@@ -18,6 +18,8 @@
 # http://msdn.microsoft.com/en-us/library/cc227282%28PROT.10%29.aspx
 # http://msdn.microsoft.com/en-us/library/cc227259%28PROT.13%29.aspx
 
+import os
+import stat
 import sys
 from azurelinuxagent.common.utils import shellutil
 from azurelinuxagent.daemon.resourcedisk import get_resourcedisk_handler
@@ -38,6 +40,11 @@ class TestResourceDisk(AgentTestCase):
         # assert
         assert os.path.exists(test_file)
 
+        # only the owner should have access
+        mode = os.stat(test_file).st_mode & (
+            stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)
+        assert mode == stat.S_IRUSR | stat.S_IWUSR
+
         # cleanup
         os.remove(test_file)
 
@@ -49,7 +56,7 @@ class TestResourceDisk(AgentTestCase):
             file_size = 1024 * 128
 
             # execute
-            if sys.version_info >= (3,3):
+            if sys.version_info >= (3, 3):
                 with patch("os.posix_fallocate",
                            side_effect=Exception('failure')):
                     get_resourcedisk_handler().mkfile(test_file, file_size)
@@ -76,20 +83,20 @@ class TestResourceDisk(AgentTestCase):
             resource_disk_handler.mkfile(test_file, file_size)
 
             # assert
-            if sys.version_info >= (3,3):
+            if sys.version_info >= (3, 3):
                 with patch("os.posix_fallocate") as posix_fallocate:
                     self.assertEqual(0, posix_fallocate.call_count)
 
             assert run_patch.call_count == 1
             assert "dd if" in run_patch.call_args_list[0][0][0]
 
-
     def test_change_partition_type(self):
         resource_handler = get_resourcedisk_handler()
         # test when sfdisk --part-type does not exist
         with patch.object(shellutil, "run_get_output",
                           side_effect=[[1, ''], [0, '']]) as run_patch:
-            resource_handler.change_partition_type(suppress_message=True, option_str='')
+            resource_handler.change_partition_type(
+                suppress_message=True, option_str='')
 
             # assert
             assert run_patch.call_count == 2
@@ -99,12 +106,42 @@ class TestResourceDisk(AgentTestCase):
         # test when sfdisk --part-type exists
         with patch.object(shellutil, "run_get_output",
                           side_effect=[[0, '']]) as run_patch:
-            resource_handler.change_partition_type(suppress_message=True, option_str='')
+            resource_handler.change_partition_type(
+                suppress_message=True, option_str='')
 
             # assert
             assert run_patch.call_count == 1
             assert "sfdisk --part-type" in run_patch.call_args_list[0][0][0]
 
+    def test_check_existing_swap_file(self):
+        test_file = os.path.join(self.tmp_dir, 'test_swap_file')
+        file_size = 1024 * 128
+        if os.path.exists(test_file):
+            os.remove(test_file)
+
+        with open(test_file, "wb") as file:
+            file.write(bytearray(file_size))
+
+        os.chmod(test_file, stat.S_ISUID | stat.S_ISGID | stat.S_IRUSR |
+                 stat.S_IWUSR | stat.S_IRWXG | stat.S_IRWXO)  # 0o6677
+
+        def swap_on(_):   # mimic the output of "swapon -s"
+            return [
+                "Filename   Type        Size      Used  Priority",
+                "{0}        partition	16498684  0     -2".format(test_file)
+            ]
+
+        with patch.object(shellutil, "run_get_output", side_effect=swap_on):
+            get_resourcedisk_handler().check_existing_swap_file(
+                test_file, test_file, file_size)
+
+        # it should remove access from group, others
+        mode = os.stat(test_file).st_mode & (stat.S_ISUID | stat.S_ISGID |
+                                             stat.S_IRWXU | stat.S_IWUSR | stat.S_IRWXG | stat.S_IRWXO)  # 0o6777
+        assert mode == stat.S_ISUID | stat.S_ISGID | stat.S_IRUSR | stat.S_IWUSR  # 0o6600
+
+        os.remove(test_file)
+
 
 if __name__ == '__main__':
     unittest.main()