转发: avoid snprintf using %n to generate coredump when F_S=2 is enabled
Checks
Commit Message
Bug 30795: https://sourceware.org/bugzilla/show_bug.cgi?id=30795
The patch to solve the problem is as follows:
From 4816192ca348e55b7b1d33feac9298d5b0ffb04c Mon Sep 17 00:00:00 2001
From: zhanghao<zhanghao383@huawei.com>
Date: Mon, 21 Aug 2023 15:39:56 +0800
Subject: [PATCH] Avoid snprintf using %n to generate coredump when F_S=2 is enabled
In nscd, F_S=2 added in 233399bce2e79e5af3b344782e9943d5f1a9cdcb just for warn_if_unused
warnings rather than anything substantial.
When F_S=2 is set, and snprintf() using %n will generate coredump and give the
following prompt:
*** %n in writable segment detected ***
It is not recommended to use %n to calculate the length of the string in the snprintf function. We strip the calculation logic outside the snprintf function for replacement.
---
nscd/grpcache.c | 5 +++--
nscd/pwdcache.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
--
2.33.0
Comments
* zhanghao via Libc-alpha:
> Subject: [PATCH] Avoid snprintf using %n to generate coredump when F_S=2 is enabled
>
> In nscd, F_S=2 added in 233399bce2e79e5af3b344782e9943d5f1a9cdcb just for warn_if_unused
> warnings rather than anything substantial.
F_S is _FORTIFY_SOURCE, maybe spell this out?
> When F_S=2 is set, and snprintf() using %n will generate coredump and give the
> following prompt:
>
> *** %n in writable segment detected ***
>
> It is not recommended to use %n to calculate the length of the string
> in the snprintf function. We strip the calculation logic outside the
> snprintf function for replacement.
So … why is the segment writable? It's a string literal, so it should
end up in .rodata. If nscd is crashing due to this, either the writable
data detection is broken, or nscd is linked incorrectly. For example,
nscd might have a RWX LOAD segement.
Thanks,
Florian
On Aug 25 2023, zhanghao (ES) via Libc-alpha wrote:
> When F_S=2 is set, and snprintf() using %n will generate coredump
That's not true if core dumps are disabled.
On Fri, Aug 25, 2023 at 3:01 AM Florian Weimer via Libc-alpha <
libc-alpha@sourceware.org> wrote:
> either the writable
> data detection is broken, or nscd is linked incorrectly.
>
As __readonly_area swallows all possible parsing errors it wouldn't be
surprising it was broken.
BTW..is there really no other way to tell than to parse /proc/self/maps ?
last time I checked I could find an alternative way :-(
Florian Weimer via Libc-alpha <libc-alpha@sourceware.org> writes:
> * zhanghao via Libc-alpha:
>
>> Subject: [PATCH] Avoid snprintf using %n to generate coredump when F_S=2 is enabled
>>
>> In nscd, F_S=2 added in 233399bce2e79e5af3b344782e9943d5f1a9cdcb just for warn_if_unused
>> warnings rather than anything substantial.
>
> F_S is _FORTIFY_SOURCE, maybe spell this out?
>
>> When F_S=2 is set, and snprintf() using %n will generate coredump and give the
>> following prompt:
>>
>> *** %n in writable segment detected ***
>>
>> It is not recommended to use %n to calculate the length of the string
>> in the snprintf function. We strip the calculation logic outside the
>> snprintf function for replacement.
>
> So … why is the segment writable? It's a string literal, so it should
> end up in .rodata. If nscd is crashing due to this, either the writable
> data detection is broken, or nscd is linked incorrectly. For example,
> nscd might have a RWX LOAD segement.
Thanks, I was wondering the same thing.
A copy of the binary might be instructive.
* Cristian Rodríguez:
> As __readonly_area swallows all possible parsing errors it wouldn't be
> surprising it was broken.
We'd get more such reports if it was indeed broken, but I guess it's
possible if the bug is subtle.
> BTW..is there really no other way to tell than to parse
> /proc/self/maps ? last time I checked I could find an alternative way
> :-(
For the last-ditch fallback, I don't know of an alternative. We could
use the _dl_find_object machinery to determine if it's in the read-only
area of a loaded object (the common case).
And the compiler knows in most cases the the argument is a literal
(which hopefully lands in .rodata, but if not, things are horribly
broken anyway). We could tweak the wrapper and implementation to convey
that information, so that we rarely have to perform this check.
Thanks,
Florian
On Fri, Aug 25, 2023 at 11:28 AM Sam James via Libc-alpha <
libc-alpha@sourceware.org> wrote:
>
>
>
> A copy of the binary might be instructive.
>
Not only instructive but necessary and which toolchain exactly. because
either libc __readonly_area did something it shouldn't.. or something
failed pretty spectacularly with the toolchain and really needs further
investigation.! (if string literal not in read only section, kaboom!)
@@ -176,8 +176,9 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
/* We need this to insert the `bygid' entry. */
int key_offset;
- n = snprintf (buf, buf_len, "%d%c%n%s", grp->gr_gid, '\0',
- &key_offset, (char *) key) + 1;
+ n = snprintf (buf, buf_len, "%d%c%s", grp->gr_gid, '\0',
+ (char *) key) + 1;
+ key_offset = n - strlen((char *) key)- 1;
/* Determine the length of all members. */
while (grp->gr_mem[gr_mem_cnt])
@@ -180,8 +180,9 @@ cache_addpw (struct database_dyn *db, int fd, request_header *req,
/* We need this to insert the `byuid' entry. */
int key_offset;
- n = snprintf (buf, buf_len, "%d%c%n%s", pwd->pw_uid, '\0',
- &key_offset, (char *) key) + 1;
+ n = snprintf (buf, buf_len, "%d%c%s", pwd->pw_uid, '\0',
+ (char *) key) + 1;
+ key_offset = n - strlen((char *) key) - 1;
total = (offsetof (struct dataset, strdata)
+ pw_name_len + pw_passwd_len
--
2.33.0
发件人: zhanghao (ES)
发送时间: 2023年8月21日 16:47
收件人: 'admin-requests@sourceware.org' <admin-requests@sourceware.org>
抄送: Yanan (Euler) <yanan@huawei.com>; Caowangbao <caowangbao@huawei.com>; liaichun <liaichun@huawei.com>; chenhaixiang (A) <chenhaixiang3@huawei.com>
主题: avoid snprintf using %n to generate coredump when F_S=2 is enabled
Dear all,
Recently, we found that two coredump occurred when nscd involved calling the snprintf function and using %n and F_S=2 is set, the following two call stacks:
Coredump1:
[cid:image003.jpg@01D9D44E.FBA59250]
Coredump2:
[cid:image005.jpg@01D9D44E.FBA59250]
and give the following prompt:
And the input parameters of the two call stacks look normal.
Involved version: glibc 2.34
We use a simple test case to verify it:
#include <stdio.h>
#include <string.h>
int main ()
{
char fmtstring[10];
char buf[100];
int count = -1;
strcpy (fmtstring, "%d%n");
snprintf (buf, 100, fmtstring, 123, &count);
return 0;
}
when compiling with
gcc snprintf_test.c -fstack-protector -Wall -Wformat -Wformat-security -D_FORTIFY_SOURCE=2 -O2 -o snprintf_test -g
./ snprintf_test
Aborted (core dumped)
when compiling with
gcc snprintf_test.c -fstack-protector -Wall -Wformat -Wformat-security -O2 -o snprintf_test -g
./ snprintf_test
no core dumped
We strip the calculation logic outside the snprintf function for replacement:
From 4816192ca348e55b7b1d33feac9298d5b0ffb04c Mon Sep 17 00:00:00 2001
From: zhanghao<zhanghao383@huawei.com<mailto:zhanghao383@huawei.com>>
Date: Mon, 21 Aug 2023 15:39:56 +0800
Subject: [PATCH] Avoid snprintf using %n to generate coredump when F_S=2 is enabled
In nscd, F_S=2 added in 233399bce2e79e5af3b344782e9943d5f1a9cdcb just for warn_if_unused
warnings rather than anything substantial.
When F_S=2 is set, and snprintf() using %n will generate coredump and give the
following prompt:
It is not recommended to use %n to calculate the length of the string in the snprintf function. We strip the calculation logic outside the snprintf function for replacement.
---
nscd/grpcache.c | 5 +++--
nscd/pwdcache.c | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
@@ -176,8 +176,9 @@ cache_addgr (struct database_dyn *db, int fd, request_header *req,
/* We need this to insert the `bygid' entry. */
int key_offset;
- n = snprintf (buf, buf_len, "%d%c%n%s", grp->gr_gid, '\0',
- &key_offset, (char *) key) + 1;
+ n = snprintf (buf, buf_len, "%d%c%s", grp->gr_gid, '\0',
+ (char *) key) + 1;
+ key_offset = n - strlen((char *) key)- 1;
/* Determine the length of all members. */
while (grp->gr_mem[gr_mem_cnt])
@@ -180,8 +180,9 @@ cache_addpw (struct database_dyn *db, int fd, request_header *req,
/* We need this to insert the `byuid' entry. */
int key_offset;
- n = snprintf (buf, buf_len, "%d%c%n%s", pwd->pw_uid, '\0',
- &key_offset, (char *) key) + 1;
+ n = snprintf (buf, buf_len, "%d%c%s", pwd->pw_uid, '\0',
+ (char *) key) + 1;
+ key_offset = n - strlen((char *) key) - 1;
total = (offsetof (struct dataset, strdata)
+ pw_name_len + pw_passwd_len