* [committed] CVE-2023-4911 @ 2023-10-03 17:08 Siddhesh Poyarekar 2023-10-03 17:08 ` [committed 1/2] Propagate GLIBC_TUNABLES in setxid binaries Siddhesh Poyarekar 2023-10-03 17:08 ` [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) Siddhesh Poyarekar 0 siblings, 2 replies; 7+ messages in thread From: Siddhesh Poyarekar @ 2023-10-03 17:08 UTC (permalink / raw) To: libc-alpha Backport fixes for privilege escalation due to buffer overwrite in tunables processing. Thanks, Sid -- 2.41.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [committed 1/2] Propagate GLIBC_TUNABLES in setxid binaries 2023-10-03 17:08 [committed] CVE-2023-4911 Siddhesh Poyarekar @ 2023-10-03 17:08 ` Siddhesh Poyarekar 2023-10-03 17:08 ` [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) Siddhesh Poyarekar 1 sibling, 0 replies; 7+ messages in thread From: Siddhesh Poyarekar @ 2023-10-03 17:08 UTC (permalink / raw) To: libc-alpha; +Cc: Carlos O'Donell GLIBC_TUNABLES scrubbing happens earlier than envvar scrubbing and some tunables are required to propagate past setxid boundary, like their env_alias. Rely on tunable scrubbing to clean out GLIBC_TUNABLES like before, restoring behaviour in glibc 2.37 and earlier. Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Reviewed-by: Carlos O'Donell <carlos@redhat.com> --- sysdeps/generic/unsecvars.h | 1 - 1 file changed, 1 deletion(-) diff --git a/sysdeps/generic/unsecvars.h b/sysdeps/generic/unsecvars.h index 81397fb90b..8278c50a84 100644 --- a/sysdeps/generic/unsecvars.h +++ b/sysdeps/generic/unsecvars.h @@ -4,7 +4,6 @@ #define UNSECURE_ENVVARS \ "GCONV_PATH\0" \ "GETCONF_DIR\0" \ - "GLIBC_TUNABLES\0" \ "HOSTALIASES\0" \ "LD_AUDIT\0" \ "LD_DEBUG\0" \ -- 2.41.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) 2023-10-03 17:08 [committed] CVE-2023-4911 Siddhesh Poyarekar 2023-10-03 17:08 ` [committed 1/2] Propagate GLIBC_TUNABLES in setxid binaries Siddhesh Poyarekar @ 2023-10-03 17:08 ` Siddhesh Poyarekar 2023-10-03 18:07 ` Adhemerval Zanella Netto 1 sibling, 1 reply; 7+ messages in thread From: Siddhesh Poyarekar @ 2023-10-03 17:08 UTC (permalink / raw) To: libc-alpha; +Cc: Carlos O'Donell The string parsing routine may end up writing beyond bounds of tunestr if the input tunable string is malformed, of the form name=name=val. This gets processed twice, first as name=name=val and next as name=val, resulting in tunestr being name=name=val:name=val, thus overflowing tunestr. Terminate the parsing loop at the first instance itself so that tunestr does not overflow. This also fixes up tst-env-setuid-tunables to actually handle failures correct and add new tests to validate the fix for this CVE. Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Reviewed-by: Carlos O'Donell <carlos@redhat.com> --- NEWS | 5 +++++ elf/dl-tunables.c | 17 +++++++++------- elf/tst-env-setuid-tunables.c | 37 +++++++++++++++++++++++++++-------- 3 files changed, 44 insertions(+), 15 deletions(-) diff --git a/NEWS b/NEWS index a94650da64..cc4b81f0ac 100644 --- a/NEWS +++ b/NEWS @@ -64,6 +64,11 @@ Security related changes: an application calls getaddrinfo for AF_INET6 with AI_CANONNAME, AI_ALL and AI_V4MAPPED flags set. + CVE-2023-4911: If a tunable of the form NAME=NAME=VAL is passed in the + environment of a setuid program and NAME is valid, it may result in a + buffer overflow, which could be exploited to achieve escalated + privileges. This flaw was introduced in glibc 2.34. + The following bugs are resolved with this release: [The release manager will add the list generated by diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c index 62b7332d95..cae67efa0a 100644 --- a/elf/dl-tunables.c +++ b/elf/dl-tunables.c @@ -180,11 +180,7 @@ parse_tunables (char *tunestr, char *valstring) /* If we reach the end of the string before getting a valid name-value pair, bail out. */ if (p[len] == '\0') - { - if (__libc_enable_secure) - tunestr[off] = '\0'; - return; - } + break; /* We did not find a valid name-value pair before encountering the colon. */ @@ -244,9 +240,16 @@ parse_tunables (char *tunestr, char *valstring) } } - if (p[len] != '\0') - p += len + 1; + /* We reached the end while processing the tunable string. */ + if (p[len] == '\0') + break; + + p += len + 1; } + + /* Terminate tunestr before we leave. */ + if (__libc_enable_secure) + tunestr[off] = '\0'; } /* Enable the glibc.malloc.check tunable in SETUID/SETGID programs only when diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c index 7dfb0e073a..f0b92c97e7 100644 --- a/elf/tst-env-setuid-tunables.c +++ b/elf/tst-env-setuid-tunables.c @@ -50,6 +50,8 @@ const char *teststrings[] = "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096", "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096", "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096", + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096", + "glibc.malloc.check=2", "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2", "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096", ":glibc.malloc.garbage=2:glibc.malloc.check=1", @@ -68,6 +70,8 @@ const char *resultstrings[] = "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096", "glibc.malloc.mmap_threshold=4096", "glibc.malloc.mmap_threshold=4096", + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096", + "", "", "", "", @@ -81,11 +85,18 @@ test_child (int off) { const char *val = getenv ("GLIBC_TUNABLES"); + printf (" [%d] GLIBC_TUNABLES is %s\n", off, val); + fflush (stdout); if (val != NULL && strcmp (val, resultstrings[off]) == 0) return 0; if (val != NULL) - printf ("[%d] Unexpected GLIBC_TUNABLES VALUE %s\n", off, val); + printf (" [%d] Unexpected GLIBC_TUNABLES VALUE %s, expected %s\n", + off, val, resultstrings[off]); + else + printf (" [%d] GLIBC_TUNABLES environment variable absent\n", off); + + fflush (stdout); return 1; } @@ -106,21 +117,26 @@ do_test (int argc, char **argv) if (ret != 0) exit (1); - exit (EXIT_SUCCESS); + /* Special return code to make sure that the child executed all the way + through. */ + exit (42); } else { - int ret = 0; - /* Spawn tests. */ for (int i = 0; i < array_length (teststrings); i++) { char buf[INT_BUFSIZE_BOUND (int)]; - printf ("Spawned test for %s (%d)\n", teststrings[i], i); + printf ("[%d] Spawned test for %s\n", i, teststrings[i]); snprintf (buf, sizeof (buf), "%d\n", i); + fflush (stdout); if (setenv ("GLIBC_TUNABLES", teststrings[i], 1) != 0) - exit (1); + { + printf (" [%d] Failed to set GLIBC_TUNABLES: %m", i); + support_record_failure (); + continue; + } int status = support_capture_subprogram_self_sgid (buf); @@ -128,9 +144,14 @@ do_test (int argc, char **argv) if (WEXITSTATUS (status) == EXIT_UNSUPPORTED) return EXIT_UNSUPPORTED; - ret |= status; + if (WEXITSTATUS (status) != 42) + { + printf (" [%d] child failed with status %d\n", i, + WEXITSTATUS (status)); + support_record_failure (); + } } - return ret; + return 0; } } -- 2.41.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) 2023-10-03 17:08 ` [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) Siddhesh Poyarekar @ 2023-10-03 18:07 ` Adhemerval Zanella Netto 2023-10-03 18:16 ` Siddhesh Poyarekar 0 siblings, 1 reply; 7+ messages in thread From: Adhemerval Zanella Netto @ 2023-10-03 18:07 UTC (permalink / raw) To: Siddhesh Poyarekar, libc-alpha; +Cc: Carlos O'Donell On 03/10/23 14:08, Siddhesh Poyarekar wrote: > The string parsing routine may end up writing beyond bounds of tunestr > if the input tunable string is malformed, of the form name=name=val. > This gets processed twice, first as name=name=val and next as name=val, > resulting in tunestr being name=name=val:name=val, thus overflowing > tunestr. > > Terminate the parsing loop at the first instance itself so that tunestr > does not overflow. > > This also fixes up tst-env-setuid-tunables to actually handle failures > correct and add new tests to validate the fix for this CVE. > > Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > NEWS | 5 +++++ > elf/dl-tunables.c | 17 +++++++++------- > elf/tst-env-setuid-tunables.c | 37 +++++++++++++++++++++++++++-------- > 3 files changed, 44 insertions(+), 15 deletions(-) > > diff --git a/NEWS b/NEWS > index a94650da64..cc4b81f0ac 100644 > --- a/NEWS > +++ b/NEWS > @@ -64,6 +64,11 @@ Security related changes: > an application calls getaddrinfo for AF_INET6 with AI_CANONNAME, > AI_ALL and AI_V4MAPPED flags set. > > + CVE-2023-4911: If a tunable of the form NAME=NAME=VAL is passed in the > + environment of a setuid program and NAME is valid, it may result in a > + buffer overflow, which could be exploited to achieve escalated > + privileges. This flaw was introduced in glibc 2.34. > + > The following bugs are resolved with this release: > > [The release manager will add the list generated by > diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c > index 62b7332d95..cae67efa0a 100644 > --- a/elf/dl-tunables.c > +++ b/elf/dl-tunables.c > @@ -180,11 +180,7 @@ parse_tunables (char *tunestr, char *valstring) > /* If we reach the end of the string before getting a valid name-value > pair, bail out. */ > if (p[len] == '\0') > - { > - if (__libc_enable_secure) > - tunestr[off] = '\0'; > - return; > - } > + break; > > /* We did not find a valid name-value pair before encountering the > colon. */ > @@ -244,9 +240,16 @@ parse_tunables (char *tunestr, char *valstring) > } > } > > - if (p[len] != '\0') > - p += len + 1; > + /* We reached the end while processing the tunable string. */ > + if (p[len] == '\0') > + break; > + > + p += len + 1; > } > + > + /* Terminate tunestr before we leave. */ > + if (__libc_enable_secure) > + tunestr[off] = '\0'; > } > So how should we handle what might be inconsistent invalid inputs like: glibc.malloc.mmap_threshold=4096=4096 Since glibc does not provide any TUNABLE_SECLEVEL_NONE, this tunables will be ignored. But for TUNABLE_SECLEVEL_NONE one, the value is still parsed by _dl_strtoul or stored in the tunable. > /* Enable the glibc.malloc.check tunable in SETUID/SETGID programs only when > diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c > index 7dfb0e073a..f0b92c97e7 100644 > --- a/elf/tst-env-setuid-tunables.c > +++ b/elf/tst-env-setuid-tunables.c > @@ -50,6 +50,8 @@ const char *teststrings[] = > "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096", > "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096", > "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.check=2", > "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2", > "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096", > ":glibc.malloc.garbage=2:glibc.malloc.check=1", > @@ -68,6 +70,8 @@ const char *resultstrings[] = > "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096", > "glibc.malloc.mmap_threshold=4096", > "glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.mmap_threshold=glibc.malloc.mmap_threshold=4096", > + "", > "", > "", > "", > @@ -81,11 +85,18 @@ test_child (int off) > { > const char *val = getenv ("GLIBC_TUNABLES"); > > + printf (" [%d] GLIBC_TUNABLES is %s\n", off, val); > + fflush (stdout); > if (val != NULL && strcmp (val, resultstrings[off]) == 0) > return 0; > > if (val != NULL) > - printf ("[%d] Unexpected GLIBC_TUNABLES VALUE %s\n", off, val); > + printf (" [%d] Unexpected GLIBC_TUNABLES VALUE %s, expected %s\n", > + off, val, resultstrings[off]); > + else > + printf (" [%d] GLIBC_TUNABLES environment variable absent\n", off); > + > + fflush (stdout); > > return 1; > } > @@ -106,21 +117,26 @@ do_test (int argc, char **argv) > if (ret != 0) > exit (1); > > - exit (EXIT_SUCCESS); > + /* Special return code to make sure that the child executed all the way > + through. */ > + exit (42); > } > else > { > - int ret = 0; > - > /* Spawn tests. */ > for (int i = 0; i < array_length (teststrings); i++) > { > char buf[INT_BUFSIZE_BOUND (int)]; > > - printf ("Spawned test for %s (%d)\n", teststrings[i], i); > + printf ("[%d] Spawned test for %s\n", i, teststrings[i]); > snprintf (buf, sizeof (buf), "%d\n", i); > + fflush (stdout); > if (setenv ("GLIBC_TUNABLES", teststrings[i], 1) != 0) > - exit (1); > + { > + printf (" [%d] Failed to set GLIBC_TUNABLES: %m", i); > + support_record_failure (); > + continue; > + } > > int status = support_capture_subprogram_self_sgid (buf); > > @@ -128,9 +144,14 @@ do_test (int argc, char **argv) > if (WEXITSTATUS (status) == EXIT_UNSUPPORTED) > return EXIT_UNSUPPORTED; > > - ret |= status; > + if (WEXITSTATUS (status) != 42) > + { > + printf (" [%d] child failed with status %d\n", i, > + WEXITSTATUS (status)); > + support_record_failure (); > + } > } > - return ret; > + return 0; > } > } > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) 2023-10-03 18:07 ` Adhemerval Zanella Netto @ 2023-10-03 18:16 ` Siddhesh Poyarekar 2023-10-04 12:20 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 7+ messages in thread From: Siddhesh Poyarekar @ 2023-10-03 18:16 UTC (permalink / raw) To: Adhemerval Zanella Netto, libc-alpha; +Cc: Carlos O'Donell, Arjun Shankar On 2023-10-03 14:07, Adhemerval Zanella Netto wrote: > So how should we handle what might be inconsistent invalid inputs like: > > glibc.malloc.mmap_threshold=4096=4096 > > Since glibc does not provide any TUNABLE_SECLEVEL_NONE, this tunables > will be ignored. But for TUNABLE_SECLEVEL_NONE one, the value is > still parsed by _dl_strtoul or stored in the tunable. > Ack, it probably makes sense to drop all tunables that don't match at this stage. Arjun is going to rework the parsing for pr#30683 and it probably makes sense to, in addition to enhancing the parsing, also weed out invalid inputs and harden around allocations for the tunable string to provide some resilience against overflows. The other aspect to think about may be the utility of passing tunables to (or through) setuid programs. I had done it to maintain compatibility with the malloc envvars that were getting passed through, but maybe it's a good idea to filter all of them out. Perhaps with systemwide tunables we could even have a way for tunables to be read in sxid programs in a safer way. Thanks, Sid ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) 2023-10-03 18:16 ` Siddhesh Poyarekar @ 2023-10-04 12:20 ` Adhemerval Zanella Netto 2023-10-04 12:34 ` Siddhesh Poyarekar 0 siblings, 1 reply; 7+ messages in thread From: Adhemerval Zanella Netto @ 2023-10-04 12:20 UTC (permalink / raw) To: Siddhesh Poyarekar, libc-alpha; +Cc: Carlos O'Donell, Arjun Shankar On 03/10/23 15:16, Siddhesh Poyarekar wrote: > On 2023-10-03 14:07, Adhemerval Zanella Netto wrote: >> So how should we handle what might be inconsistent invalid inputs like: >> >> glibc.malloc.mmap_threshold=4096=4096 >> >> Since glibc does not provide any TUNABLE_SECLEVEL_NONE, this tunables >> will be ignored. But for TUNABLE_SECLEVEL_NONE one, the value is >> still parsed by _dl_strtoul or stored in the tunable. >> > > Ack, it probably makes sense to drop all tunables that don't match at this stage. Arjun is going to rework the parsing for pr#30683 and it probably makes sense to, in addition to enhancing the parsing, also weed out invalid inputs and harden around allocations for the tunable string to provide some resilience against overflows. > > The other aspect to think about may be the utility of passing tunables to (or through) setuid programs. I had done it to maintain compatibility with the malloc envvars that were getting passed through, but maybe it's a good idea to filter all of them out. Perhaps with systemwide tunables we could even have a way for tunables to be read in sxid programs in a safer way. I think it would be best to avoid changing AT_SECURE binaries semantic through tunables or even environment setting (/etc/suid-debug). It means adding back GLIBC_TUNABLES to unsecvars (so even non AT_SECURE binaries won't see GLIBC_TUNABLES), do not process GLIBC_TUNABLES for AT_SECURE (including malloc mcheck), and dropping any ill-formed GLIBC_TUNABLES strings (so first parse and only apply well-formatted ones). This would allow to just remove the tunables_strdup altogether, simplifying the code a lot. It also means that any security tunable (such as the glibc.mem.tagging) would also stop appling to AT_SECURE, but I think we should stopping giving users to change secure binares semantics even in this way. And I don't think we should make this changes iff we have a a trusted system-wide tunable. I am working on coming up with this ideas, and we can discuss whether adding trusting system-wide tunable would be a good idea. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) 2023-10-04 12:20 ` Adhemerval Zanella Netto @ 2023-10-04 12:34 ` Siddhesh Poyarekar 0 siblings, 0 replies; 7+ messages in thread From: Siddhesh Poyarekar @ 2023-10-04 12:34 UTC (permalink / raw) To: Adhemerval Zanella Netto, libc-alpha; +Cc: Carlos O'Donell, Arjun Shankar On 2023-10-04 08:20, Adhemerval Zanella Netto wrote: > I think it would be best to avoid changing AT_SECURE binaries semantic > through tunables or even environment setting (/etc/suid-debug). It > means adding back GLIBC_TUNABLES to unsecvars (so even non AT_SECURE > binaries won't see GLIBC_TUNABLES), do not process GLIBC_TUNABLES for > AT_SECURE (including malloc mcheck), and dropping any ill-formed > GLIBC_TUNABLES strings (so first parse and only apply well-formatted > ones). I started that process with this patchset[1] that's probably the most contentious (or maybe not) issue. If we get consensus on that, we can move to drop GLIBC_TUNABLES support completely for AT_SECURE in 2.39. > This would allow to just remove the tunables_strdup altogether, > simplifying the code a lot. It also means that any security tunable > (such as the glibc.mem.tagging) would also stop appling to AT_SECURE, > but I think we should stopping giving users to change secure binares > semantics even in this way. We won't get to drop tunables_strdup; it's still needed to make a copy to delimit tunable values. We can drop all the twiddling under the __libc_enable_secure though. > And I don't think we should make this changes iff we have a a trusted > system-wide tunable. Ack, I can live with that. Thanks, Sid [1] https://patchwork.sourceware.org/project/glibc/list/?series=25312 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-10-04 12:34 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-03 17:08 [committed] CVE-2023-4911 Siddhesh Poyarekar 2023-10-03 17:08 ` [committed 1/2] Propagate GLIBC_TUNABLES in setxid binaries Siddhesh Poyarekar 2023-10-03 17:08 ` [committed 2/2] tunables: Terminate if end of input is reached (CVE-2023-4911) Siddhesh Poyarekar 2023-10-03 18:07 ` Adhemerval Zanella Netto 2023-10-03 18:16 ` Siddhesh Poyarekar 2023-10-04 12:20 ` Adhemerval Zanella Netto 2023-10-04 12:34 ` Siddhesh Poyarekar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).