libpkgconf: tuple: fix out of bounds again v1 PROPOSED

Vegard Nossum: 1
 libpkgconf: tuple: fix out of bounds again

 1 files changed, 7 insertions(+), 14 deletions(-)

[PATCH] libpkgconf: tuple: fix out of bounds again Export this patch

If there is no '}' in a ${variable} placeholder, the "varname" buffer
will not be NUL-terminated, resulting in various valgrind splats:

  ==2749814== Conditional jump or move depends on uninitialised value(s)
  ==2749814==    at 0x484ED28: strlen (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==2749814==    by 0x48FED30: __vfprintf_internal (vfprintf-internal.c:1517)
  ==2749814==    by 0x4910499: __vsnprintf_internal (vsnprintf.c:114)
  ==2749814==    by 0x48609AC: vsnprintf (stdio2.h:85)
  ==2749814==    by 0x48609AC: pkgconf_trace (client.c:384)
  ==2749814==    by 0x4864D6A: pkgconf_tuple_parse (tuple.c:358)
  ==2749814==    by 0x48643B3: pkgconf_fragment_parse (fragment.c:680)

We can simplify this code by detecting when it is time to terminate the
loop and then always NUL-terminating the result.

It is always safe to do this final NUL write because "vend" always points
to the last byte of the buffer and we know that "vptr" will never go beyond
it.

Test case:

  echo 'Cflags: -I${includedir' > test.pc
  LD_LIBRARY_PATH=.libs PKG_CONFIG_PATH=. valgrind ./.libs/pkgconf --cflags test

Fixes: 5eb9cae0091e (libpkgconf: tuple: fix out of boundary write)

Tobias Stoeckmann <tobias@stoeckmann.org>

5 months ago

Reviewing that commit and this diff, I'm still not sure why it "fixes"
it, since the issue has been there before as well.

Vegard Nossum <vegard.nossum@oracle.com>

5 months ago

Ah, sorry, you're right; looks like the issue has been there since
commit ba1c41eb. Feel free to drop the Fixes: line.

We could also change the patch title to something like

   libpkgconf: tuple: fix NUL termination on missing '}'

or whatever the maintainer would prefer :-)

Next

Anyway, since I spent some time comparing the different versions,
this patch looks correct and fixes an issue which my previous attempt
missed.

Vegard Nossum <vegard.nossum@oracle.com>

5 months ago

Thanks for looking over it!


Vegard

Tobias

Cc: Tobias Stoeckmann <tobias@stoeckmann.org>
---
 libpkgconf/tuple.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/libpkgconf/tuple.c b/libpkgconf/tuple.c
index 83f6a47..fa08c5c 100644
--- a/libpkgconf/tuple.c
+++ b/libpkgconf/tuple.c
@@ -338,23 +338,16 @@ pkgconf_tuple_parse(const pkgconf_client_t *client, pkgconf_list_t *vars, const
 
 			for (pptr = ptr + 2; *pptr != '\0'; pptr++)
 			{
-				if (*pptr != '}')
-				{
-					if (vptr < vend)
-						*vptr++ = *pptr;
-					else
-					{
-						*vptr = '\0';
-						break;
-					}
-				}
-				else
-				{
-					*vptr = '\0';
+				if (*pptr == '}')
 					break;
-				}
+				if (vptr == vend)
+					break;

Tobias Stoeckmann <tobias@stoeckmann.org>

5 months ago

This implies that looking for a closing bracket stops when the
internal buffer capacity is reached.

If something like "${.....${test}" is written, it can happen
that "test" is actually resolved, while with the current code,
the whole expression between first { and first } is considered.

This means that very long illegal expressions would be interpreted
differently.

I think it's best to still iterate until the closing bracket or
end of string is reached.


Tobias

+
+				*vptr++ = *pptr;
 			}
 
+			*vptr = '\0';
+
 			PKGCONF_TRACE(client, "lookup tuple %s", varname);
 
 			size_t remain = PKGCONF_BUFSIZE - (bptr - buf);
-- 
2.34.1

Tobias Stoeckmann <tobias@stoeckmann.org>

5 months ago

Hi,

Tobias Stoeckmann <tobias@stoeckmann.org>

5 months ago

On a second thought...

While it is debatable if ${ without a closing bracket should be
interpreted at all, this change adds a subtle issue: