id	summary	reporter	owner	description	type	status	component	version	severity	resolution	keywords	cc	stage	has_patch	needs_docs	needs_tests	needs_better_patch	easy	ui_ux
37289	GDALRaster.__del__() shouldn't delete rasters opened in read-only mode	Jacob Walls	Django Sprints	"`GDALRaster.__del__()`, implemented in #28300, deletes the raster file that backs the python object if it's located on GDAL's [https://docs.djangoproject.com/en/dev/ref/contrib/gis/gdal/#gdal-raster-vsimem 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:

{{{#!py
    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:
{{{#!py
# 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 [https://docs.djangoproject.com/en/dev/ref/contrib/gis/gdal/#using-other-virtual-filesystems 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:
{{{#!diff
diff --git a/django/contrib/gis/gdal/raster/source.py b/django/contrib/gis/gdal/raster/source.py
index 42ab1e3a70..d3a6e7deee 100644
--- a/django/contrib/gis/gdal/raster/source.py
+++ b/django/contrib/gis/gdal/raster/source.py
@@ -217,7 +217,10 @@ class GDALRaster(GDALRasterBase):
             )
 
     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,9 +279,7 @@ class GDALRaster(GDALRasterBase):
 
     @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,6 +296,10 @@ class GDALRaster(GDALRasterBase):
     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):
         """"""
diff --git a/tests/gis_tests/gdal_tests/test_raster.py b/tests/gis_tests/gdal_tests/test_raster.py
index ca7251914b..7c21a9c343 100644
--- a/tests/gis_tests/gdal_tests/test_raster.py
+++ b/tests/gis_tests/gdal_tests/test_raster.py
@@ -283,6 +283,9 @@ class GDALRasterTests(SimpleTestCase):
         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."	Bug	assigned	GIS	5.2	Release blocker		not-security, gdal	Jordi Castells Daniel Wiesmann	Unreviewed	0	0	0	0	0	0