Re: Use after free in X config parser

2023-10-14 Thread Crystal Kolipe
On Sat, Oct 14, 2023 at 07:14:29PM +0200, Matthieu Herrb wrote:
> On Sun, Oct 08, 2023 at 02:50:10PM -0300, Crystal Kolipe wrote:
> > Hi,
> > 
> > On Sun, Oct 08, 2023 at 07:07:27PM +0200, Matthieu Herrb wrote:
> > > I can confirm that there's an issue here. However in my tests, I don't
> > > see a garbled error message.
> > > If I set MALLOC_OPTIONS=F then a double free is reported, which I
> > > tracked down to the realloc() calls in getNextLine() that will make
> > > the xf68_lex_val.str pointer point to already free()d memory.
> > > 
> > > So the following patch, which invalidates this pointer before reading
> > > more data from the config file fixes the issue I'm seeing. Can you
> > > confirm that it also fixes it for you ?
> > 
> > Unfortunately it doesn't :-).
> > 
> > Or rather, yes and no...  Your patch does fix most cases, but I'm
> > still able to read free'd memory with a specially crafted config
> > file that contains bare 0x0d bytes as line separators instead of 0x0a.
> >
> 
> Ok, after some more attempts, I have to agree that copying the token
> like you proposed originally is the only way to completely fix the
> issue.
> 
> Here's a patch that also free()s the copy after it's been consumed, to
> avoid the (minor) leaks.
> 
> Also submitted upstreams:
> https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1176
> 
> Comments, ok?

Works fine for me, and I am unable to reproduce the original problem anymore
with this version of the patch so OK here.

There seems to be a stray comment left in, though, not sure if that was
intentional or not:

> RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/scan.c,v
> retrieving revision 1.12
> diff -u -p -u -r1.12 scan.c
> --- hw/xfree86/parser/scan.c  27 Jul 2019 07:57:18 -  1.12
> +++ hw/xfree86/parser/scan.c  9 Oct 2023 05:56:22 -
> @@ -278,9 +278,10 @@ xf86getToken(const xf86ConfigSymTabRec *
>  if (!c) {
>  char *ret;
>  
> -if (numFiles > 0)
> +if (numFiles > 0) {
> + // xf86_lex_val.str = NULL;
>  ret = xf86getNextLine();
> -else {
> +} else {
>  if (builtinConfig[builtinIndex] == NULL)
>  ret = NULL;
>  else {



Re: Use after free in X config parser

2023-10-14 Thread Matthieu Herrb
On Sun, Oct 08, 2023 at 02:50:10PM -0300, Crystal Kolipe wrote:
> Hi,
> 
> On Sun, Oct 08, 2023 at 07:07:27PM +0200, Matthieu Herrb wrote:
> > I can confirm that there's an issue here. However in my tests, I don't
> > see a garbled error message.
> > If I set MALLOC_OPTIONS=F then a double free is reported, which I
> > tracked down to the realloc() calls in getNextLine() that will make
> > the xf68_lex_val.str pointer point to already free()d memory.
> > 
> > So the following patch, which invalidates this pointer before reading
> > more data from the config file fixes the issue I'm seeing. Can you
> > confirm that it also fixes it for you ?
> 
> Unfortunately it doesn't :-).
> 
> Or rather, yes and no...  Your patch does fix most cases, but I'm
> still able to read free'd memory with a specially crafted config
> file that contains bare 0x0d bytes as line separators instead of 0x0a.
>

Ok, after some more attempts, I have to agree that copying the token
like you proposed originally is the only way to completely fix the
issue.

Here's a patch that also free()s the copy after it's been consumed, to
avoid the (minor) leaks.

Also submitted upstreams:
https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1176

Comments, ok?

Index: hw/xfree86/parser/DRI.c
===
RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/DRI.c,v
retrieving revision 1.8
diff -u -p -u -r1.8 DRI.c
--- hw/xfree86/parser/DRI.c 8 Dec 2017 15:02:01 -   1.8
+++ hw/xfree86/parser/DRI.c 9 Oct 2023 05:56:22 -
@@ -77,6 +77,8 @@ xf86parseDRISection(void)
 break;
 case COMMENT:
 ptr->dri_comment = xf86addComment(ptr->dri_comment, 
xf86_lex_val.str);
+free(xf86_lex_val.str);
+xf86_lex_val.str = NULL;
 break;
 default:
 Error(INVALID_KEYWORD_MSG, xf86tokenString());
Index: hw/xfree86/parser/Device.c
===
RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Device.c,v
retrieving revision 1.10
diff -u -p -u -r1.10 Device.c
--- hw/xfree86/parser/Device.c  27 Jul 2019 07:57:18 -  1.10
+++ hw/xfree86/parser/Device.c  9 Oct 2023 05:56:22 -
@@ -106,6 +106,8 @@ xf86parseDeviceSection(void)
 switch (token) {
 case COMMENT:
 ptr->dev_comment = xf86addComment(ptr->dev_comment, 
xf86_lex_val.str);
+free(xf86_lex_val.str);
+xf86_lex_val.str = NULL;
 break;
 case IDENTIFIER:
 if (xf86getSubToken(&(ptr->dev_comment)) != STRING)
Index: hw/xfree86/parser/Extensions.c
===
RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Extensions.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 Extensions.c
--- hw/xfree86/parser/Extensions.c  8 Dec 2017 15:02:01 -   1.5
+++ hw/xfree86/parser/Extensions.c  9 Oct 2023 05:56:22 -
@@ -67,6 +67,8 @@ xf86parseExtensionsSection(void)
 case COMMENT:
 ptr->extensions_comment =
 xf86addComment(ptr->extensions_comment, xf86_lex_val.str);
+free(xf86_lex_val.str);
+xf86_lex_val.str = NULL;
 break;
 default:
 Error(INVALID_KEYWORD_MSG, xf86tokenString());
Index: hw/xfree86/parser/Files.c
===
RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Files.c,v
retrieving revision 1.9
diff -u -p -u -r1.9 Files.c
--- hw/xfree86/parser/Files.c   8 Dec 2017 15:02:01 -   1.9
+++ hw/xfree86/parser/Files.c   9 Oct 2023 05:56:22 -
@@ -89,6 +89,8 @@ xf86parseFilesSection(void)
 switch (token) {
 case COMMENT:
 ptr->file_comment = xf86addComment(ptr->file_comment, 
xf86_lex_val.str);
+free(xf86_lex_val.str);
+xf86_lex_val.str = NULL;
 break;
 case FONTPATH:
 if (xf86getSubToken(&(ptr->file_comment)) != STRING)
Index: hw/xfree86/parser/Flags.c
===
RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/Flags.c,v
retrieving revision 1.11
diff -u -p -u -r1.11 Flags.c
--- hw/xfree86/parser/Flags.c   11 Nov 2021 09:03:09 -  1.11
+++ hw/xfree86/parser/Flags.c   9 Oct 2023 05:56:22 -
@@ -98,6 +98,8 @@ xf86parseFlagsSection(void)
 switch (token) {
 case COMMENT:
 ptr->flg_comment = xf86addComment(ptr->flg_comment, 
xf86_lex_val.str);
+free(xf86_lex_val.str);
+xf86_lex_val.str = NULL;
 break;
 /*
  * these old keywords are turned into standard generic options.
@@ -436,17 +438,21 @@ xf86parseOption(XF86OptionPtr head)
 if ((token = xf86getSubToken()) == STRING) {
 option = xf86newOption(name, xf86_lex_val.str);
   

Re: Use after free in X config parser

2023-10-08 Thread Crystal Kolipe
Hi,

On Sun, Oct 08, 2023 at 07:07:27PM +0200, Matthieu Herrb wrote:
> I can confirm that there's an issue here. However in my tests, I don't
> see a garbled error message.
> If I set MALLOC_OPTIONS=F then a double free is reported, which I
> tracked down to the realloc() calls in getNextLine() that will make
> the xf68_lex_val.str pointer point to already free()d memory.
> 
> So the following patch, which invalidates this pointer before reading
> more data from the config file fixes the issue I'm seeing. Can you
> confirm that it also fixes it for you ?

Unfortunately it doesn't :-).

Or rather, yes and no...  Your patch does fix most cases, but I'm
still able to read free'd memory with a specially crafted config
file that contains bare 0x0d bytes as line separators instead of 0x0a.

For example, consider the following config file:

$ hexdump -C custom
  23 66 6f 6f 0d 62 61 72  0a   |#foo.bar.|
0009

Running with your patch, (or unpatched), I see:

$ X -config custom

[...]
Parse error on line 1 of section InputClass in file custom
"on" is not a valid keyword in this section.
[...]

With my patch this 0x0d case was mitigated too.

If I set vm.malloc_conf=CFGJ, the behaviour becomes non-deterministic,
sometimes it segfaults, other times it just dumps what looks like 1024
0xdf bytes as the error string.



Re: Use after free in X config parser

2023-10-08 Thread Matthieu Herrb
On Thu, Oct 05, 2023 at 08:11:39PM -0300, Crystal Kolipe wrote:
> This is an interesting one...
> 
> There is a use after free bug in the X config parser, which can be trivially
> demonstrated by creating a config file with the first line being a comment and
> the second line containing invalid syntax:
> 
> $ echo "#foo\nfoo" > custom_config
> $ X -config custom_config
> 
> [...]
> 
> (++) Using config file: "custom_config"
> (==) Using system config directory "/usr/X11R6/share/X11/xorg.conf.d"
> Parse error on line 3 of section InputClass in file custom_config
>   "on" is not a valid keyword in this section.
> (EE) 
> Fatal server error:
> (EE) no screens found(EE) 
> 
> [...]
> 
> So part of the error message is reading 0xdf bytes, which are obviously from a
> buffer which has been free'd.


Hi,

I can confirm that there's an issue here. However in my tests, I don't
see a garbled error message.
If I set MALLOC_OPTIONS=F then a double free is reported, which I
tracked down to the realloc() calls in getNextLine() that will make
the xf68_lex_val.str pointer point to already free()d memory.

So the following patch, which invalidates this pointer before reading
more data from the config file fixes the issue I'm seeing. Can you
confirm that it also fixes it for you ?

Index: hw/xfree86/parser/scan.c
===
RCS file: /cvs/OpenBSD/xenocara/xserver/hw/xfree86/parser/scan.c,v
retrieving revision 1.12
diff -u -p -u -r1.12 scan.c
--- hw/xfree86/parser/scan.c27 Jul 2019 07:57:18 -  1.12
+++ hw/xfree86/parser/scan.c8 Oct 2023 17:03:21 -
@@ -278,9 +278,11 @@ xf86getToken(const xf86ConfigSymTabRec *
 if (!c) {
 char *ret;
 
-if (numFiles > 0)
+if (numFiles > 0) {
+/* invalidate lex token */
+xf86_lex_val.str = NULL;
 ret = xf86getNextLine();
-else {
+} else {
 if (builtinConfig[builtinIndex] == NULL)
 ret = NULL;
 else {

-- 
Matthieu Herrb