Security: Out-of-bounds read in importshp plugin, due to mismatched sizes in DBF file header

[eldstal]

Summary

An out-of-bounds read on a heap buffer in the importshp plugin may allow an attacker to read unintended data.

Cause

A DBF file has the following structure:

* file header
   * number of records
   * record length
   * size of file header + size of field headers
* 1 or more field headers
   * field type
   * field length (bytes)
* 1 or more records
   * 1 or more field data blobs

Each record contains all fields, in order, and thus each record is of the same size.

The dbfopen.c module parses the file and field headers into an internal structure, saving the record length as psDBF->nRecordLength as well as calculating the byte offset of each field within the record using each field's individual field length. These offsets are stored in the array psDBF->panFieldOffset[].

To access file data, the buffer psDBF->pszCurrentRecord is allocated on the heap at dbfopen.c:487, and given the size nRecordLength which comes straight from the input file.

When a record is accessed, at dbfopen.c:1000, the offset from the data buffer is based on the field length (as precomputed into psDBF->panFieldOffset[]) and the length of the transfer is directly taken from the field length.

There is no sanity check on the nRecordLength, which means it can be significantly smaller than the actual size of a record (the sum of all field lengths). As a result, an input file with a small nRecordLength causes a small heap buffer to be allocated, while a set of large field length can make the strncpy() copy more data than intended, reading outside the bounds of the heap buffer.

Steps to reproduce

1. Unzip the provided proof of concept (an shp and a dbf file)
2. Start LibreCAD in a debugger.
3. Set a breakpoint at dbfopen.c:1000
4. Plugins/ESRI Shapefile
5. Click "File..." and load the provided SHP file
6. Select "From data" in all radio boxes and pick one of the Megatext fields in each dropdown
7. Click "Accept"

When the breakpoint triggers, observe the parameters to strncpy and verify that the size parameter exceeds the allocated size (nRecordLength) of the source buffer, and/or that the offset from pabyRec leads to the source starting outside the buffer.

The attached proof of concept has been modified to trigger the bug, but does not always lead to a crash. The actual record size is quite large, consisting of several 255 byte data fields, but the header sets nRecordLength = 48

Impact

Possible leak of protected data. An out-of-bounds read can be used to bypass automatic security features such as stack canaries and pointer encryption.

Proposed mitigation

Update the bundled shapelib with an up-to-date version from OSGeo/shapelib, which includes a sanity check for the record length in the DBF header.

Vulnerable version

• LibreCAD (commit eba1413)

[eldstal]

As more than a year has passed since report, I've taken the liberty of requesting a CVE for this vulnerability. Please consider updating the embedded shapelib version to mitigate this.

[eldstal]

CVE-2023-30259 has been assigned to this vulnerability.

[dxli]

The code quality of the shapelib is not good.

Should we remove it from LibreCAD?

If it's still relevant, someone should rewrite the lib by safer implementation:

1. avoid C-style array;
2. avoid C-style memory management;
3. add abstraction layers and building block data structures.

[lordofbikes]

I agree, we simply remove the importshp plugin. If this cause an outcry, we can restore it from git anytime.

[carnil]

CVE-2023-30259 appears to have assigned for this issue

[lordofbikes]

If anybody is missing the importshp plugin, please say so.
We can then revert this commit in a new branch and do some code refactoring there.

[u2fly]

We can then revert this commit in a new branch and do some code refactoring there.

Is GDAL (ogr2ogr), as external convertor, is a good replacement for importshp?