[Bug stdio/28989] New: __snprintf_chk bounds check is too strict

[Bug stdio/28989] New: __snprintf_chk bounds check is too strict

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

            Bug ID: 28989
           Summary: __snprintf_chk bounds check is too strict
           Product: glibc
           Version: 2.35
            Status: NEW
          Severity: normal
          Priority: P2
         Component: stdio
          Assignee: unassigned at sourceware dot org
          Reporter: siddhesh at sourceware dot org
  Target Milestone: ---

__snprintf_chk aborts if the object size is less than the provide length
argument in snprintf.  While this enforces the POSIX requirement of the length
argument being the object size, there's a counter-argument that it's overridden
by the C standard's lack of such a requirement.

We could resolve this by having __vsnprintf_internal accept slen too and call
__chk_fail only if an actual overflow happens but it makes __vsnprintf_internal
more expensive in the general case too.

from the gcc bug:

$ cat sratom.c
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

int size = 3;
unsigned char data = 0xff;

int main()
{
    unsigned len = size * 2 + 1;
    char * str = __builtin_calloc(len, 1);

    for (uint32_t i = 0; i < size; ++i) {
      fprintf (stderr, "i=%i\n", i);
      snprintf((char*)str + (2 * i), len, "%02X", data);
    }

    fprintf (stderr, "R=%s\n", str);
}

$ gcc sratom.c -O2 -D_FORTIFY_SOURCE=3 && ./a.out
i=0
i=1
*** buffer overflow detected ***: terminated
Aborted (core dumped)

$ clang sratom.c -O2 -D_FORTIFY_SOURCE=3 && ./a.out
i=0
i=1
*** buffer overflow detected ***: terminated
Aborted (core dumped)

$ gcc-11 sratom.c -g -O2 -fsanitize=address,undefined && ./a.out 
i=0
i=1
i=2
R=FFFFFF

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

Siddhesh Poyarekar <siddhesh at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=104969

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

--- Comment #1 from Andreas Schwab <schwab@linux-m68k.org> ---
That's not a bug.  Fortify checks can be stricter than what the standards
require, especially to make them cheap enough.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

--- Comment #2 from Siddhesh Poyarekar <siddhesh at sourceware dot org> ---
(In reply to Andreas Schwab from comment #1)
> That's not a bug.  Fortify checks can be stricter than what the standards
> require, especially to make them cheap enough.

FWIW, I agree.  I just want to put the alternative out there for discussion so
that more folks from the community pitch in and our decision and rationale is
documented.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com

--- Comment #3 from Carlos O'Donell <carlos at redhat dot com> ---
I just reviewed the language in the upcoming C2x standard (N2454) and it
remains the same in that the exact language for snprintf is ambiguous about
what happens for the bytes between the actual size of 's' and the point at
which characters start being discarded i.e. 'n-1'.

It is in my opinion a weak argument to say that because C2x is ambiguous that
we should relax the check in __snprintf_chk.

POSIX is clearer in that it states that 'n' is the size of the buffer in 's',
rather than just the point at which discarding starts to happen.

_FORTIFY_SOURCE should allow a strict bounds to catch defects where writes
occur beyond 's' but before 'n-1' which may or may not be out-of-bounds. Yes,
there may be a *practical* false-positive here if the format specifier would
not write more than the actual size of 's', and the caller did not want to
adjust 'n' because it had a runtime cost. In this case I see two scenarios:

(a) Caller doesn't adjust 'n' to match size of 's' because of runtime cost, and
relies on specifier to control how much was written to 's'.
- This is risky since in the future the specifier may change.

(b) __snprintf_chk becomes more expensive to avoid the case where another
restriction (format specifier) would prevent the overflow from happening.

Again, this seems like a weak argument. Relying on the format specifier, which
is in no way directly coupled with 'n' is a recipe for a potential future bug.

The caller may not know the size of 's', in which case it should not use
snprintf since there is no inherent benefit.

I agree with Andreas here, we can be stricter in __snprintf_chk, it is both
cheaper, and safer.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[dj at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

dj at redhat dot com <dj at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dj at redhat dot com

--- Comment #4 from dj at redhat dot com <dj at redhat dot com> ---
I would argue that if you passed a length beyond the buffer, "knowing it will
fit", you might as well use sprintf instead.  The major point of snprintf is to
prevent buffer overflows, so any diagnostic to that effect - whether through a
realized error or predictive - is IMHO acceptable.  Perhaps the wording of the
diagnostic could be more useful if it's predictive, though.

*** possible buffer overflow detected - length exceeds buffer ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[mliska at suse dot cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

Martin Liska <mliska at suse dot cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mliska at suse dot cz

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

--- Comment #5 from Siddhesh Poyarekar <siddhesh at sourceware dot org> ---
(In reply to dj@redhat.com from comment #4)
> *** possible buffer overflow detected - length exceeds buffer ***

Strictly speaking, I agree that the message is misleading, but I'm not sure if
changing it is worthwhile.  If we agree to change it then we should probably do
the same for strncpy, wcsncpy, vsnprintf, etc. too since they all use the same
check to fail on overflow.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[i at maskray dot me]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

Fangrui Song <i at maskray dot me> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://sourceware.org/bugz
                   |                            |illa/show_bug.cgi?id=30441

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug stdio/28989] __snprintf_chk bounds check is too strict

[gabravier at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=28989

Gabriel Ravier <gabravier at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gabravier at gmail dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.