glibc 2.37: snprintf() on armhf wrongly truncates writes given extremely large size argument

[Steve Langasek]

The cyrus-imapd package fails to build from source on armhf in lunar against glibc 2.37. I've tracked this down to a combination of bad string handling in the cyrus library's API, and a regression in glibc 2.37 vs 2.36 when snprintf() is passed a size argument whose value is very close to INT_MAX.

Basically, since the API is passed a buffer of unknown size, and then passes this on to functions that DO safe handling of buffer lengths, it claims a buffer size of INT_MAX. Because the functions start filling the buffer before the call to snprintf(), the actual size argument to snprintf() is slightly less than INT_MAX. This is unrealistic and incorrect, but technically valid, so snprintf() should handle it correctly.

Below is a reproducer that demonstrates the bug on armhf.

#include <limits.h>
#include <stdio.h>
#include <string.h>

int main() {

    char buf[32];
    int res;

    res = snprintf(buf, sizeof(buf)-1, "%s", "hello world");

    printf("having a normal one. res=%d,buf=%s\n",res,buf);

    res = snprintf(buf, INT_MAX, "%s", "hello world");

    printf("res=%d but buf=%s\n",res,buf);
}

[Steve Langasek (vorlon)]

Marking this as critical because a pretty fundamental string handling function is misbehaving, and even though the only known failure at the moment is from a consumer doing something patently ridiculous, the overall impact is not known.

[Steve Langasek (vorlon)]

Output on lunar/armhf with libc6 2.36-0ubuntu4 installed:

$ gcc -o /tmp/augh /tmp/augh.c && /tmp/augh
having a normal one. res=11,buf=hello world
res=11 but buf=hello world
$

And with libc6 2.37-0ubuntu1 installed:

$ gcc -o /tmp/augh /tmp/augh.c && /tmp/augh
having a normal one. res=11,buf=hello world
res=11 but buf=hello worl
$

[Florian Weimer (fw)]

The C standard says that the input is an array of the specified size. So I think an application that does this triggers undefined behavior.

We could support this as an extension, by extending the end-of-address-space saturation logic introduced for the fortified variant in this commit:

commit 0d50f477f47ba637b54fb03ac48d769ec4543e8d
Author: Florian Weimer <email address hidden>
Date: Wed Jan 25 08:01:00 2023 +0100

    stdio-common: Handle -1 buffer size in __sprintf_chk & co (bug 30039)

The fortified case is different because the application does not specify the -1 buffer size in that case, so there's no application bug.

[Michael Hudson-Doyle (mwhudson)]

I can't find language in the standard that says that, the description of snprintf just says that output won't be written past the specified size. Is there some more generic section that covers this? Obviously causing output to be written beyond the actual size of the array is undefined behaviour. I'm not going to argue this is good code (sprintf is bad -> let's call snprintf with INT_MAX is some impressive thinking).

[Florian Weimer (fw)]

It is usually assumed that it's implied by using the term “array” and accepting a size argument. One could perhaps argue that the standard does not state explicitly how the size argument and the array length are related.

Previous upstream discussions for other *n* string function strongly went in favor of the array length interpretation, even in cases where existing programming practice is clearly different (although I believe this is just a documentation issue, the implementations work as programmers expect).

Anyway, it's probably a good idea to bring up the snprintf case on libc-alpha.

[Simon Chopin (schopin)]

A related bug is that one: https://bugs.launchpad.net/ubuntu/+source/notcurses/+bug/2008108

I've raised the issue upstream: https://sourceware.org/pipermail/libc-alpha/2023-March/146343.html

Note that even though I technically asked the question "should we fix it?", my personal conviction is that it's not worth it. The issue triggers whenever the given size overflows the address space, which happens there because stack addressed are around 0xffxxxxxx in 32-bit architectures, so INT_MAX would overflow. Switch the buffer to a heap-allocated one, and the issue disappears.

You can reproduce it on other architectures by substituting INT_MAX with ~(size_t)buf+1, which should overflow the address space by 1.

IMHO both the notcurses and cyrus-imapd authors are abusing the API in a way that's just broken and doesn't actually translate the (apparent) intent, which is to say "just write as far as you can". Which would of course be better served by simply using sprintf.

The code responsible for the truncature is there: https://sourceware.org/git/?p=glibc.git;a=blob;f=libio/vsnprintf.c;h=aab32aa9e7a35745713875b7fdfafbefcb31632c;hb=9e2ff880f3cbc0b4ec8505ad2ce4a1c92d7f6d56#l85

[Simon Chopin (schopin)]

Marking as Invalid in glibc as it's not possible to trigger the regression with valid arguments (with the understanding that valid here is "n is the actual size of the buffer"): if it's the size of the buffer, by definition it can't overflow the address space.

[Simon Chopin (schopin)]

Steve pointed out to me that the new glibc behavior still breaks existing cyrus-imapd binaries, and we should ensure that the fixed cyrus-imapd package is upgraded at the same time as glibc (using Breaks declaration), thus reopening the bug while lowering severity.

[Launchpad Janitor (janitor)]

This bug was fixed in the package cyrus-imapd - 3.6.1-2ubuntu1

---------------
cyrus-imapd (3.6.1-2ubuntu1) lunar; urgency=medium

  * debian/patches/no-INT_MAX-snprintf.patch: don't let callers pass
    INT_MAX as a length to snprintf(). Closes LP: #2011326.

 -- Steve Langasek <email address hidden> Wed, 15 Mar 2023 15:11:47 +0000

[Launchpad Janitor (janitor)]

This bug was fixed in the package glibc - 2.37-0ubuntu2

---------------
glibc (2.37-0ubuntu2) lunar; urgency=medium

  * d/p/u/lp2007599*.patch: add tunables for s390x (LP: #2007599)
  * d/p/any/local-ldso-disable-hwcap: fix compilation error (LP: #2006485)
  * d/sysdeps/arm64.mk: enable Memory Tagging Extension (MTE) checking on arm64
    (LP: #2006739)
  * d/control: declare a Breaks on older cyrus-imapd (LP: #2011326)
  * d/control: Fix missing version bumps that could cause issues on upgrades
  * Cherry-pick patches from upstream maintenance branch:
    - 0001-cdefs-Limit-definition-of-fortification-macros.patch
    - 0002-LoongArch-Add-new-relocation-types.patch
    - 0003-Use-64-bit-time_t-interfaces-in-strftime-and-strptim.patch
    - 0004-Account-for-grouping-in-printf-width-bug-30068.patch
    - 0005-NEWS-Document-CVE-2023-25139.patch
    - 0006-elf-Smoke-test-ldconfig-p-against-system-etc-ld.so.c.patch
    - 0007-stdlib-Undo-post-review-change-to-16adc58e73f3-BZ-27.patch
    - 0008-elf-Restore-ldconfig-libc6-implicit-soname-logic-BZ-.patch

 -- Simon Chopin <email address hidden> Thu, 16 Mar 2023 09:44:01 +0100