Merge lp:~sergiusens/snapcraft/1503495 into lp:~snappy-dev/snapcraft/core

[Sergio Schvezov]

Removing suid/guid bits from extracted debs

[lp:~sergiusens/snapcraft/1503495]

235. By Sergio Schvezov on 2015-10-09

Removing suid/guid bits from extracted debs

[John Lenton (chipaca)]

Yes.

review: Approve

=== modified file 'snapcraft/repo.py'
--- snapcraft/repo.py	2015-10-08 19:25:43 +0000
+++ snapcraft/repo.py	2015-10-09 21:50:28 +0000
@@ -22,6 +22,7 @@
 import platform
 import string
 import shutil
+import stat
 import subprocess
 import urllib
 import urllib.request
@@ -133,7 +134,7 @@
             except subprocess.CalledProcessError:
                 raise UnpackError(pkg)
 
-        _fix_symlinks(rootdir)
+        _fix_contents(rootdir)
 
     def _manifest_dep_names(self):
         manifest_dep_names = set()
@@ -223,11 +224,14 @@
     return apt_cache, progress
 
 
-def _fix_symlinks(debdir):
+def _fix_contents(debdir):
     '''
     Sometimes debs will contain absolute symlinks (e.g. if the relative
     path would go all the way to root, they just do absolute).  We can't
     have that, so instead clean those absolute symlinks.
+
+    Some unpacked items will also contain suid binaries which we do not want in
+    the resulting snap.
     '''
     for root, dirs, files in os.walk(debdir):
         # Symlinks to directories will be in dirs, while symlinks to
@@ -244,6 +248,15 @@
                         continue
                 os.remove(path)
                 os.symlink(os.path.relpath(target, root), path)
+            elif os.path.exists(path):
+                _fix_filemode(path)
+
+
+def _fix_filemode(path):
+    mode = stat.S_IMODE(os.stat(path).st_mode)
+    if mode & 0o4000 or mode & 0o2000:
+        logger.debug('Removing suid/guid from {}'.format(path))
+        os.chmod(path, mode & 0o1777)
 
 
 _skip_list = None

=== modified file 'snapcraft/tests/test_repo.py'
--- snapcraft/tests/test_repo.py	2015-10-08 19:25:43 +0000
+++ snapcraft/tests/test_repo.py	2015-10-09 21:50:28 +0000
@@ -17,6 +17,7 @@
 import fixtures
 import logging
 import os
+import stat
 import tempfile
 import unittest.mock
 
@@ -26,6 +27,14 @@
 
 class UbuntuTestCase(tests.TestCase):
 
+    def setUp(self):
+        super().setUp()
+        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
+        self.useFixture(fake_logger)
+        tempdirObj = tempfile.TemporaryDirectory()
+        self.addCleanup(tempdirObj.cleanup)
+        self.tempdir = tempdirObj.name
+
     @unittest.mock.patch('snapcraft.repo._get_geoip_country_code_prefix')
     def test_sources_amd64_vivid(self, mock_cc):
         mock_cc.return_value = 'ar'
@@ -66,25 +75,37 @@
         self.assertFalse(mock_cc.called)
 
     def test_fix_symlinks(self):
-        fake_logger = fixtures.FakeLogger(level=logging.ERROR)
-        self.useFixture(fake_logger)
-        tempdirObj = tempfile.TemporaryDirectory()
-        self.addCleanup(tempdirObj.cleanup)
-        tempdir = tempdirObj.name
-
-        os.makedirs(tempdir + '/a')
-        open(tempdir + '/1', mode='w').close()
-
-        os.symlink('a', tempdir + '/rel-to-a')
-        os.symlink('/a', tempdir + '/abs-to-a')
-        os.symlink('/b', tempdir + '/abs-to-b')
-        os.symlink('1', tempdir + '/rel-to-1')
-        os.symlink('/1', tempdir + '/abs-to-1')
-
-        repo._fix_symlinks(debdir=tempdir)
-
-        self.assertEqual(os.readlink(tempdir + '/rel-to-a'), 'a')
-        self.assertEqual(os.readlink(tempdir + '/abs-to-a'), 'a')
-        self.assertEqual(os.readlink(tempdir + '/abs-to-b'), '/b')
-        self.assertEqual(os.readlink(tempdir + '/rel-to-1'), '1')
-        self.assertEqual(os.readlink(tempdir + '/abs-to-1'), '1')
+        os.makedirs(self.tempdir + '/a')
+        open(self.tempdir + '/1', mode='w').close()
+
+        os.symlink('a', self.tempdir + '/rel-to-a')
+        os.symlink('/a', self.tempdir + '/abs-to-a')
+        os.symlink('/b', self.tempdir + '/abs-to-b')
+        os.symlink('1', self.tempdir + '/rel-to-1')
+        os.symlink('/1', self.tempdir + '/abs-to-1')
+
+        repo._fix_contents(debdir=self.tempdir)
+
+        self.assertEqual(os.readlink(self.tempdir + '/rel-to-a'), 'a')
+        self.assertEqual(os.readlink(self.tempdir + '/abs-to-a'), 'a')
+        self.assertEqual(os.readlink(self.tempdir + '/abs-to-b'), '/b')
+        self.assertEqual(os.readlink(self.tempdir + '/rel-to-1'), '1')
+        self.assertEqual(os.readlink(self.tempdir + '/abs-to-1'), '1')
+
+    def test_fix_suid(self):
+        files = {
+            'suid_file': (0o4765, 0o0765),
+            'guid_file': (0o2777, 0o0777),
+            'suid_guid_file': (0o6744, 0o0744),
+            'suid_guid_sticky_file': (0o7744, 0o1744),
+        }
+
+        for key in files:
+            with self.subTest(key=key):
+                file = os.path.join(self.tempdir, key)
+                open(file, mode='w').close()
+                os.chmod(file, files[key][0])
+
+                repo._fix_contents(debdir=self.tempdir)
+                self.assertEqual(
+                    stat.S_IMODE(os.stat(file).st_mode), files[key][1])