Stack-based buffer overflow in the spiro_to_bpath0() function

[fcambus]

Hi,

When building libspiro 20190731 with AddressSanitizer enabled and running the test suite, I found a stack-based buffer overflow in spiro_to_bpath0() in spiro.c.

In the test suite, call-test14 to call-test19 all trigger the same issue.

The issue can be triggered as follow:

./configure CFLAGS="-fsanitize=address"
make
./tests/call-test14

=================================================================
==1829==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffcf5a2c938 at pc 0x7fdc1174d487 bp 0x7ffcf5a2c450 sp 0x7ffcf5a2c448
WRITE of size 8 at 0x7ffcf5a2c938 thread T0
    #0 0x7fdc1174d486 in spiro_to_bpath0 /home/fcambus/libspiro-20190731/spiro.c:1182:11
    #1 0x4c9ea0 in test_curve /home/fcambus/libspiro-20190731/tests/./call-test.c:666:5
    #2 0x4ca119 in main /home/fcambus/libspiro-20190731/tests/./call-test.c:1172:9
    #3 0x7fdc1152e1e2 in __libc_start_main /build/glibc-4WA41p/glibc-2.30/csu/../csu/libc-start.c:308:16
    #4 0x41b31d in _start (/home/fcambus/libspiro-20190731/tests/.libs/call-test14+0x41b31d)

Address 0x7ffcf5a2c938 is located in stack of thread T0 at offset 632 in frame
    #0 0x4c832f in test_curve /home/fcambus/libspiro-20190731/tests/./call-test.c:536

  This frame has 3 object(s):
    [32, 416) 'spiro' (line 537)
    [480, 548) 'nextknot' (line 538)
    [592, 632) 'd' (line 539) <== Memory access at offset 632 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /home/fcambus/libspiro-20190731/spiro.c:1182:11 in spiro_to_bpath0
Shadow bytes around the buggy address:
  0x10001eb3d8d0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00
  0x10001eb3d8e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001eb3d8f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001eb3d900: 00 00 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 f2
  0x10001eb3d910: f2 f2 f2 f2 00 00 00 00 00 00 00 00 04 f2 f2 f2
=>0x10001eb3d920: f2 f2 00 00 00 00 00[f3]f3 f3 f3 f3 00 00 00 00
  0x10001eb3d930: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001eb3d940: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001eb3d950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001eb3d960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10001eb3d970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==1829==ABORTING

[ctrlcctrlv]

How could this have been missed?

https://github.com/fontforge/libspiro/blob/6c04efe38570a09d2a0ffd3e5d8ea9117a7f3033/tests/call-test.c#L539 https://github.com/fontforge/libspiro/blob/6c04efe38570a09d2a0ffd3e5d8ea9117a7f3033/spiro.c#L1182

C arrays are 0-indexed! This could never have worked, could it??

[JoesCat]

Yes, you are correct, dm[5] = 1. would be a problem. (line 539 should be dm[6] instead of dm[5] like in spiroentrypoint.c line 48)

Also noticed line 548

d[0] = 1.; d[1] = d[1] = 0.;

needs to be fixed as:

d[0] = 1.; d[1] = d[2] = 0.;

@fcambus, can you do those two mods and try it again? If it works, are you interested/able to formulate a patch - we can also merge it in.

[fcambus]

This issue has been assigned CVE-2019-19847.

[fcambus]

@JoesCat Sure, will try your modifications when I come back home and report back.

[JoesCat]

For clarification - the buffer overflow happens in the test programs, not the library itself.

call-testN do progressive tests of the library, so it does some calls that aren't defined entrypoints in the developer H files - anyone that uses the libspiro H files shouldn't need to be playing around with these dm[6] variables.

Thanks! - good catch - will look forwards to seeing a patch.

[fcambus]

Well, the buffer overflow happens in spiro_to_bpath0() which is a library function and an exported symbol:

nm /usr/lib/libspiro.so.1.0.0 | grep spiro_to_bpath0
00000000000052f0 T spiro_to_bpath0

And you also define its prototype in spiro.h which is installed in PREFIX/include.

But I understand it is called directly in the test suite for testing purposes but is not supposed to be called directly by library users.

Your modifications indeed fix the crashes in the test suite, so you should commit that as a first step. As a second step, you should probably stop exporting this function.

[ctrlcctrlv]

We should first make sure it's not being used, as it's likely been exported for years. I checked—it's not in use by FontForge. @JoesCat would know best as to other projects to check.

[fcambus]

Debian Code Search shows it is apparently only used in libspiro itself: https://codesearch.debian.net/search?q=spiro_to_bpath0&literal=1

[JoesCat]

Likewise, internet search for spiro_to_bpath0 also showed nothing except for the CVE-2019-19847.

Most users that come to mind, use the "advertised" API entries, examples: FontForge, FontAnvil, GEGL ...but I'd rather avoid to stop exporting the function since there is a possibility someone could make use of it, one example that comes to mind is Inkscape's forked version (however, this was based on 20071029 code). It demonstrates that there can be an advantage for someone knowing what they're doing at this lower level.

I've verified ./configure CFLAGS="-fsanitize=address" and the patch solution and have also started looking at fortifying this entry point even though it's not an "advertised" entry. If you wish to enter a patch, please go ahead, otherwise I'll just go ahead and patch this bug this weekend.

Good catch.

[fcambus]

Thanks for the update, you can go ahead and commit your patch.

[JoesCat]

I worked on this a bit, and you have to copy the dm[] resulting values calculated in run_spiro0() and give them to the spiro_to_bpath0() - it can be internalized, but it would make the access points run_spiro0() and spiro_to_bpath0() of no value as you would not be able to scale or shift results.

bugs patched.

[JoesCat]

Released Libspiro, version 20200505 (1.1.0) Closing issue as resolved.