From 62ea1fe973eaf1652b52d613c24a9a0a544d3ae4 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Fri, 10 Jan 2020 12:01:50 -0600 Subject: [PATCH 1/2] OPENAFS-SA-2024-001: afs: Introduce afs_genpag() CVE-2024-10394 Currently, several areas in the code call genpag() to generate a new PAG id, but the signature of genpag() is very limited. To allow for the code in genpag() to return errors and to examine the calling user's credentials, introduce a new function, afs_genpag(), that does the same thing as genpag(), but accepts creds and allows errors to be returned. Convert all existing callers to use afs_genpag() and to handle any errors, though no errors are ever returned in this commit on its own. To ensure there are no old callers of genpag() left around, change the existing genpag() to be called genpagval(), and declare it static. FIXES 135062 Reviewed-on: https://gerrit.openafs.org/14090 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit f701f704c7bc93cf5fd7cffaaa043cef6a99e77f) Reviewed-on: https://gerrit.openafs.org/15927 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 20c22347b41eea2ebbdc0ab15f16c822af44df51) [1.6: In this branch, afs_genpag() never returns an error, and afs_setpag() is not able to handle errors from afs_genpag(). So, afs_setpag() does not check for errors from afs_genpag() in this branch. Also, the signature of afs_genpag() says it accepts an afs_ucred_t** (not an afs_ucred_t*), even though 'acred' must actually be an 'afs_ucred_t*'. Though this is confusing, we keep it this way to match afs_pag_wait() and existing callers. Add some casts to avoid warnings in some places.] Change-Id: I2e7ab827d15d33dbee3195068da6c87b38f30695 --- src/afs/AIX/osi_groups.c | 10 +++++++- src/afs/DARWIN/osi_groups.c | 10 +++++++- src/afs/FBSD/osi_groups.c | 10 +++++++- src/afs/HPUX/osi_groups.c | 10 +++++++- src/afs/IRIX/osi_groups.c | 13 +++++++++- src/afs/LINUX/osi_groups.c | 10 +++++++- src/afs/NBSD/osi_groups.c | 9 ++++++- src/afs/OBSD/osi_groups.c | 10 +++++++- src/afs/SOLARIS/osi_groups.c | 9 +++++-- src/afs/UKERNEL/osi_groups.c | 9 ++++++- src/afs/afs_osi_pag.c | 50 +++++++++++++++++++++++------------- src/afs/afs_pioctl.c | 8 +++++- src/afs/afs_prototypes.h | 2 +- src/afs/afs_stats.h | 2 +- 14 files changed, 130 insertions(+), 32 deletions(-) diff --git a/src/afs/AIX/osi_groups.c b/src/afs/AIX/osi_groups.c index 2ef317f424..f7bfa9bb88 100644 --- a/src/afs/AIX/osi_groups.c +++ b/src/afs/AIX/osi_groups.c @@ -86,6 +86,14 @@ setpag(cred, pagvalue, newpag, change_parent) int j; AFS_STATCNT(setpag); + + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { + return (setuerror(code), code); + } + } + #ifndef AFS_AIX51_ENV ngroups = afs_getgroups(*cred, NGROUPS, gidset); if (afs_get_pag_from_groups(gidset[0], gidset[1]) == NOPAG) { @@ -99,7 +107,7 @@ setpag(cred, pagvalue, newpag, change_parent) ngroups += 2; } #endif - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; #ifdef AFS_AIX51_ENV if (change_parent) { code = kcred_setpag(*cred, PAG_AFS, *newpag); diff --git a/src/afs/DARWIN/osi_groups.c b/src/afs/DARWIN/osi_groups.c index 11e65e3787..2e3750f35d 100644 --- a/src/afs/DARWIN/osi_groups.c +++ b/src/afs/DARWIN/osi_groups.c @@ -97,6 +97,14 @@ setpag(proc, cred, pagvalue, newpag, change_parent) int j; AFS_STATCNT(setpag); + + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { + return code; + } + } + ngroups = afs_getgroups(*cred, NGROUPS, gidset); if (afs_get_pag_from_groups(gidset[1], gidset[2]) == NOPAG) { /* We will have to shift grouplist to make room for pag */ @@ -108,7 +116,7 @@ setpag(proc, cred, pagvalue, newpag, change_parent) } ngroups += 2; } - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_get_groups_from_pag(*newpag, &gidset[1], &gidset[2]); code = afs_setgroups(proc, cred, ngroups, gidset, change_parent); return code; diff --git a/src/afs/FBSD/osi_groups.c b/src/afs/FBSD/osi_groups.c index e897f46104..2950273085 100644 --- a/src/afs/FBSD/osi_groups.c +++ b/src/afs/FBSD/osi_groups.c @@ -95,6 +95,14 @@ setpag(struct thread *td, struct ucred **cred, afs_uint32 pagvalue, int j; AFS_STATCNT(setpag); + + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { + return code; + } + } + #ifdef AFS_FBSD80_ENV gidset = osi_Alloc(gidset_len * sizeof(gid_t)); #endif @@ -109,7 +117,7 @@ setpag(struct thread *td, struct ucred **cred, afs_uint32 pagvalue, } ngroups += 2; } - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_get_groups_from_pag(*newpag, &gidset[1], &gidset[2]); code = afs_setgroups(td, cred, ngroups, gidset, change_parent); #ifdef AFS_FBSD80_ENV diff --git a/src/afs/HPUX/osi_groups.c b/src/afs/HPUX/osi_groups.c index 8d6d9dbd7f..87989f6bc2 100644 --- a/src/afs/HPUX/osi_groups.c +++ b/src/afs/HPUX/osi_groups.c @@ -70,6 +70,14 @@ setpag(cred, pagvalue, newpag, change_parent) int j; AFS_STATCNT(setpag); + + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { + return (setuerror(code), code); + } + } + ngroups = afs_getgroups(*cred, NGROUPS, gidset); if (afs_get_pag_from_groups(gidset[0], gidset[1]) == NOPAG) { /* We will have to shift grouplist to make room for pag */ @@ -81,7 +89,7 @@ setpag(cred, pagvalue, newpag, change_parent) } ngroups += 2; } - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_get_groups_from_pag(*newpag, &gidset[0], &gidset[1]); if (code = afs_setgroups(cred, ngroups, gidset, change_parent)) { diff --git a/src/afs/IRIX/osi_groups.c b/src/afs/IRIX/osi_groups.c index d028c20260..9d1fc3374b 100644 --- a/src/afs/IRIX/osi_groups.c +++ b/src/afs/IRIX/osi_groups.c @@ -276,6 +276,17 @@ setpag(cred, pagvalue, newpag, change_parent) AFS_STATCNT(setpag); + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { +#if defined(KERNEL_HAVE_UERROR) + return (setuerror(code), code); +#else + return code; +#endif + } + } + ngroups = afs_getgroups(*cred, NGROUPS, gidset); if (afs_get_pag_from_groups(gidset[0], gidset[1]) == NOPAG) { /* We will have to shift grouplist to make room for pag */ @@ -291,7 +302,7 @@ setpag(cred, pagvalue, newpag, change_parent) } ngroups += 2; } - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_get_groups_from_pag(*newpag, &gidset[0], &gidset[1]); if (code = afs_setgroups(cred, ngroups, gidset, change_parent)) { #if defined(KERNEL_HAVE_UERROR) diff --git a/src/afs/LINUX/osi_groups.c b/src/afs/LINUX/osi_groups.c index de3671c3ac..05ccb1ed7d 100644 --- a/src/afs/LINUX/osi_groups.c +++ b/src/afs/LINUX/osi_groups.c @@ -144,11 +144,19 @@ __setpag(cred_t **cr, afs_uint32 pagvalue, afs_uint32 *newpag, { struct group_info *group_info; struct group_info *tmp; + int code; + + if (pagvalue == -1) { + code = afs_genpag((afs_ucred_t**)*cr, &pagvalue); + if (code != 0) { + return code; + } + } get_group_info(afs_cr_group_info(*cr)); group_info = afs_cr_group_info(*cr); - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_linux_pag_to_groups(*newpag, group_info, &tmp); if (old_groups) { diff --git a/src/afs/NBSD/osi_groups.c b/src/afs/NBSD/osi_groups.c index 307973ae95..138a7b2d7c 100644 --- a/src/afs/NBSD/osi_groups.c +++ b/src/afs/NBSD/osi_groups.c @@ -88,6 +88,13 @@ setpag(struct proc *proc, afs_ucred_t *cred, afs_uint32 pagvalue, int j; AFS_STATCNT(setpag); + + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { + return code; + } + } ngroups = osi_getgroups(*cred, NGROUPS, gidset); if (afs_get_pag_from_groups(gidset[1], gidset[2]) == NOPAG) { /* We will have to shift grouplist to make room for pag */ @@ -99,7 +106,7 @@ setpag(struct proc *proc, afs_ucred_t *cred, afs_uint32 pagvalue, } ngroups += 2; } - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_get_groups_from_pag(*newpag, &gidset[1], &gidset[2]); code = osi_setgroups(proc, cred, ngroups, gidset, change_parent); return code; diff --git a/src/afs/OBSD/osi_groups.c b/src/afs/OBSD/osi_groups.c index e26088f19d..2c0ada1445 100644 --- a/src/afs/OBSD/osi_groups.c +++ b/src/afs/OBSD/osi_groups.c @@ -80,6 +80,14 @@ setpag(struct proc *proc, struct ucred **cred, afs_uint32 pagvalue, int j; AFS_STATCNT(setpag); + + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { + return code; + } + } + ngroups = afs_getgroups(*cred, NGROUPS, gidset); /* * If the group list is empty, use the task's primary group as the group @@ -101,7 +109,7 @@ setpag(struct proc *proc, struct ucred **cred, afs_uint32 pagvalue, } ngroups += 2; } - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_get_groups_from_pag(*newpag, &gidset[1], &gidset[2]); code = afs_setgroups(proc, cred, ngroups, gidset, change_parent); return code; diff --git a/src/afs/SOLARIS/osi_groups.c b/src/afs/SOLARIS/osi_groups.c index 527964b9f6..3b3074c9d4 100644 --- a/src/afs/SOLARIS/osi_groups.c +++ b/src/afs/SOLARIS/osi_groups.c @@ -176,6 +176,13 @@ setpag(cred, pagvalue, newpag, change_parent) AFS_STATCNT(setpag); + if (pagvalue == -1) { + code = afs_genpag(*cred, &pagvalue); + if (code != 0) { + return code; + } + } + /* Derive gidset size from running kernel's ngroups_max; * default 16, but configurable up to 32 (Sol10) or * 1024 (Sol11). @@ -185,8 +192,6 @@ setpag(cred, pagvalue, newpag, change_parent) /* must use osi_Alloc, osi_AllocSmallSpace may not be enough. */ gidset = osi_Alloc(gidset_sz); - pagvalue = (pagvalue == -1 ? genpag() : pagvalue); - mutex_enter(&curproc->p_crlock); ngroups = afs_getgroups(*cred, gidset); diff --git a/src/afs/UKERNEL/osi_groups.c b/src/afs/UKERNEL/osi_groups.c index f72fd61ace..3aee97f209 100644 --- a/src/afs/UKERNEL/osi_groups.c +++ b/src/afs/UKERNEL/osi_groups.c @@ -74,6 +74,13 @@ usr_setpag(struct usr_ucred **cred, afs_uint32 pagvalue, afs_uint32 * newpag, AFS_STATCNT(setpag); + if (pagvalue == -1) { + code = afs_genpag((afs_ucred_t**)*cred, &pagvalue); + if (code != 0) { + return code; + } + } + gidset = (gid_t *) osi_AllocSmallSpace(AFS_SMALLOCSIZ); ngroups = afs_getgroups(*cred, gidset); @@ -88,7 +95,7 @@ usr_setpag(struct usr_ucred **cred, afs_uint32 pagvalue, afs_uint32 * newpag, } ngroups += 2; } - *newpag = (pagvalue == -1 ? genpag() : pagvalue); + *newpag = pagvalue; afs_get_groups_from_pag(*newpag, &gidset[0], &gidset[1]); if ((code = afs_setgroups(cred, ngroups, gidset, change_parent))) { osi_FreeSmallSpace((char *)gidset); diff --git a/src/afs/afs_osi_pag.c b/src/afs/afs_osi_pag.c index cdb1998595..a57be5e0c2 100644 --- a/src/afs/afs_osi_pag.c +++ b/src/afs/afs_osi_pag.c @@ -9,7 +9,7 @@ /* * Implements: - * genpag + * genpagval * getpag * afs_setpag * AddPag @@ -67,8 +67,8 @@ afs_uint32 pagCounter = 0; * secure (although of course not absolutely secure). */ #if !defined(UKERNEL) -afs_uint32 -genpag(void) +static afs_uint32 +genpagval(void) { AFS_STATCNT(genpag); #ifdef AFS_LINUX20_ENV @@ -96,8 +96,8 @@ getpag(void) /* Web enhancement: we don't need to restrict pags to 41XXXXXX since * we are not sharing the space with anyone. So we use the full 32 bits. */ -afs_uint32 -genpag(void) +static afs_uint32 +genpagval(void) { AFS_STATCNT(genpag); #ifdef AFS_LINUX20_ENV @@ -181,6 +181,18 @@ afs_pag_wait(afs_ucred_t **acred) return 0; } +/* + * Note that 'acred' is actually an 'afs_ucred_t*', not an 'afs_ucred_t**'. But + * various callers and callees expect an 'afs_ucred_t*' due to historical + * reasons. + */ +afs_int32 +afs_genpag(afs_ucred_t **acred, afs_uint32 *apag) +{ + *apag = genpagval(); + return 0; +} + int #if defined(AFS_SUN5_ENV) afs_setpag(afs_ucred_t **credpp) @@ -202,6 +214,7 @@ afs_setpag(void) #endif int code = 0; + afs_uint32 pag; #if defined(AFS_SGI53_ENV) && defined(MP) /* This is our first chance to get the global lock. */ @@ -212,16 +225,17 @@ afs_setpag(void) afs_pag_wait(acred); + afs_genpag(acred, &pag); #if defined(AFS_SUN5_ENV) - code = AddPag(genpag(), credpp); + code = AddPag(pag, credpp); #elif defined(AFS_FBSD_ENV) - code = AddPag(td, genpag(), &td->td_ucred); + code = AddPag(td, pag, &td->td_ucred); #elif defined(AFS_NBSD40_ENV) /* XXXX won't work */ - code = AddPag(p, genpag(), (afs_ucred_t **) osi_curcred()); + code = AddPag(p, pag, (afs_ucred_t **) osi_curcred()); #elif defined(AFS_XBSD_ENV) - code = AddPag(p, genpag(), &p->p_rcred); + code = AddPag(p, pag, &p->p_rcred); #elif defined(AFS_AIX41_ENV) { struct ucred *credp; @@ -229,7 +243,7 @@ afs_setpag(void) credp = crref(); credp0 = credp; - code = AddPag(genpag(), &credp); + code = AddPag(pag, &credp); /* If AddPag() didn't make a new cred, then free our cred ref */ if (credp == credp0) { crfree(credp); @@ -238,36 +252,36 @@ afs_setpag(void) #elif defined(AFS_HPUX110_ENV) { struct ucred *credp = p_cred(u.u_procp); - code = AddPag(genpag(), &credp); + code = AddPag(pag, &credp); } #elif defined(AFS_SGI_ENV) { cred_t *credp; credp = OSI_GET_CURRENT_CRED(); - code = AddPag(genpag(), &credp); + code = AddPag(pag, &credp); } #elif defined(AFS_LINUX20_ENV) { afs_ucred_t *credp = crref(); - code = AddPag(genpag(), &credp); + code = AddPag(pag, &credp); crfree(credp); } #elif defined(AFS_DARWIN80_ENV) { afs_ucred_t *credp = kauth_cred_proc_ref(p); - code = AddPag(p, genpag(), &credp); + code = AddPag(p, pag, &credp); crfree(credp); } #elif defined(AFS_DARWIN_ENV) { afs_ucred_t *credp = crdup(p->p_cred->pc_ucred); - code = AddPag(p, genpag(), &credp); + code = AddPag(p, pag, &credp); crfree(credp); } #elif defined(UKERNEL) - code = AddPag(genpag(), &(get_user_struct()->u_cred)); + code = AddPag(pag, &(get_user_struct()->u_cred)); #else - code = AddPag(genpag(), &u.u_cred); + code = AddPag(pag, &u.u_cred); #endif afs_Trace1(afs_iclSetp, CM_TRACE_SETPAG, ICL_TYPE_INT32, code); @@ -288,7 +302,7 @@ afs_setpag(void) /* * afs_setpag_val * This function is like setpag but sets the current thread's pag id to a - * caller-provided value instead of calling genpag(). This implements a + * caller-provided value instead of calling afs_genpag(). This implements a * form of token caching since the caller can recall a particular pag value * for the thread to restore tokens, rather than reauthenticating. */ diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index d96d7b7148..24c0cc7534 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -4615,7 +4615,13 @@ HandleClientContext(struct afs_ioctl *ablob, int *com, *acred = newcred; if (!code && *com == PSETPAG) { /* Special case for 'setpag' */ - afs_uint32 pagvalue = genpag(); + afs_uint32 pagvalue; + + code = afs_genpag(*acred, &pagvalue); + if (code != 0) { + EXP_RELE(outexporter); + return code; + } au = afs_GetUser(pagvalue, -1, WRITE_LOCK); /* a new unixuser struct */ /* diff --git a/src/afs/afs_prototypes.h b/src/afs/afs_prototypes.h index 2386963599..7e8fa44038 100644 --- a/src/afs/afs_prototypes.h +++ b/src/afs/afs_prototypes.h @@ -589,7 +589,7 @@ extern int afs_setpag(afs_proc_t *p, void *args, int *retval); extern int afs_setpag(void); #endif -extern afs_uint32 genpag(void); +extern afs_int32 afs_genpag(afs_ucred_t **acred, afs_uint32 *apag); extern afs_uint32 getpag(void); #if defined(AFS_FBSD_ENV) extern int AddPag(struct thread *td, afs_int32 aval, afs_ucred_t **credpp); diff --git a/src/afs/afs_stats.h b/src/afs/afs_stats.h index 71e337e630..c24c7202a4 100644 --- a/src/afs/afs_stats.h +++ b/src/afs/afs_stats.h @@ -545,7 +545,7 @@ struct afs_MeanStats { AFS_CS(afs_swapvp) /* afs_vfsops.c */ \ AFS_CS(afs_AddMarinerName) /* afs_vnodeops.c */ \ AFS_CS(afs_setpag) /* afs_vnodeops.c */ \ - AFS_CS(genpag) /* afs_vnodeops.c */ \ + AFS_CS(genpag) /* afs_vnodeops.c, renamed to genpagval() */ \ AFS_CS(getpag) /* afs_vnodeops.c */ \ AFS_CS(afs_GetMariner) /* afs_vnodeops.c */ \ AFS_CS(afs_badop) /* afs_vnodeops.c */ \ -- 2.45.2 From 7722383313efdd0e749b42efd14c465347f0b73b Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Fri, 10 Jan 2020 12:40:15 -0600 Subject: [PATCH 2/2] OPENAFS-SA-2024-001: afs: Throttle PAG creation in afs_genpag() CVE-2024-10394 Currently, we only throttle PAG creation in afs_setpag(). But there are several callers that call setpag() directly, not via afs_setpag; notably _settok_setParentPag in afs_pioctl.c. When setpag() is called with a PAG value of -1, it generates a new PAG internally without any throttling. So, those callers effectively bypass the PAG throttling mechanism, which allows a calling user to create PAGs without any delay. To avoid this, move our afs_pag_wait call from afs_setpag() to afs_genpag(), which all code uses to generate a new PAG value. This ensures that PAG creation is always throttled for unprivileged users. FIXES 135062 Reviewed-on: https://gerrit.openafs.org/15907 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 0358648dbed7656e7bda30f6f0ea6e8e01bf6527) Reviewed-on: https://gerrit.openafs.org/15928 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 57b655e4837d8660ebcc25d95efb09118adaff07) [1.6: In this branch, afs_pag_wait() cannot return an error, and afs_setpag() assumes it will not return an error.] Change-Id: I1d678f24ded708200f7d14061f9a874df7fbbb2c --- src/afs/afs_osi_pag.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/afs/afs_osi_pag.c b/src/afs/afs_osi_pag.c index a57be5e0c2..2744b51d68 100644 --- a/src/afs/afs_osi_pag.c +++ b/src/afs/afs_osi_pag.c @@ -189,6 +189,7 @@ afs_pag_wait(afs_ucred_t **acred) afs_int32 afs_genpag(afs_ucred_t **acred, afs_uint32 *apag) { + afs_pag_wait(acred); *apag = genpagval(); return 0; } @@ -223,8 +224,6 @@ afs_setpag(void) AFS_STATCNT(afs_setpag); - afs_pag_wait(acred); - afs_genpag(acred, &pag); #if defined(AFS_SUN5_ENV) -- 2.45.2