Repository navigation
lib/raster: guard compressed row memcpy size - #8015
Conversation
Addresses CID 1687668 introduced by 6e39994
|
My AI review found something, I asked it to create a real test to prove the overflow it speculated, I’ll come back later when it’s done |
|
Reading a crafted compressed CELL raster still overflows the row buffer with this PR applied — the new Patch (fix + a pytest that fails with a heap-buffer-overflow without the guard and passes with it): diff --git a/lib/raster/get_row.c b/lib/raster/get_row.c
index 6540f64..c1bc65a 100644
--- a/lib/raster/get_row.c
+++ b/lib/raster/get_row.c
@@ -180,6 +180,17 @@ static void read_data_compressed(int fd, int row, unsigned char *data_buf,
/* one byte is nbyte count */
n = *nbytes = *cmp++;
readamount -= 1;
+
+ /* The per-cell byte count is read from the file and must not exceed
+ * the bytes-per-cell the data buffer was allocated for (fcb->nbytes
+ * from the map header); a larger value indicates a corrupted file and
+ * would otherwise overflow data_buf below. */
+ if (n > fcb->nbytes) {
+ G_free(cmp2);
+ G_fatal_error(
+ _("Invalid byte count in raster data for row %d of <%s>"), row,
+ fcb->name);
+ }
}
else
/* pre 3.0 compression */
diff --git a/lib/raster/tests/lib_raster_get_row_corrupt_test.py b/lib/raster/tests/lib_raster_get_row_corrupt_test.py
new file mode 100644
index 0000000..957e6a9
--- /dev/null
+++ b/lib/raster/tests/lib_raster_get_row_corrupt_test.py
@@ -0,0 +1,55 @@
+"""Tests that reading a corrupted compressed CELL raster fails gracefully
+
+(C) 2025 by the GRASS Development Team
+
+This program is free software under the GNU General Public License
+(>=v2). Read the file COPYING that comes with GRASS for details.
+"""
+
+import struct
+from pathlib import Path
+
+import pytest
+
+from grass.exceptions import CalledModuleError
+from grass.tools import Tools
+
+
+def test_compressed_cell_row_byte_count_overflow(session):
+ """A crafted per-cell byte count must not overflow the row buffer
+
+ read_data_compressed() reads the per-cell byte count from the first byte
+ of each compressed row and computes the copy size as count * cols. The
+ row buffer, however, is allocated for the bytes-per-cell recorded in the
+ map header. A corrupted file whose byte count exceeds that header value
+ would make the copy write past the end of the buffer (a heap overflow,
+ reported by Coverity). Reading such a file must fail with an error instead.
+ """
+ tools = Tools(session=session)
+ cols = 4
+ tools.g_region(rows=1, cols=cols, n=1, s=0, e=cols, w=0, env=session.env)
+ # Values 0..cols fit in one byte, so the map header records one byte per
+ # cell and the read buffer is cols bytes long.
+ tools.r_mapcalc(expression="corrupt = col()", env=session.env)
+
+ cell_file = Path(
+ tools.g_findfile(
+ element="cell", file="corrupt", format="json", env=session.env
+ )["file"]
+ )
+
+ # Rewrite the cell file as a single compressed row whose first byte claims
+ # 255 bytes per cell. The row pointer table is one leading byte giving the
+ # offset width (8) followed by one big-endian offset per row boundary.
+ offset_width = 8
+ table_len = 1 + (1 + 1) * offset_width
+ byte_count = 255
+ row_payload = bytes([byte_count]) + b"\x00" * (byte_count * cols)
+
+ table = bytearray([offset_width])
+ table += struct.pack(">q", table_len)
+ table += struct.pack(">q", table_len + len(row_payload))
+ cell_file.write_bytes(bytes(table) + row_payload)
+
+ with pytest.raises(CalledModuleError, match="byte count"):
+ tools.r_out_ascii(input="corrupt", env=session.env) |
The per-cell byte count is read from the file and must not exceed the bytes-per-cell the data buffer was allocated for (fcb->nbytes from the map header); a larger value indicates a corrupted file and would otherwise overflow data_buf below.
Thanks, makes sense, added the suggested check. |
Addresses Coverity Scan CID 1687668 introduced by 6e39994