diff mbox series

转发: avoid snprintf using %n to generate coredump when F_S=2 is enabled

Message ID 8f63a5a69ee84f38b4e73ae5b5ff38ef@huawei.com
State New
Headers
Series 转发: avoid snprintf using %n to generate coredump when F_S=2 is enabled |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch fail Patch failed to apply to master at the time it was sent
redhat-pt-bot/TryBot-32bit fail Patch series failed to apply
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_glibc_check--master-arm fail Patch failed to apply
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

zhanghao \(ES\) Aug. 25, 2023, 2:49 a.m. UTC 
  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

Florian Weimer Aug. 25, 2023, 7:01 a.m. UTC | #1 
* 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
Andreas Schwab Aug. 25, 2023, 7:11 a.m. UTC | #2 
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.
Cristian Rodríguez Aug. 25, 2023, 3:22 p.m. UTC | #3 
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 :-(
Sam James Aug. 25, 2023, 3:26 p.m. UTC | #4 
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.
Florian Weimer Aug. 25, 2023, 3:35 p.m. UTC | #5 
* 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
Cristian Rodríguez Aug. 26, 2023, 10:25 p.m. UTC | #6 
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!)
diff mbox series

Patch

diff --git a/nscd/grpcache.c b/nscd/grpcache.c
index 457ca4d8..d7200f4e 100644
--- a/nscd/grpcache.c
+++ b/nscd/grpcache.c
@@ -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])
diff --git a/nscd/pwdcache.c b/nscd/pwdcache.c
index dfafb526..37dd402f 100644
--- a/nscd/pwdcache.c
+++ b/nscd/pwdcache.c
@@ -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:
*** %n in writable segment detected ***
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
*** %n in writable segment detected ***
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:

*** %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(-)

diff --git a/nscd/grpcache.c b/nscd/grpcache.c
index 457ca4d8..d7200f4e 100644
--- a/nscd/grpcache.c
+++ b/nscd/grpcache.c
@@ -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])
diff --git a/nscd/pwdcache.c b/nscd/pwdcache.c
index dfafb526..37dd402f 100644
--- a/nscd/pwdcache.c
+++ b/nscd/pwdcache.c
@@ -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