Merge remote-tracking branch 'remediate/master'

newsfragments/3824.security

@@ -0,0 +1 @@
+The storage server implementation no longer records corruption advisories about storage indexes for which it holds no shares.

src/allmydata/storage/server.py

@@ -77,9 +77,9 @@ class StorageServer(service.MultiService, Referenceable):
         sharedir = os.path.join(storedir, "shares")
         fileutil.make_dirs(sharedir)
         self.sharedir = sharedir
-        # we don't actually create the corruption-advisory dir until necessary
         self.corruption_advisory_dir = os.path.join(storedir,
                                                     "corruption-advisories")
+        fileutil.make_dirs(self.corruption_advisory_dir)
         self.reserved_space = int(reserved_space)
         self.no_storage = discard_storage
         self.readonly_storage = readonly_storage
@@ -730,6 +730,21 @@ class StorageServer(service.MultiService, Referenceable):
         self.add_latency("readv", self._get_current_time() - start)
         return datavs
 
+    def _share_exists(self, storage_index, shnum):
+        """
+        Check local share storage to see if a matching share exists.
+
+        :param bytes storage_index: The storage index to inspect.
+        :param int shnum: The share number to check for.
+
+        :return bool: ``True`` if a share with the given number exists at the
+            given storage index, ``False`` otherwise.
+        """
+        for existing_sharenum, ignored in self._get_bucket_shares(storage_index):
+            if existing_sharenum == shnum:
+                return True
+        return False
+
     def remote_advise_corrupt_share(self, share_type, storage_index, shnum,
                                     reason):
         # This is a remote API, I believe, so this has to be bytes for legacy
@@ -739,18 +754,27 @@ class StorageServer(service.MultiService, Referenceable):
 
         si_s = si_b2a(storage_index)
 
+        if not self._share_exists(storage_index, shnum):
+            log.msg(
+                format=(
+                    "discarding client corruption claim for %(si)s/%(shnum)d "
+                    "which I do not have"
+                ),
+                si=si_s,
+                shnum=shnum,
+            )
+            return
+
         log.msg(format=("client claims corruption in (%(share_type)s) " +
                         "%(si)s-%(shnum)d: %(reason)s"),
                 share_type=share_type, si=si_s, shnum=shnum, reason=reason,
                 level=log.SCARY, umid="SGx2fA")
 
-        fileutil.make_dirs(self.corruption_advisory_dir)
-        now = time_format.iso_utc(sep="T")
-
         report = render_corruption_report(share_type, si_s, shnum, reason)
         if len(report) > self.get_available_space():
             return None
 
+        now = time_format.iso_utc(sep="T")
         report_path = get_corruption_report_path(
             self.corruption_advisory_dir,
             now,

src/allmydata/test/test_storage.py

@@ -1018,6 +1018,7 @@ class Server(unittest.TestCase):
         ss = StorageServer(workdir, b"\x00" * 20, discard_storage=True)
         ss.setServiceParent(self.sparent)
 
+        upload_immutable(ss, b"si0", b"r" * 32, b"c" * 32, {0: b""})
         ss.remote_advise_corrupt_share(b"immutable", b"si0", 0,
                                        b"This share smells funny.\n")
 
@@ -1032,6 +1033,7 @@ class Server(unittest.TestCase):
         ss.setServiceParent(self.sparent)
 
         si0_s = base32.b2a(b"si0")
+        upload_immutable(ss, b"si0", b"r" * 32, b"c" * 32, {0: b""})
         ss.remote_advise_corrupt_share(b"immutable", b"si0", 0,
                                        b"This share smells funny.\n")
         reportdir = os.path.join(workdir, "corruption-advisories")
@@ -1070,6 +1072,26 @@ class Server(unittest.TestCase):
         self.failUnlessIn(b"share_number: 1", report)
         self.failUnlessIn(b"This share tastes like dust.", report)
 
+    def test_advise_corruption_missing(self):
+        """
+        If a corruption advisory is received for a share that is not present on
+        this server then it is not persisted.
+        """
+        workdir = self.workdir("test_advise_corruption_missing")
+        ss = StorageServer(workdir, b"\x00" * 20, discard_storage=True)
+        ss.setServiceParent(self.sparent)
+
+        # Upload one share for this storage index
+        upload_immutable(ss, b"si0", b"r" * 32, b"c" * 32, {0: b""})
+
+        # And try to submit a corruption advisory about a different share
+        ss.remote_advise_corrupt_share(b"immutable", b"si0", 1,
+                                       b"This share smells funny.\n")
+
+        self.assertEqual(
+            [],
+            os.listdir(ss.corruption_advisory_dir),
+        )
 
 
 class MutableServer(unittest.TestCase):