[v2,1/4] CVE-2024-33599: nscd: Stack-based buffer overflow in netgroup cache (bug 31677)
Checks
| Context |
Check |
Description |
| redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
Commit Message
Using alloca matches what other caches do. The request length is
bounded by MAXKEYLEN.
---
v2: Added the comment Sid requested. CVE assignment.
nscd/netgroupcache.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
base-commit: f4724843ada64a51d66f65d3199fe431f9d4c254
Comments
On 4/24/24 16:54, Florian Weimer wrote:
> Using alloca matches what other caches do. The request length is
> bounded by MAXKEYLEN.
LGTM. Provides comment that Siddhesh requested in v1 review.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
> v2: Added the comment Sid requested. CVE assignment.
> nscd/netgroupcache.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c
> index 0c6e46f15c..f227dc7fa2 100644
> --- a/nscd/netgroupcache.c
> +++ b/nscd/netgroupcache.c
> @@ -502,12 +502,13 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
> = (struct indataset *) mempool_alloc (db,
> sizeof (*dataset) + req->key_len,
> 1);
> - struct indataset dataset_mem;
OK. Static structure has no space for key material.
> bool cacheable = true;
> if (__glibc_unlikely (dataset == NULL))
> {
> cacheable = false;
> - dataset = &dataset_mem;
> + /* The alloca is safe because nscd_run_worker verfies that
> + key_len is not larger than MAXKEYLEN. */
> + dataset = alloca (sizeof (*dataset) + req->key_len);
OK. Use of alloca is bounded by MAXKEYLEN.
glibc/nscd/connections.c
1674 /* It should not be possible to crash the nscd with a silly
1675 request (i.e., a terribly large key). We limit the size to 1kb. */
1676 if (__builtin_expect (req.key_len, 1) < 0
1677 || __builtin_expect (req.key_len, 1) > MAXKEYLEN)
1678 {
1679 if (debug_level > 0)
1680 dbg_log (_("key length in request too long: %d"), req.key_len);
1681 }
1682 else
1683 {
1684 /* Get the key. */
1685 char keybuf[MAXKEYLEN + 1];
Thus we limit the stack use to at most 1KiB here for the key material.
We also have a cross check in nscd_helper.c for __nscd_open_socket which means
that the clients we provide will not use nscd if the key length is greater than
this value.
> }
>
> datahead_init_pos (&dataset->head, sizeof (*dataset) + req->key_len,
>
> base-commit: f4724843ada64a51d66f65d3199fe431f9d4c254
@@ -502,12 +502,13 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req,
= (struct indataset *) mempool_alloc (db,
sizeof (*dataset) + req->key_len,
1);
- struct indataset dataset_mem;
bool cacheable = true;
if (__glibc_unlikely (dataset == NULL))
{
cacheable = false;
- dataset = &dataset_mem;
+ /* The alloca is safe because nscd_run_worker verfies that
+ key_len is not larger than MAXKEYLEN. */
+ dataset = alloca (sizeof (*dataset) + req->key_len);
}
datahead_init_pos (&dataset->head, sizeof (*dataset) + req->key_len,