kilo
kilo copied to clipboard
antirez
Heap buffer overflow in kilo's editor_update_syntax(…)
Hi,
Kilo appears to have a heap buffer overflow triggered by the memcmp call on line 475 of kilo.c:
int j;
for (j = 0; keywords[j]; j++) {
int klen = strlen(keywords[j]);
int kw2 = keywords[j][klen-1] == '|';
if (kw2) klen--;
if (!memcmp(p,keywords[j],klen) &&
The signature of memcmp:
int memcmp(const void *s1, const void *s2, size_t n);
memcmp operates under the assumption that s1 and s2 are at least n bytes long each.
This assumption clearly holds for the second argument (keywords[j]), but there are no checks in place that makes sure that this assumption holds also for the first argument (p).
The heap overflow can be verified by compiling kilo with ASAN enabled:
$ git clone https://github.com/antirez/kilo.git
$ cd kilo
$ clang -fsanitize=address -o kilo kilo.c
$ ./kilo kilo.c
ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60300000dcd5 at pc 0x0001029f16f9 bp 0x7fff5d233440 sp 0x7fff5d232c00
READ of size 6 at 0x60300000dcd5 thread T0
#0 0x1029f16f8 in wrap_memcmp (libclang_rt.asan_osx_dynamic.dylib+0x116f8)
#1 0x1029d08db in editorUpdateSyntax (kilo+0x1000048db)
#2 0x1029d18fb in editorUpdateRow (kilo+0x1000058fb)
#3 0x1029d1d86 in editorInsertRow (kilo+0x100005d86)
#4 0x1029d3c85 in editorOpen (kilo+0x100007c85)
#5 0x1029d6ae7 in main (kilo+0x10000aae7)
#6 0x7fff8956c5ac in start (libdyld.dylib+0x35ac)
#7 0x1 (<unknown module>)
0x60300000dcd5 is located 0 bytes to the right of 21-byte region [0x60300000dcc0,0x60300000dcd5)
allocated by thread T0 here:
#0 0x102a289c0 in wrap_malloc (libclang_rt.asan_osx_dynamic.dylib+0x489c0)
#1 0x1029d1303 in editorUpdateRow (kilo+0x100005303)
#2 0x1029d1d86 in editorInsertRow (kilo+0x100005d86)
#3 0x1029d3c85 in editorOpen (kilo+0x100007c85)
#4 0x1029d6ae7 in main (kilo+0x10000aae7)
#5 0x7fff8956c5ac in start (libdyld.dylib+0x35ac)
#6 0x1 (<unknown module>)
This issue has been fixed in my kilo fork called openemacs: https://github.com/practicalswift/openemacs :-)
Well then you could try sending a pull request upstream we all will get it aswell in the process.
@pankajdoharey I think @antirez' intention is to let any further development be done by others in their respective forks, so submitted pull requests have not being merged into this repo historically. So I'm afraid a PR is unlikely to be merged.
Luckily the fix is trivial, just check so that strlen(p) >= klen prior to doing the memcmp :-)
@practicalswift WoW. Well in that case may be he should handover the maintenance to someone able in the community.
@antirez What's the sitrep on this repo? Are you going to be maintaining it?
