On Fri, Dec 31, 2021 at 6:22 AM Tobias Stoeckmann <tob...@stoeckmann.org>
wrote:
> Hi,
>
> it is possible to trigger a use after free bug in less with huge
> files or tight memory constraints. PoC with 100 MB file:
>
> dd if=/dev/zero bs=1024 count=102400 | tr '\0' 'a' > less-poc.txt
> ulimit -d 157286
> less less-poc.txt
>
> The linebuf and attr buffers in line.c are supposed to never be freed,
> since they are used for keeping (meta) data of current line in RAM.
>
> The linebuf and attr buffers are expanded in expand_linebuf. If linebuf
> could be expanded but attr could not, then returned pointers are freed,
> but linebuf variable still points to previous location. Subsequent
> accesses will therefore trigger a use after free bug.
>
> The easiest fix is to keep reallocated linebuf. The size of both buffers
> differ, but the code still assumes that the previous (smaller) size is
> the correct one.
>
> Thoughts on this approach?
>
Analysis makes sense.
To bikeshed slightly I would be inclined to do the work progressively,
perhaps like the diff below...but your diff works too.
Philip Guenther
Index: line.c
===================================================================
RCS file: /data/src/openbsd/src/usr.bin/less/line.c,v
retrieving revision 1.34
diff -u -p -r1.34 line.c
--- line.c 25 Oct 2021 19:54:29 -0000 1.34
+++ line.c 1 Jan 2022 06:24:31 -0000
@@ -96,16 +96,16 @@ expand_linebuf(void)
/* Just realloc to expand the buffer, if we can. */
char *new_buf = recallocarray(linebuf, size_linebuf, new_size, 1);
- char *new_attr = recallocarray(attr, size_linebuf, new_size, 1);
- if (new_buf == NULL || new_attr == NULL) {
- free(new_attr);
- free(new_buf);
- return (1);
+ if (new_buf != NULL) {
+ char *new_attr = recallocarray(attr, size_linebuf,
new_size, 1);
+ linebuf = new_buf;
+ if (new_attr != NULL) {
+ attr = new_attr;
+ size_linebuf = new_size;
+ return (0);
+ }
}
- linebuf = new_buf;
- attr = new_attr;
- size_linebuf = new_size;
- return (0);
+ return (1);
}
/*
