GDALRaster.__del__() shouldn't delete rasters opened in read-only mode

[Jacob Walls]

GDALRaster.__del__(), implemented in #28300, deletes the raster file that backs the python object if it's located on GDAL's ​virtual filesystem, because at the time, the only virtual filesystem in use was the in-memory one, and the principal (only?) use was for temporary files, as you can see from this comment:

    def __del__(self):
        if self.is_vsi_based:
            # Remove the temporary file from the VSI in-memory filesystem.
            capi.unlink_vsi_file(force_bytes(self.name))
        super().__del__()

... and this constant:

# Should the memory file system take ownership of the buffer, freeing it when
# the file is deleted? (No, GDALRaster.__del__() will delete the buffer.)
VSI_TAKE_BUFFER_OWNERSHIP = False

Later, by adding support for all virtual fileystems, #32670 added support for compressed and network rasters, see ​docs.

Since GDALRaster.__del__() was not adjusted, a security report observed that when the python object dies, a delete is fired off and could delete a network raster from a commercial storage provider like S3 (using the vsis3 driver).

The Security Team rejected the report on the basis that even assuming some other legitimate use for assigning delete permissions to the web process user, e.g. in some other workflow, since this deletion happens on every legitimate use as well, this would be discovered immediately in even the most preliminary user acceptance testing, and the feature would be withdrawn, worked around, or reconfigured with reduced permissions well before an attacker could reach it. (But potentially after a few unintentionally deleted rasters, hence marking as a data loss bug / 5.2 release blocker.)

Thanks "garden_" for the informative report.

---

GDALRaster defaults the write parameter to False. Paths are one of the input classes where that parameter is respected instead of overwritten back to True. As an implementation hint, I would expect something like:

django/contrib/gis/gdal/raster/source.py

@@ -217 +217 @@
             )
 
     def __del__(self):
-        if self.is_vsi_based:
+        if (
+            (self.is_vsi_based and self._write)
+            or self.is_vsi_in_memory_based
+        ):
             # Remove the temporary file from the VSI in-memory filesystem.
             capi.unlink_vsi_file(force_bytes(self.name))
         super().__del__()
@@ -276 +279 @@
 
     @property
     def vsi_buffer(self):
-        if not (
-            self.is_vsi_based and self.name.startswith(VSI_MEM_FILESYSTEM_BASE_PATH)
-        ):
+        if not self.is_vsi_in_memory_based:
             return None
         # Prepare an integer that will contain the buffer length.
         out_length = c_int()
@@ -295 +296 @@
     def is_vsi_based(self):
         return self._ptr and self.name.startswith(VSI_FILESYSTEM_PREFIX)
 
+    @cached_property
+    def is_vsi_in_memory_based(self):  # help with naming, please
+        return self._ptr and self.name.startswith(VSI_MEM_FILESYSTEM_BASE_PATH)
+
     @property
     def name(self):
         """

tests/gis_tests/gdal_tests/test_raster.py

@@ -283 +283 @@
         self.assertEqual(rst.name, rst_path)
         self.assertIs(rst.is_vsi_based, True)
         self.assertIsNone(rst.vsi_buffer)
+        # assert the virtual file is still there, needs release note...
+        # retry with GDALRaster(..., write=False)
+        # assert the virtual file is gone...
 
     def test_offset_size_and_shape_on_raster_creation(self):
         rast = GDALRaster(

... but that will need a release note as in the compressed raster case (e.g. /vsizip/..., this will require passing write=True to "take ownership" of the delete). CCing other knowledgable folks for opinions.