[Django] #37289: GDALRaster.__del__() shouldn't delete rasters opened in read-only mode

[Django]

#37289: GDALRaster.__del__() shouldn't delete rasters opened in read-only mode
-------------------------------------+-------------------------------------
Reporter: Jacob | Owner: Django Sprints
Walls |
Type: Bug | Status: assigned
Component: GIS | Version: 5.2
Severity: Release | Keywords: not-security, gdal
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37289>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

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

Reporter: Jacob Walls | Owner: Django
| Sprints

Type: Bug | Status: assigned
Component: GIS | Version: 5.2

Severity: Release blocker | Resolution:
Keywords: not-security, gdal | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/37289#comment:1>

[Django]

#37289: GDALRaster.__del__() shouldn't delete rasters opened in read-only mode
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Django
| Sprints
Type: Bug | Status: assigned
Component: GIS | Version: 5.2
Severity: Release blocker | Resolution:
Keywords: not-security, gdal | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jacob Walls:

Old description:

New description:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/37289#comment:2>

[Django]

`GDALRaster` defaults the `write` parameter to `False`. Paths are one of
the input classes where that parameter is respected instead of overwritten

later via `self._write = 1`. As an implementation hint, I would expect

+ # retry with GDALRaster(..., write=True)

+ # 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.

--
--

Ticket URL: <https://code.djangoproject.com/ticket/37289#comment:3>

[Django]

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

Reporter: Jacob Walls | Owner: Jacob
| Walls

Type: Bug | Status: assigned
Component: GIS | Version: 5.2
Severity: Release blocker | Resolution:
Keywords: not-security, gdal | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* owner: Django Sprints => Jacob Walls

--
Ticket URL: <https://code.djangoproject.com/ticket/37289#comment:4>

[Django]

#37289: GDALRaster.__del__() shouldn't delete rasters opened in read-only mode
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: GIS | Version: 5.2
Severity: Release blocker | Resolution:
Keywords: not-security, gdal | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* has_patch: 0 => 1

Comment:

On second look, I don't think we need to provide any sort of deletion
behavior here. (The `_write` flag has to do with whether GDAL can update
files, not whether it's the owner and should auto-delete.)

[https://github.com/django/django/pull/21881 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/37289#comment:5>