[img] heap-buffer-overflow reading a v8 .3d with a non-default separator and no coordinate system

[Philip Schuchardt]

dump3d on the .3d cavern writes for tests/wallsdiving.srv aborts under AddressSanitizer:

==33594==ERROR: AddressSanitizer: heap-buffer-overflow READ of size 6 at 0x602000000120 thread T0

1 memcmp

2 img_read_stream_survey img.c:1400

3 img_for_survex_open_survey img_for_survex.c:46

4 main dump3d.c:92

0x602000000120 is located 0 bytes after 16-byte region allocated by getline_alloc_len img.c:555 from img_read_stream_survey img.c:1387

The title line of that file is "wallsdiving\0\0:" - the writer (img.c:1687) adds the hidden fields whenever the separator isn't '.', which is the case for every Walls import, so the cs field is an empty string. The reader then does

if (memcmp(cs, "+init=", 6) == 0) {

on it, which reads 6 bytes from a 0 length string. The line fits in getline_alloc_len's initial 16 byte buffer, so that runs 2 bytes past the end of the allocation. A .svx with no *cs and the default separator doesn't trip it because the extra fields aren't written at all.

Same shape one level down: with a cs of "+init=e" the p[4] / p[5] checks and the memcmp against "epsg"/"esri" read past the end too.

Seen with the clang ASan build on macOS arm64; the sanitisers CI job may not be reaching dump3d on the Walls tests. Unrelated to #150, found while running the full cavern.tst for that patch.

[Philip Schuchardt]

Fix for this issue

[Philip Schuchardt]

Patch attached!

[Olly Betts]

Thanks, except:

tests/wallsdiving.srv already triggers this: running dump3d on the .3d cavern writes for it reports a heap-buffer-overflow under AddressSanitizer, so the sanitisers and valgrind CI jobs cover the fix.

Both those jobs are green before your fix though (so if your fix later got reverted CI would still pass...)

[Olly Betts]

It seems the difference is gcc vs clang; the gcc sanitisers seem to miss this, but that's what CI currently uses. There's a comment:

  # Job with asan+ubsan enabled.  Uses GCC - the clang versions of the
  # sanitisers support some extra things, but I failed to get the build to work
  # with clang+sanitisers.  In case we want to try again in the future, I think
  # it needs -stdlib=libstdc++ because we used a packaged wxwidgets, which is a
  # C++ library and built with GCC (and so libstdc++).

I can get a working build on Debian unstable using clang with the same sanitisers enabled, which is interesting. I only added this job in late June, so it's unlikely the comment is out of date, but given it works on Debian unstable I'll try again. We could perhaps run this job in a docker container if necessary.

Incidentally, this can easily be triggered with .svx data too - e.g. by matching the filename length of wallsdiving.srv using an existing testcase file:

$ cd tests
$ cp cmd_set_dot_in_name.svx tmpxxxxxxxx.svx
$ ../src/cavern tmpxxxxxxxx.svx
$ ../src/dump3d tmpxxxxxxxx.3d

[Philip Schuchardt]

I'm glad you were able to reproduce it. Seems very reproducible on my mac.

[Olly Betts]

Changing the sanitisers CI job to use ubuntu-26.04 and switching it to clang reproduces the problem in CI, so that was easy to resolve.

[Olly Betts]

Merged: 2e93f1d09617add147b025d528a06acbc31154e7