Possible use after free in fvwm3 [Re: sysutils/rofi sometimes coredumps in __vfprintf (+ similar crash in fvwm3)]

Cc:

+ Thomas Adam, Michael for fvwm3 expertise
- jasper since this is no longer about rofi

> > As a side note - yesterday I got very suspicious crash in fvwm3
> > during simple fvwm restart, I can't reproduce it, but the bt also had
> > __vfprintf in it, fvwm3 dev's said that it was very strange segfault and
> > they have no idea what has happened, but with fvwm the snapshot wasn't
> > very new.
> >
> > Reading symbols from fvwm3...
> > Reading symbols from /usr/local/bin/.debug/fvwm3.dbg...
> > [New process 532945]
> > Core was generated by `fvwm3'.
> > Program terminated with signal SIGSEGV, Segmentation fault.
> > #0 strlen () at /usr/src/lib/libc/arch/amd64/string/strlen.S:125
> > 125 movq (%rax),%rdx /* get bytes to check */
> > (gdb) bt
> > #0 strlen () at /usr/src/lib/libc/arch/amd64/string/strlen.S:125
> > #1 0x00000141784fd6d8 in __vfprintf (fp=<optimized out>, fmt0=<optimized out>, ap=<optimized out>) at /usr/src/lib/libc/stdio/vfprintf.c:877
> > #2 0x00000141784fa996 in _libc_vfprintf (fp=0x14178548630 <usual>, fmt0=0x13f0429b0be " [KEY] %s\n", ap=0x7a1e8b73bb80) at /usr/src/lib/libc/stdio/vfprintf.c:263
> > #3 0x000001417852784c in _libc_fprintf (fp=0x14169019ff0, fmt=0x0) at /usr/src/lib/libc/stdio/fprintf.c:44
> > #4 0x0000013f04319b60 in SaveGlobalState (f=0x14178548630 <usual>) at session.c:183
>
> use egdb to go to this frame (f 4) and inspect what it's doing there.
> It could be another not-NUL terminated string or some other garbage
> pointer.

I think it's a garbage pointer due to a UAF free in SaveGlobalState():

https://github.com/fvwmorg/fvwm3/blob/main/fvwm/session.c#L183

prints the key from the MetaInfo list. That key was likely inserted by
insert_metainfo() in LoadGlobalState() earlier (as a key not already
present):

https://github.com/fvwmorg/fvwm3/blob/main/fvwm/session.c#L1231

and then freed right afterward.

insert_metainfo() has dubious ownership handling leaving the caller
clueless whether it should free the key that was passed or not (since
the caller has no way to know whether the key was already present
without querying beforehand):

If the key was already present in the MetaInfo list, the value is copied
and the caller needs to free the key. If not, the value is copied and
ownership of the key is taken:

https://github.com/fvwmorg/fvwm3/blob/main/fvwm/infostore.c#L81

I think this diff should fix this problem: always copy the key and make
sure it's freed in the other caller. But I don't use fvwm3 and don't
know how to reproduce, so this was only compile tested.

Index: Makefile
===================================================================
RCS file: /cvs/ports/x11/fvwm3/Makefile,v
retrieving revision 1.11
diff -u -p -r1.11 Makefile
--- Makefile 27 Sep 2023 20:37:05 -0000 1.11
+++ Makefile 30 Sep 2023 10:17:19 -0000
@@ -2,6 +2,7 @@ COMMENT= multiple virtual desktop window

VERSION= 1.0.8
DISTNAME= fvwm3-${VERSION}
+REVISION= 0

CATEGORIES= x11

Index: patches/patch-fvwm_infostore_c
===================================================================
RCS file: patches/patch-fvwm_infostore_c
diff -N patches/patch-fvwm_infostore_c
--- /dev/null 1 Jan 1970 00:00:00 -0000
+++ patches/patch-fvwm_infostore_c 30 Sep 2023 10:16:21 -0000
@@ -0,0 +1,24 @@
+Avoid use-after-free caused by LoadGlobalState() freeing the key
+that was just inserted. Fix this by always copying the key and
+avoiding a leak in the only other caller of insert_metainfo()
+
+Index: fvwm/infostore.c
+--- fvwm/infostore.c.orig
++++ fvwm/infostore.c
+@@ -78,7 +78,7 @@ void insert_metainfo(char *key, char *value)
+
+ /* It's a new item, add it to the list. */
+ mi_new = new_metainfo();
+- mi_new->key = key;
++ mi_new->key = fxstrdup(key);
+ CopyString(&mi_new->value, value);
+
+ mi_new->next = mi_store;
+@@ -192,6 +192,7 @@ void CMD_InfoStoreAdd(F_CMD_ARGS)
+ }
+
+ insert_metainfo(key, value);
++ free(key);
+ free(value);
+
+ return;