found a integer overflow leads to stack_overflow

[lometsj]

in parser.c k seems like the size of the rest of buffer on stack(var 'buf' which type is char[]). n is memcpy size as 3rd para. while condition k>0 to keep buffer is not full. if enter in if(k<n) branch n=k,var n is assigned the value of k then k -=n ,var k is assigned the value 0 then --k, var k is assigned the value 0xffffffffffffffff while var k's type is size_t. so the while conditionk > 0 not make sense. next memcpy will lead to stack overflow

void error_ref_sym(fb_parser_t *P, fb_ref_t *ref, const char *msg, fb_symbol_t *s2)
{
    fb_ref_t *p;
    char buf[FLATCC_MAX_IDENT_SHOW + 1];
    size_t k = FLATCC_MAX_IDENT_SHOW;
    size_t n = 0;
    size_t n0 = 0;
    p = ref;
    while (p && k > 0) {
        n = (size_t)p->ident->len;
        if (k < n) {
            n = k;
        }
        memcpy(buf + n0, p->ident->text, n);
        k -= n;
        n0 += n;
        buf[n0] = '.';
        --k; //overflow here when k = 0
        ++n0;
        p = p->link;
    }
    buf[n0] = '\0';
    if (n0 > 0) {
        --n0;
    }
    if (k <= 0) {
        memcpy(buf + FLATCC_MAX_IDENT_SHOW + 1 - 4, "...\0", 4);
        n0 = FLATCC_MAX_IDENT_SHOW;
    }
    error_report(P, ref->ident, msg, s2 ? s2->ident : 0, buf, n0);
}

poc:

➜  fuzz_test cat test9
struct tsj{
        damage:shortllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllllve.tlllllllllllllllllllllllllllllllllllllll;
}
➜  fuzz_test ../flatcc/bin/flatcc test9
test9:2:9: error: 'shortlllllllllllllllllllllllllllllllllllllllllllll.tlllllllllllllllllllllllllllllllllllllll': unknown type reference used with struct field: test9:2:2: 'damage'
*** stack smashing detected ***: terminated
[1]    3685 abort (core dumped)  ../flatcc/bin/flatcc test9
➜  fuzz_test

➜  fuzz_test gdb ../flatcc/bin/flatcc
GNU gdb (Ubuntu 9.2-0ubuntu1~20.04) 9.2
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
pwndbg: loaded 196 commands. Type pwndbg [filter] for a list.
pwndbg: created $rebase, $ida gdb functions (can be used with print/break)
Reading symbols from ../flatcc/bin/flatcc...
pwndbg> set args test9
pwndbg> r
Starting program: /mnt/c/Users/tsj/Desktop/flatcc/bin/flatcc test9
test9:2:9: error: 'shortlllllllllllllllllllllllllllllllllllllllllllll.tlllllllllllllllllllllllllllllllllllllll': unknown type reference used with struct field: test9:2:2: 'damage'
*** stack smashing detected ***: terminated

Program received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
50      ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
LEGEND: STACK | HEAP | CODE | DATA | RWX | RODATA
─────────────────────────────────────────────────────────────────────────────────[ REGISTERS ]──────────────────────────────────────────────────────────────────────────────────
 RAX  0x0
 RBX  0x7fffff7e13c0 ◂— 0x7fffff7e13c0
 RCX  0x8
 RDX  0x0
 RDI  0x2
 RSI  0x7ffffffed990 ◂— 0x0
 R8   0x0
 R9   0x7ffffffed990 ◂— 0x0
 R10  0x8
 R11  0x8
 R12  0x7ffffffedc10 ◂— 0x642ccf000a276567 /* "ge'\n" */
 R13  0x20
 R14  0x7fffff510000 ◂— 0x202a2a2a00001000
 R15  0x1
 RBP  0x7ffffffedd10 —▸ 0x7fffff76a07c ◂— '*** %s ***: terminated\n'
 RSP  0x7ffffffed990 ◂— 0x0
 RIP  0x7fffff5f618b (raise+203) ◂— mov    rax, qword ptr [rsp + 0x108]
───────────────────────────────────────────────────────────────────────────────────[ DISASM ]───────────────────────────────────────────────────────────────────────────────────
 ► 0x7fffff5f618b <raise+203>    mov    rax, qword ptr [rsp + 0x108]
   0x7fffff5f6193 <raise+211>    xor    rax, qword ptr fs:[0x28]
   0x7fffff5f619c <raise+220>    jne    raise+260 <raise+260>
    ↓
   0x7fffff5f61c4 <raise+260>    call   __stack_chk_fail <__stack_chk_fail>

   0x7fffff5f61c9                nop    dword ptr [rax]
   0x7fffff5f61d0 <killpg>       endbr64
   0x7fffff5f61d4 <killpg+4>     test   edi, edi
   0x7fffff5f61d6 <killpg+6>     js     killpg+16 <killpg+16>

   0x7fffff5f61d8 <killpg+8>     neg    edi
   0x7fffff5f61da <killpg+10>    jmp    kill <kill>

   0x7fffff5f61df <killpg+15>    nop
───────────────────────────────────────────────────────────────────────────────────[ STACK ]────────────────────────────────────────────────────────────────────────────────────
00:0000│ rsi r9 rsp 0x7ffffffed990 ◂— 0x0
01:0008│            0x7ffffffed998 —▸ 0x7ffffffedb48 ◂— 0x3000000030 /* '0' */
02:0010│            0x7ffffffed9a0 —▸ 0x7ffffffedb60 ◂— "test9:2:9: error: 'shortlllllllllllllllllllllllllllllllllllllllllllll.tlllllllllllllllllllllllllllllllll"
03:0018│            0x7ffffffed9a8 —▸ 0x7fffff63f11a (__vsnprintf_internal+170) ◂— mov    r9, qword ptr [rsp + 8]
04:0020│            0x7ffffffed9b0 ◂— 0x3
05:0028│            0x7ffffffed9b8 —▸ 0x7ffffffedab0 ◂— 0x20 /* ' ' */
06:0030│            0x7ffffffed9c0 ◂— 0xfbad8001
07:0038│            0x7ffffffed9c8 —▸ 0x7ffffffedb60 ◂— "test9:2:9: error: 'shortlllllllllllllllllllllllllllllllllllllllllllll.tlllllllllllllllllllllllllllllllll"
─────────────────────────────────────────────────────────────────────────────────[ BACKTRACE ]──────────────────────────────────────────────────────────────────────────────────
 ► f 0   0x7fffff5f618b raise+203
   f 1   0x7fffff5d5859 abort+299
   f 2   0x7fffff6403ee __libc_message+670
   f 3   0x7fffff6e2b4a __fortify_fail+42
   f 4   0x7fffff6e2b16
   f 5        0x8073af9
   f 6        0x80976b9 __flatcc_fb_build_schema+21961
   f 7        0x80976b9 __flatcc_fb_build_schema+21961
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
pwndbg>

[mikkelfj]

Indeed. I wonder if --k has any purpose at all, even without hitting overflow.

[mikkelfj]

I think there are more problems. The assignment of '.' might be outside the buffer. --k accounts for the .. EDIT: no, there is one extra space reserved for that.

[lometsj]

yes, i agree --k accounts for the .

[mikkelfj]

Please review last commit on master: 8a90c92

[mikkelfj]

BTW: it is rather strange that the compiler did not warn about this.

[lometsj]

~~it seems not solve memcpy's copy overflow actually the stack overflow happens on the next loop after truncated in my opinion just mod while (p && k>0) to while(p && k>0 && k<0x8000000000000000)~~ EDIT: sorry for mistake. it seems ok for solving it.

[lometsj]

compiler did not warn because it don't know the value of n0, n on the runtime memcpy(buf + n0, p->ident->text, n);

[mikkelfj]

Sorry, but your suggestion is not good because it depends on the integer size.

The following was written before your last EDIT, FYI. So I will close here.

Why do you think an overflow still happens? Does your test still fail?

The invariant is that k is what is left in the buffer. In the new version, k > 0 on entry. If '.' is added, k could become zero. However, n will not be larger than k when entering memcpy, so worst case, 0 bytes are copied, which will not result in a buffer overrun. At least as I understand it.

The compiler could have warned that the test <= 0 does not make sense. There are many other places where the compiler warns even if the code actually makes sense.

It's getting late, so I'll continue tomorrow if you have some suggestions.

[mikkelfj]

Thanks for reporting.