[musl] [PATCH] fix wide printf numbered argument buffer overflow

[musl] [PATCH] fix wide printf numbered argument buffer overflow

[Gabriel Ravier]

The nl_type and nl_arg arrays defined in vfwprintf may be accessed
with an index up to and including NL_ARGMAX, but they are only of size
NL_ARGMAX, meaning they may be written to or read from 1 element too
far.

For example, the following program:

 #include <assert.h>
 #include <stdio.h>
 #include <string.h>
 #include <wchar.h>

int main(void) {
   char buffer[500];
   for (size_t i = 0; i < sizeof(buffer); ++i)
      buffer[i] = (i % 3) ? 0 : 42;

   wchar_t result;

   int ret = swprintf(&result, 1, L"%1$s", "");
   assert(ret != -1);
}

evidences the bug, by sometimes mistakenly failing the assertion
and always triggering a warning under valgrind (the behavior is highly
dependent on build configuration - I could only reproduce the assert
failure with GCC 12.2.1 on a very specific system - but the bug is
still there, as evidenced by the warning valgrind always emits)

This patch fixes this.
---
 src/stdio/vfwprintf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/stdio/vfwprintf.c b/src/stdio/vfwprintf.c
index 18784113..53697701 100644
--- a/src/stdio/vfwprintf.c
+++ b/src/stdio/vfwprintf.c
@@ -347,8 +347,8 @@ overflow:
 int vfwprintf(FILE *restrict f, const wchar_t *restrict fmt, va_list ap)
 {
 	va_list ap2;
-	int nl_type[NL_ARGMAX] = {0};
-	union arg nl_arg[NL_ARGMAX];
+	int nl_type[NL_ARGMAX+1] = {0};
+	union arg nl_arg[NL_ARGMAX+1];
 	int olderr;
 	int ret;
 
-- 
2.39.2

Re: [musl] [PATCH] fix wide printf numbered argument buffer overflow

[Rich Felker]

On Fri, Apr 14, 2023 at 04:55:42PM +0200, Gabriel Ravier wrote:
> The nl_type and nl_arg arrays defined in vfwprintf may be accessed
> with an index up to and including NL_ARGMAX, but they are only of size
> NL_ARGMAX, meaning they may be written to or read from 1 element too
> far.
> 
> For example, the following program:
> 
>  #include <assert.h>
>  #include <stdio.h>
>  #include <string.h>
>  #include <wchar.h>
> 
> int main(void) {
>    char buffer[500];
>    for (size_t i = 0; i < sizeof(buffer); ++i)
>       buffer[i] = (i % 3) ? 0 : 42;
> 
>    wchar_t result;
> 
>    int ret = swprintf(&result, 1, L"%1$s", "");
>    assert(ret != -1);
> }
> 
> evidences the bug, by sometimes mistakenly failing the assertion
> and always triggering a warning under valgrind (the behavior is highly
> dependent on build configuration - I could only reproduce the assert
> failure with GCC 12.2.1 on a very specific system - but the bug is
> still there, as evidenced by the warning valgrind always emits)

Thanks!

To clarify for readers, the reason it fails is that the wide printf
core checks that the argument type list consists of a contiguous run
of used positions followed by unused positions all the way up to the
end of the buffer. This involves checking the 9 position, which was
out-of-bounds due to an off-by-one error in the declaration. If the
overlapping memory happened to have a nonzero value, it would appear
as a present %9$ slot with one or more prior slots unused, thereby
triggering the error condition.

While all instances of nonconforming behavior and UB are bad, I don't
think this is of particular security relevance, despite being
out-of-bounds array read/write. Write is only reachable if the
application is using format strings with a 9th numbered argument
position, and affected programs seem like they should be
malfunctioning already regardless of any attacker-controlled input.
Still I'm glad to have this fix in time for a release.

Rich