Re: less: fix use after free bug

[Tobias Stoeckmann]

On Fri, Dec 31, 2021 at 10:29:28PM -0800, Philip Guenther wrote:
> To bikeshed slightly I would be inclined to do the work progressively,
> perhaps like the diff below...but your diff works too.

I'm fine with your version as well.

In fact I have used a comparable approach but opted out to the one
I've sent because it modifies less lines of code.

So if nobody objects, I will use your version.


Tobias

Re: less: fix use after free bug

[Philip Guenther]

On Fri, Dec 31, 2021 at 6:22 AM Tobias Stoeckmann 
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 -  1.34
+++ line.c  1 Jan 2022 06:24:31 -
@@ -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);
 }

 /*

less: fix use after free bug

[Tobias Stoeckmann]

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?

Index: line.c
===
RCS file: /cvs/src/usr.bin/less/line.c,v
retrieving revision 1.34
diff -u -p -u -p -r1.34 line.c
--- line.c  25 Oct 2021 19:54:29 -  1.34
+++ line.c  31 Dec 2021 13:56:48 -
@@ -98,8 +98,10 @@ expand_linebuf(void)
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);
+   if (new_buf != NULL)
+   linebuf = new_buf;
+   if (new_attr != NULL)
+   attr = new_attr;
return (1);
}
linebuf = new_buf;