This is the same issue which has been fixed in dependency code.
If a line contains a variable which is longer than PKGCONF_ITEM_SIZE,
then the varname buffer overflows.
The code itself still does not check if a closing } exists and
truncates variable names which are too long. Since these would
be functional changes and this commit is about a protection against
undefined behaviour on a language level, these changes are not
included.
Proof of concept:
$ echo "Description: poc" > poc.pc
$ echo "Version: 1" >> poc.pc
$ echo -n 'Name: ${'
$ dd if=/dev/zero bs=1 count=66535 | tr '\0' 'x' >> poc.pc
$ echo >> poc.pc
$ pkgconf poc.pc
On my Linux system, when compiled with gcc, the varname buffer overflows
directly into buf, which means that no crash can be notified.
It's easiest to figure out when adding strlen() and sizeof() output
as debug lines.
fragment_quote adds quotation to fragments if needed. It allocates a
buffer and grows it as needed.
Unfortunately the dst pointer is not updated after a realloc, which
means that dst still points into the old memory area. Further writing
characters into that area leads to out of boundy writes.
Proof of concept:
$ cat > poc.pc << EOF
Name: poc
Description: poc
Version: 1
CFlags: -Ia
CFlags: -I%%%%%%%%%%%%%%%%%%%%b
CFlags: -I%%%%%%%%%%%%%%%%%%%%c
CFlags: -Id
EOF
$ pkgconf --cflags poc.pc
Most reliable attempt is to compile pkgconf with address sanitizer,
but this file should lead to an abort on a glibc system due to modified
chunk pointers (tested with Linux on amd64).
But since this is undefined behaviour, it depends on system details.
Parsing a fragment which consists only of a single dash leads to
an out of boundary read. It duplicates the following entry which
is not expected behaviour if another fragment follows.
Proof of concept:
$ cat > poc.pc << "EOF"
Name: poc
Description: poc
Version: 1
Cflags: - -I/somewhere
EOF
$ PKG_CONFIG_PATH=. pkgconf --cflags poc
-I/somewhere -I/somewhere
If - is the last entry, it leads to an out of boundary read, which is
easy to see if pkgconf is compiled with address sanitizer.
According to
https://docs.microsoft.com/fr-fr/windows/win32/fileio/naming-a-file
backslashes (with slashes) are a path separator, hence must no be
considered as an escape code.
The first fix, in argvsplit.c, disables this. But because of fragment_quote(),
the backslashes are doubled. Hence the second fix in fragment.c
With this pc file :
prefix=C:/Documents/msys2/opt/efl_64
libdir=${prefix}/lib
includedir=${prefix}/include
Name: eina
Description: efl: eina
Version: 1.24.99
Requires.private: iconv
Libs: -L${libdir} -leina -pthread -levil
Libs.private: -lpsapi -lole32 -lws2_32 -lsecur32 -luuid -lregex -lm
Cflags:-I${includedir}/eina-1 -I${includedir}/efl-1
-I${includedir}/eina-1/eina -pthread
pkgconf.exe --cflags eina
returns :
-IC:\Documents\msys2\opt\efl_64/include/eina-1
-IC:\Documents\msys2\opt\efl_64/include/efl-1
-IC:\Documents\msys2\opt\efl_64/include/eina-1/eina -pthread
-DWINICONV_CONST= -IC:\Documents\msys2\opt\ewpi_64/include
It is possible to trigger an out of boundary write in function
pkgconf_dependency_parse_str if a dependency line contains a very
long comparator. The comparator is stored in a temporary buffer which
has a size of PKGCONF_ITEM_SIZE.
The line which is parsed can be up to PKGCONF_BUFSIZE characters long,
which is larger than PKGCONF_ITEM_SIZE (although it depends on PATH_MAX).
Having a comparator which is longer than PKGCONF_ITEM_SIZE therefore
leads to an out of boundary write. Although it is undefined behaviour,
this can lead to an overridden compare variable, which in turn can lead
to an invalid instruction pointer, i.e. most likely a crash or code
execution (very unlikely).
Proof of concept:
$ echo "Requires: x " > poc.pc
$ dd if=/dev/zero bs=1 count=65535 | tr '\0' '<' >> poc.pc
$ pkgconf poc.pc
Eiter compile pkgconf with address sanitizer or run pkgconf multiple
times, eventually it might crash (assuming that ASLR is in place).
In order to fix this, I decided to use an end pointer to avoid OOB write.
Alternative would be to increase the buffer size, but I try to avoid that
since this would be additional ~60 KB stack space for a very unlikely
situation.
Windows allows both \ and / as valid path characters. A computed path
such as C:\development\libfoo\pkgconfig/foo.pc will result in a computed
pkgconf_pkg_t.id of "pkgconfig/foo".
Accordingly, correct the path normalization for checking for / after
the \ path has been dealt with in all cases.
It is possible to set the instruction pointer to undefined values by
using an operator larger than ':' in ASCII.
Since the personality function array does not have 256 entries, an
invalid operator can overflow the array.
Proof of concept:
$ echo "a _ b" > poc
$ ln -s $(which pkgconf) poc-pkgconf
$ ./poc-pkgconf
Every version line has a newline at the end; the malformed whitespace checker
should just check for trailing spaces and tabs.
Resolves https://todo.sr.ht/~kaniini/pkgconf/15
It is possible to trigger an out of boundary access with specially
crafted files. If a line consist of only a key and spaces, then
op will point to '\0'-ending of the buffer. Since p is iterated by
one byte right past this ending '\0', the next read access to p is
effectively out of bounds.
Theoretically this can also lead to out of boundary writes if spaces
are encountered.
Proof of concept (I recommend to compile with address sanitizer):
$ echo -n a > poc.pc
$ dd if=/dev/zero bs=1 count=65533 | tr '\0' ' ' >> poc.pc
$ pkgconf poc.pc
pkgconf_fgetline is called with a user-defined buffer, its size, and
a FILE stream to read input from.
If the buffer is almost completely filled and the file stream contains
an escaped character, then it is possible to trigger an off-by-one
buffer overflow with a '\0' character.
Easiest example to trigger this:
char buf[2];
pkgconf_fgetline(buf, sizeof(buf), stdin);
Enter "\\" (two backslashes) and press enter. If the library and the
program are compiled with address sanitizer, you will see the program
crashing. Otherwise it depends on your architecture what happens.
Since nobody should be using a buffer of only size 1 or 2, keep enough
space for a possibly escaped character in while loop by subtracting one
more byte for this situation, not just for '\0'.
This fixes a problem where on Windows the prefix would
not match if the prefix is generated with backslashes
and the rest of the variables use normal slashes
If a key is defined with no value, dequote will allocate a buffer with a
length of 0. Since the buffer's length is 0, any manipulation of its
content is UB.
Example .pc file:
prefix=/usr
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include
xcflags=
xlibs= -lSM -lICE -lX11
Name: Obt
Description: Openbox Toolkit Library
Version: 3.6
Requires: glib-2.0 libxml-2.0
Libs: -L${libdir} -lobt ${xlibs}
Cflags: -I${includedir}/openbox/3.6 ${xcflags}
Output using pkgconf 1.5.2 on x86_64 Linux/musl:
% pkgconf --cflags obt-3.5
-I/usr/include/openbox/3.6 \�\\�I\�\ -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libxml2
On Debian/Ubuntu I get:
CC libpkgconf/personality.lo
libpkgconf/personality.c: In function ‘load_personality_with_path’:
libpkgconf/personality.c:195:3: warning: implicit declaration of function ‘strlcpy’ [-Wimplicit-function-declaration]
strlcpy(pathbuf, path, sizeof pathbuf);
^~~~~~~
CC libpkgconf/parser.lo
CCLD libpkgconf.la
ar: `u' modifier ignored since `D' is the default (see `U')
CC cli/pkgconf-main.o
CC cli/pkgconf-getopt_long.o
CC cli/pkgconf-renderer-msvc.o
CCLD pkgconf
./.libs/libpkgconf.so: undefined reference to `strlcpy'
Ariadne Conill
Tobias Stoeckmann