Renaud Allard <[email protected]> wrote: > The chars[] lookup table in deroff(1) is declared as char chars[128] > but is indexed by byte values from input which range from 0 to 255. > Any input byte >= 128 causes an out-of-bounds read. > > The table is accessed at 13 call sites, including: > > 606: if (chars[c] == PUNCT) > 751: if (c2 != -1 && chars[c2] == PUNCT) > 860: if (... chars[(unsigned char)s[0]] == LETTER && > 897: while (chars[(unsigned char)*p1] < DIGIT) > 901: for (p = p1 ; (i = chars[(unsigned char)*p]) != SPECIAL ; ++p) > > All indices can be 0-255 (from getc() or unsigned char casts). > Values >= 128 read past the end of the 128-byte array. > > The out-of-bounds read at line 897 and 901 is particularly > dangerous: if the garbage value satisfies the loop condition, > the loop walks past the string's null terminator, eventually > reading unmapped memory. > > AFL++ fuzzing found 64 crashes (37 SIGSEGV, 27 UBSan/SIGILL). > > Fix: expand chars[] from 128 to 256 entries. Entries 128-255 > are zero-initialized (SPECIAL), which is correct: non-ASCII > bytes should not be treated as letters, digits, or punctuation.
makes sense. this is a bit gross, `c' is a global var fed from getchar(), sometimes it's casted to unsigned char and sometimes not, etc. at least we're sure it's not EOF =) so, cranking the lookup table seems a sane choice. okay op@ > Index: usr.bin/deroff/deroff.c > =================================================================== > RCS file: /cvs/src/usr.bin/deroff/deroff.c,v > retrieving revision 1.18 > diff -u -p -r1.18 deroff.c > --- usr.bin/deroff/deroff.c 27 Sep 2023 21:06:33 -0000 1.18 > +++ usr.bin/deroff/deroff.c > @@ -133,7 +133,7 @@ int intable; > int keepblock; /* keep blocks of text; normally false when msflag */ > > -char chars[128]; /* SPECIAL, PUNCT, APOS, DIGIT, or LETTER */ > +char chars[256]; /* SPECIAL, PUNCT, APOS, DIGIT, or LETTER */ > > size_t linesz; > char *line;
