From 08f8b6c581d6feebedfba4556cb0b4ff573900e2 Mon Sep 17 00:00:00 2001 From: Michael Meffie Date: Fri, 21 May 2021 12:38:01 -0400 Subject: [PATCH 01/11] libadmin: Let xdr allocate rpc output strings In most functions, the libadmin library provides fixed sized buffers for RPC output strings instead of letting xdr allocate the output string. Unfortunately the fixed sized buffers do not account for the terminating nul char when the output string is the maximum length possible for the xdr output strings. To avoid potential buffer overflows, and to allow for larger xdr string sizes in the future, convert these to xdr allocated strings and use safe string functions to copy the results to the application buffers. Fail with an error if the application buffer is too small, instead of overflowing the buffer or truncating results. Thanks to Cheyenne Wills for pointing out this issue. Reviewed-on: https://gerrit.openafs.org/14626 Reviewed-by: Benjamin Kaduk Tested-by: BuildBot (cherry picked from commit 0d6b9defb36cb94f3d34b058f00055e9e99d85fc) Reviewed-on: https://gerrit.openafs.org/14664 Tested-by: BuildBot Reviewed-by: Michael Meffie Reviewed-by: Cheyenne Wills Reviewed-by: Stephan Wiesand (cherry picked from commit 00423e6920fba66ce4ce7ab746210825830aae5b) [1.6: We don't have strlcpy() available here, since that is defined in afsutil in 1.6. To avoid needing to change what libraries we link against or introduce other issues, use a locally-defined function admin_strlcpy() instead of the normal strlcpy().] Change-Id: I8d3a58d5d4132b5e72de86fcfd4378db52d088a2 --- src/libadmin/adminutil/afs_AdminUtilErrors.et | 1 + src/libadmin/adminutil/afs_utilAdmin.c | 78 +++++++++- src/libadmin/bos/afs_bosAdmin.c | 146 +++++++++++++++--- 3 files changed, 198 insertions(+), 27 deletions(-) diff --git a/src/libadmin/adminutil/afs_AdminUtilErrors.et b/src/libadmin/adminutil/afs_AdminUtilErrors.et index e60c2e5ea4..d031638117 100644 --- a/src/libadmin/adminutil/afs_AdminUtilErrors.et +++ b/src/libadmin/adminutil/afs_AdminUtilErrors.et @@ -22,4 +22,5 @@ error_table AU ec ADMRXDEBUGHANDLENULL, "the rxdebug handle parameter cannot be null" ec ADMRXDEBUGVERSIONNULL, "the rxdebug version parameter cannot be null" ec ADMRXDEBUGSTATSNULL, "the rxdebug stats parameter cannot be null" + ec ADMRPCTOOBIG, "the rpc output data exceeds buffer size." end diff --git a/src/libadmin/adminutil/afs_utilAdmin.c b/src/libadmin/adminutil/afs_utilAdmin.c index c4f2f5412a..16e27d8c08 100644 --- a/src/libadmin/adminutil/afs_utilAdmin.c +++ b/src/libadmin/adminutil/afs_utilAdmin.c @@ -42,6 +42,58 @@ #endif #include "afs_utilAdmin.h" +/* + * The 'admin_strlcpy' function is copied from heimdal, under the following + * license: + * + * Copyright (c) 1995-2002 Kungliga Tekniska Högskolan + * (Royal Institute of Technology, Stockholm, Sweden). + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * 3. Neither the name of the Institute nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE INSTITUTE AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE INSTITUTE OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ +static size_t +admin_strlcpy(char *dst, const char *src, size_t dst_sz) +{ + size_t n; + + for (n = 0; n < dst_sz; n++) { + if ((*dst++ = *src++) == '\0') + break; + } + + if (n < dst_sz) + return n; + if (n > 0) + *(dst - 1) = '\0'; + return n + strlen (src); +} + /* * AIX 4.2 has PTHREAD_CREATE_UNDETACHED and not PTHREAD_CREATE_JOINABLE * @@ -2007,14 +2059,14 @@ ListCellsRPC(void *rpc_specific, int slot, int *last_item, int rc = 0; afs_status_t tst = 0; cm_list_cell_get_p t = (cm_list_cell_get_p) rpc_specific; - char *name; + char *name = NULL; + size_t len; serverList sl; unsigned int n; /* * Get the next entry in the CellServDB. */ - name = t->cell[slot].cellname; sl.serverList_len = 0; sl.serverList_val = NULL; memset(t->cell[slot].serverAddr, 0, sizeof(afs_int32)*UTIL_MAX_CELL_HOSTS); @@ -2023,7 +2075,11 @@ ListCellsRPC(void *rpc_specific, int slot, int *last_item, if (tst) { goto fail_ListCellsRPC; } - strcpy(t->cell[slot].cellname, name); + len = admin_strlcpy(t->cell[slot].cellname, name, sizeof(t->cell[slot].cellname)); + if (len >= sizeof(t->cell[slot].cellname)) { + tst = ADMRPCTOOBIG; + goto fail_ListCellsRPC; + } if (sl.serverList_val) { for (n=0; ncell[slot].serverAddr[n] = sl.serverList_val[n]; @@ -2043,6 +2099,7 @@ ListCellsRPC(void *rpc_specific, int slot, int *last_item, rc = 1; fail_ListCellsRPC: + xdr_free((xdrproc_t) xdr_string, &name); if (st != NULL) { *st = tst; @@ -2240,7 +2297,8 @@ util_CMLocalCell(struct rx_connection *conn, afs_CMCellName_p cellName, { int rc = 0; afs_status_t tst = 0; - afs_CMCellName_p name; + char *name = NULL; + size_t len; if (conn == NULL) { tst = ADMRXCONNNULL; @@ -2252,15 +2310,21 @@ util_CMLocalCell(struct rx_connection *conn, afs_CMCellName_p cellName, goto fail_util_CMLocalCell; } - name = cellName; tst = RXAFSCB_GetLocalCell(conn, &name); + if (tst != 0) { + goto fail_util_CMLocalCell; + } - if (!tst) { - rc = 1; + len = admin_strlcpy(cellName, name, sizeof(cellName)); + if (len >= sizeof(cellName)) { + tst = ADMRPCTOOBIG; + goto fail_util_CMLocalCell; } + rc = 1; fail_util_CMLocalCell: + xdr_free((xdrproc_t)xdr_string, &name); if (st != NULL) { *st = tst; } diff --git a/src/libadmin/bos/afs_bosAdmin.c b/src/libadmin/bos/afs_bosAdmin.c index df53cf1660..b1d221c982 100644 --- a/src/libadmin/bos/afs_bosAdmin.c +++ b/src/libadmin/bos/afs_bosAdmin.c @@ -34,6 +34,58 @@ #endif #include +/* + * The 'admin_strlcpy' function is copied from heimdal, under the following + * license: + * + * Copyright (c) 1995-2002 Kungliga Tekniska Högskolan + * (Royal Institute of Technology, Stockholm, Sweden). + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * 3. Neither the name of the Institute nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE INSTITUTE AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE INSTITUTE OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ +static size_t +admin_strlcpy(char *dst, const char *src, size_t dst_sz) +{ + size_t n; + + for (n = 0; n < dst_sz; n++) { + if ((*dst++ = *src++) == '\0') + break; + } + + if (n < dst_sz) + return n; + if (n > 0) + *(dst - 1) = '\0'; + return n + strlen (src); +} + typedef struct bos_server { int begin_magic; int is_valid; @@ -535,6 +587,8 @@ bos_ProcessExecutionStateGet(const void *serverHandle, afs_status_t tst = 0; bos_server_p b_handle = (bos_server_p) serverHandle; afs_int32 state; + char *desc = NULL; + size_t len; if (!isValidServerHandle(b_handle, &tst)) { goto fail_bos_ProcessExecutionStateGet; @@ -556,18 +610,24 @@ bos_ProcessExecutionStateGet(const void *serverHandle, } tst = - BOZO_GetStatus(b_handle->server, processName, &state, - &auxiliaryProcessStatus); - + BOZO_GetStatus(b_handle->server, processName, &state, &desc); if (tst != 0) { goto fail_bos_ProcessExecutionStateGet; } + /* This function assumes the caller provides a BOS_MAX_NAME_LEN sized buffer. */ + len = admin_strlcpy(auxiliaryProcessStatus, desc, BOS_MAX_NAME_LEN); + if (len >= BOS_MAX_NAME_LEN) { + tst = ADMRPCTOOBIG; + goto fail_bos_ProcessExecutionStateGet; + } *processStatusP = (bos_ProcessExecutionState_t) state; + rc = 1; fail_bos_ProcessExecutionStateGet: + xdr_free((xdrproc_t)xdr_string, &desc); if (st != NULL) { *st = tst; } @@ -716,11 +776,17 @@ GetProcessNameRPC(void *rpc_specific, int slot, int *last_item, int rc = 0; afs_status_t tst = 0; process_name_get_p proc = (process_name_get_p) rpc_specific; - char *ptr = (char *)&proc->process[slot]; + char *name = NULL; + size_t len; - tst = BOZO_EnumerateInstance(proc->server, proc->next++, &ptr); + tst = BOZO_EnumerateInstance(proc->server, proc->next++, &name); if (tst == 0) { + len = admin_strlcpy(proc->process[slot], name, sizeof(proc->process[slot])); + if (len >= sizeof(proc->process[slot])) { + tst = ADMRPCTOOBIG; + goto fail_GetProcessNameRPC; + } rc = 1; } else if (tst == BZDOM) { tst = 0; @@ -729,6 +795,9 @@ GetProcessNameRPC(void *rpc_specific, int slot, int *last_item, *last_item_contains_data = 0; } + fail_GetProcessNameRPC: + + xdr_free((xdrproc_t)xdr_string, &name); if (st != NULL) { *st = tst; } @@ -946,8 +1015,7 @@ bos_ProcessInfoGet(const void *serverHandle, char *processName, int rc = 0; afs_status_t tst = 0; bos_server_p b_handle = (bos_server_p) serverHandle; - char type[BOS_MAX_NAME_LEN]; - char *ptr = type; + char *type = NULL; struct bozo_status status; int i; @@ -970,13 +1038,12 @@ bos_ProcessInfoGet(const void *serverHandle, char *processName, goto fail_bos_ProcessInfoGet; } - tst = BOZO_GetInstanceInfo(b_handle->server, processName, &ptr, &status); + tst = BOZO_GetInstanceInfo(b_handle->server, processName, &type, &status); if (tst != 0) { goto fail_bos_ProcessInfoGet; } - for (i = 0; (processTypes[i] != NULL); i++) { if (!strcmp(processTypes[i], type)) { *processTypeP = (bos_ProcessType_t) i; @@ -1011,6 +1078,7 @@ bos_ProcessInfoGet(const void *serverHandle, char *processName, fail_bos_ProcessInfoGet: + xdr_free((xdrproc_t)xdr_string, &type); if (st != NULL) { *st = tst; } @@ -1035,13 +1103,19 @@ GetParameterRPC(void *rpc_specific, int slot, int *last_item, int rc = 0; afs_status_t tst = 0; param_get_p param = (param_get_p) rpc_specific; - char *ptr = (char *)¶m->param[slot]; + char *parm = NULL; + size_t len; tst = BOZO_GetInstanceParm(param->server, param->processName, param->next++, - &ptr); + &parm); if (tst == 0) { + len = admin_strlcpy(param->param[slot], parm, sizeof(param->param[slot])); + if (len >= sizeof(param->param[slot])) { + tst = ADMRPCTOOBIG; + goto fail_GetParameterRPC; + } rc = 1; } else if (tst == BZDOM) { tst = 0; @@ -1050,6 +1124,9 @@ GetParameterRPC(void *rpc_specific, int slot, int *last_item, *last_item_contains_data = 0; } + fail_GetParameterRPC: + + xdr_free((xdrproc_t)xdr_string, &parm); if (st != NULL) { *st = tst; } @@ -1703,23 +1780,32 @@ GetAdminRPC(void *rpc_specific, int slot, int *last_item, int rc = 0; afs_status_t tst = 0; admin_get_p admin = (admin_get_p) rpc_specific; - char *ptr = (char *)&admin->admin[slot]; + char *name = NULL; + size_t len; - tst = BOZO_ListSUsers(admin->server, admin->next++, &ptr); + tst = BOZO_ListSUsers(admin->server, admin->next++, &name); /* * There's no way to tell the difference between an rpc failure * and the end of the list, so we assume that any error means the * end of the list */ - - if (tst != 0) { + if (tst == 0) { + len = admin_strlcpy(admin->admin[slot], name, sizeof(admin->admin[slot])); + if (len >= sizeof(admin->admin[slot])) { + tst = ADMRPCTOOBIG; + goto fail_GetAdminRPC; + } + } else { tst = 0; *last_item = 1; *last_item_contains_data = 0; } rc = 1; + fail_GetAdminRPC: + + xdr_free((xdrproc_t)xdr_string, &name); if (st != NULL) { *st = tst; } @@ -2291,6 +2377,8 @@ bos_CellGet(const void *serverHandle, char *cellName, afs_status_p st) int rc = 0; afs_status_t tst = 0; bos_server_p b_handle = (bos_server_p) serverHandle; + char *tcellname = NULL; + size_t len; if (!isValidServerHandle(b_handle, &tst)) { goto fail_bos_CellGet; @@ -2301,14 +2389,23 @@ bos_CellGet(const void *serverHandle, char *cellName, afs_status_p st) goto fail_bos_CellGet; } - tst = BOZO_GetCellName(b_handle->server, &cellName); + tst = BOZO_GetCellName(b_handle->server, &tcellname); + if (tst != 0) { + goto fail_bos_CellGet; + } - if (tst == 0) { - rc = 1; + /* This function assumes the caller provides a BOS_MAX_NAME_LEN sized buffer. */ + len = admin_strlcpy(cellName, tcellname, BOS_MAX_NAME_LEN); + if (len >= BOS_MAX_NAME_LEN) { + tst = ADMRPCTOOBIG; + goto fail_bos_CellGet; } + rc = 1; + fail_bos_CellGet: + xdr_free((xdrproc_t)xdr_string, &tcellname); if (st != NULL) { *st = tst; } @@ -2430,11 +2527,17 @@ GetHostRPC(void *rpc_specific, int slot, int *last_item, int rc = 0; afs_status_t tst = 0; host_get_p host = (host_get_p) rpc_specific; - char *ptr = (char *)&host->host[slot]; + char *name = NULL; + size_t len; - tst = BOZO_GetCellHost(host->server, host->next++, &ptr); + tst = BOZO_GetCellHost(host->server, host->next++, &name); if (tst == 0) { + len = admin_strlcpy(host->host[slot], name, sizeof(host->host[slot])); + if (len >= sizeof(host->host[slot])) { + tst = ADMRPCTOOBIG; + goto fail_GetHostRPC; + } rc = 1; } else if (tst == BZDOM) { tst = 0; @@ -2443,6 +2546,9 @@ GetHostRPC(void *rpc_specific, int slot, int *last_item, *last_item_contains_data = 0; } + fail_GetHostRPC: + + xdr_free((xdrproc_t)xdr_string, &name); if (st != NULL) { *st = tst; } -- 2.45.2 From e8f28a7c6d6aca2ed2376d23984a80707b8e77a7 Mon Sep 17 00:00:00 2001 From: Michael Meffie Date: Tue, 22 Jun 2021 20:02:18 -0400 Subject: [PATCH 02/11] bos: Let xdr allocate rpc output strings The bos client provides fixed sized buffers on the stack for RPC output strings instead of letting xdr allocate output strings. Unfortunately, the fixed sized buffers do not account for the terminating nul char when the output string is the maximum size defined for the bozo RPCs. To avoid potential buffer overflows, and to allow for larger xdr string sizes in the future, convert these to xdr allocated strings. Be sure to always free the xdr allocated strings. The following bozo RPCs have xdr output strings, and are addressed in this commit: BOZO_EnumerateInstance BOZO_GetCellHost BOZO_GetCellName BOZO_GetInstanceInfo BOZO_GetInstanceParm BOZO_GetInstanceStrings BOZO_GetStatus BOZO_ListSUsers Thanks to Cheyenne Wills for pointing out this issue. Reviewed-on: https://gerrit.openafs.org/14650 Tested-by: BuildBot Reviewed-by: Michael Meffie Reviewed-by: Benjamin Kaduk (cherry picked from commit 9ae5b599c7289a6f3ea2b03f2646402da182bb5d) Reviewed-on: https://gerrit.openafs.org/14666 Tested-by: BuildBot Reviewed-by: Michael Meffie Reviewed-by: Cheyenne Wills Reviewed-by: Stephan Wiesand (cherry picked from commit 5abea9b8b1164f203fe18b5abe7d64ac8cb514eb) Change-Id: Ie6666b942515b8b90796102f22ef2118dac91386 --- src/bozo/bos.c | 127 +++++++++++++++++++++++++++---------------------- 1 file changed, 71 insertions(+), 56 deletions(-) diff --git a/src/bozo/bos.c b/src/bozo/bos.c index 8d0cb6869e..d0c90ed331 100644 --- a/src/bozo/bos.c +++ b/src/bozo/bos.c @@ -380,13 +380,12 @@ UnInstall(struct cmd_syndesc *as, void *arock) static afs_int32 GetServerGoal(struct rx_connection *aconn, char *aname) { - char buffer[500]; - char *tp; + char *itype = NULL; afs_int32 code; struct bozo_status istatus; - tp = buffer; - code = BOZO_GetInstanceInfo(aconn, aname, &tp, &istatus); + code = BOZO_GetInstanceInfo(aconn, aname, &itype, &istatus); + xdr_free((xdrproc_t)xdr_string, &itype); if (code) { printf("bos: failed to get instance info for '%s' (%s)\n", aname, em(code)); @@ -730,28 +729,31 @@ ListHosts(struct cmd_syndesc *as, void *arock) { struct rx_connection *tconn; afs_int32 code; - char tbuffer[256]; - char *tp; + char *cellname = NULL; + char *hostname = NULL; afs_int32 i; - tp = tbuffer; tconn = GetConn(as, 0); - code = BOZO_GetCellName(tconn, &tp); + code = BOZO_GetCellName(tconn, &cellname); if (code) { printf("bos: failed to get cell name (%s)\n", em(code)); exit(1); } - printf("Cell name is %s\n", tbuffer); + printf("Cell name is %s\n", cellname); for (i = 0;; i++) { - code = BOZO_GetCellHost(tconn, i, &tp); + xdr_free((xdrproc_t)xdr_string, &hostname); + code = BOZO_GetCellHost(tconn, i, &hostname); if (code == BZDOM) break; if (code != 0) { printf("bos: failed to get cell host %d (%s)\n", i, em(code)); exit(1); } - printf(" Host %d is %s\n", i + 1, tbuffer); + printf(" Host %d is %s\n", i + 1, hostname); } + + xdr_free((xdrproc_t)xdr_string, &cellname); + xdr_free((xdrproc_t)xdr_string, &hostname); return 0; } @@ -943,23 +945,22 @@ ListSUsers(struct cmd_syndesc *as, void *arock) struct rx_connection *tconn; int i; afs_int32 code; - char tbuffer[256]; - char *tp; + char *name = NULL; int lastNL, printGreeting; tconn = GetConn(as, 0); lastNL = 0; printGreeting = 1; for (i = 0;; i++) { - tp = tbuffer; - code = BOZO_ListSUsers(tconn, i, &tp); + xdr_free((xdrproc_t)xdr_string, &name); + code = BOZO_ListSUsers(tconn, i, &name); if (code) break; if (printGreeting) { printGreeting = 0; /* delay until after first call succeeds */ printf("SUsers are: "); } - printf("%s ", tbuffer); + printf("%s ", name); if ((i % NPERLINE) == NPERLINE - 1) { printf("\n"); lastNL = 1; @@ -969,11 +970,15 @@ ListSUsers(struct cmd_syndesc *as, void *arock) if (code != 1) { /* a real error code, instead of scanned past end */ printf("bos: failed to retrieve super-user list (%s)\n", em(code)); - return code; + goto done; } if (lastNL == 0) printf("\n"); - return 0; + code = 0; + + done: + xdr_free((xdrproc_t)xdr_string, &name); + return code; } static int @@ -982,8 +987,7 @@ StatServer(struct cmd_syndesc *as, void *arock) struct rx_connection *tconn; afs_int32 code; int i; - char ibuffer[BOZO_BSSIZE]; - char *tp; + char *iname = NULL; int int32p; /* int32p==1 is obsolete, smaller, printout */ @@ -996,8 +1000,8 @@ StatServer(struct cmd_syndesc *as, void *arock) tconn = GetConn(as, 0); for (i = 0;; i++) { /* for each instance */ - tp = ibuffer; - code = BOZO_EnumerateInstance(tconn, i, &tp); + xdr_free((xdrproc_t)xdr_string, &iname); + code = BOZO_EnumerateInstance(tconn, i, &iname); if (code == BZDOM) break; if (code) { @@ -1005,8 +1009,10 @@ StatServer(struct cmd_syndesc *as, void *arock) em(code)); break; } - DoStat(ibuffer, tconn, int32p, (i == 0)); /* print status line */ + DoStat(iname, tconn, int32p, (i == 0)); /* print status line */ } + + xdr_free((xdrproc_t)xdr_string, &iname); return 0; } @@ -1300,9 +1306,10 @@ DoSalvage(struct rx_connection * aconn, char * aparm1, char * aparm2, /* now wait for bnode to disappear */ count = 0; while (1) { + char *itype = NULL; IOMGR_Sleep(1); - tp = tbuffer; - code = BOZO_GetInstanceInfo(aconn, "salvage-tmp", &tp, &istatus); + code = BOZO_GetInstanceInfo(aconn, "salvage-tmp", &itype, &istatus); + xdr_free((xdrproc_t)xdr_string, &itype); if (code) break; if ((count++ % 5) == 0) @@ -1386,14 +1393,12 @@ GetLogCmd(struct cmd_syndesc *as, void *arock) static int IsDAFS(struct rx_connection *aconn) { - char buffer[BOZO_BSSIZE]; - char *tp; + char *itype = NULL; struct bozo_status istatus; afs_int32 code; - tp = &buffer[0]; - - code = BOZO_GetInstanceInfo(aconn, "dafs", &tp, &istatus); + code = BOZO_GetInstanceInfo(aconn, "dafs", &itype, &istatus); + xdr_free((xdrproc_t)xdr_string, &itype); if (code) { /* no dafs bnode; cannot be dafs */ return 0; @@ -1724,26 +1729,30 @@ DoStat(IN char *aname, IN int firstTime) /* true iff first instance in cmd */ { afs_int32 temp; - char buffer[500]; afs_int32 code; afs_int32 i; struct bozo_status istatus; - char *tp; - char *is1, *is2, *is3, *is4; /* instance strings */ - - tp = buffer; - code = BOZO_GetInstanceInfo(aconn, aname, &tp, &istatus); + char *itype = NULL; + char *is1 = NULL; + char *is2 = NULL; + char *is3 = NULL; + char *is4 = NULL; + char *desc = NULL; + char *parm = NULL; + char *notifier_parm = NULL; + + code = BOZO_GetInstanceInfo(aconn, aname, &itype, &istatus); if (code) { printf("bos: failed to get instance info for '%s' (%s)\n", aname, em(code)); - return -1; + goto done; } if (firstTime && aint32p && (istatus.flags & BOZO_BADDIRACCESS)) printf ("Bosserver reports inappropriate access on server directories\n"); printf("Instance %s, ", aname); if (aint32p) - printf("(type is %s) ", buffer); + printf("(type is %s) ", itype); if (istatus.fileGoal == istatus.goal) { if (!istatus.goal) printf("disabled, "); @@ -1759,8 +1768,7 @@ DoStat(IN char *aname, if (istatus.flags & BOZO_HASCORE) printf("has core file, "); - tp = buffer; - code = BOZO_GetStatus(aconn, aname, &temp, &tp); + code = BOZO_GetStatus(aconn, aname, &temp, &desc); if (code) printf("bos: failed to get status for instance '%s' (%s)\n", aname, em(code)); @@ -1774,14 +1782,16 @@ DoStat(IN char *aname, printf("starting up.\n"); else if (temp == BSTAT_SHUTTINGDOWN) printf("shutting down.\n"); - if (buffer[0] != 0) { - printf(" Auxiliary status is: %s.\n", buffer); + if (desc[0] != '\0') { + printf(" Auxiliary status is: %s.\n", desc); } } /* are we done yet? */ - if (!aint32p) - return 0; + if (!aint32p) { + code = 0; + goto done; + } if (istatus.procStartTime) printf(" Process last started at %s (%d proc starts)\n", @@ -1790,7 +1800,6 @@ DoStat(IN char *aname, printf(" Last exit at %s\n", DateOf(istatus.lastAnyExit)); } if (istatus.lastErrorExit) { - is1 = is2 = is3 = is4 = NULL; printf(" Last error exit at %s, ", DateOf(istatus.lastErrorExit)); code = BOZO_GetInstanceStrings(aconn, aname, &is1, &is2, &is3, &is4); /* don't complain about failing call, since could simply mean @@ -1801,10 +1810,6 @@ DoStat(IN char *aname, /* non-null instance string */ printf("by %s, ", is1); } - free(is1); - free(is2); - free(is3); - free(is4); } if (istatus.errorSignal) { if (istatus.errorSignal == SIGTERM) @@ -1818,21 +1823,31 @@ DoStat(IN char *aname, if (aint32p > 1) { /* try to display all the parms */ for (i = 0;; i++) { - tp = buffer; - code = BOZO_GetInstanceParm(aconn, aname, i, &tp); + xdr_free((xdrproc_t)xdr_string, &parm); + code = BOZO_GetInstanceParm(aconn, aname, i, &parm); if (code) break; - printf(" Command %d is '%s'\n", i + 1, buffer); + printf(" Command %d is '%s'\n", i + 1, parm); } - tp = buffer; - code = BOZO_GetInstanceParm(aconn, aname, 999, &tp); + code = BOZO_GetInstanceParm(aconn, aname, 999, ¬ifier_parm); if (!code) { /* Any type of failure is treated as not having a notifier program */ - printf(" Notifier is '%s'\n", buffer); + printf(" Notifier is '%s'\n", notifier_parm); } printf("\n"); } - return 0; + code = 0; + + done: + xdr_free((xdrproc_t)xdr_string, &itype); + xdr_free((xdrproc_t)xdr_string, &is1); + xdr_free((xdrproc_t)xdr_string, &is2); + xdr_free((xdrproc_t)xdr_string, &is3); + xdr_free((xdrproc_t)xdr_string, &is4); + xdr_free((xdrproc_t)xdr_string, &desc); + xdr_free((xdrproc_t)xdr_string, &parm); + xdr_free((xdrproc_t)xdr_string, ¬ifier_parm); + return code; } static int -- 2.45.2 From 2ecbbe7f5bbcda1ec560041aef6b7a8ae8ce6035 Mon Sep 17 00:00:00 2001 From: Michael Meffie Date: Fri, 10 Mar 2023 17:51:17 -0500 Subject: [PATCH 03/11] xdr: Avoid xdr_string maxsize check when freeing The maxsize argument in xdr_string() is garbage when called by xdr_free(), since xdr_free() only passes the XDR handle and the xdr string to be freed. Sometimes the size check fails and xdr_string() returns early, without freeing the string and without setting the object pointer to NULL. Usually this just results in leaking the string's memory. But since commit 9ae5b599c7 (bos: Let xdr allocate rpc output strings), many callers in bos.c rely on xdr_free(xdr_string) to set the given string to NULL; if this doesn't happen, subsequent calls to BOZO_ RPCs can corrupt memory, often causing the 'bos' process to segfault. We only need the maxsize check when encoding or decoding, so avoid accessing the maxsize agument when the op mode is XDR_FREE. In general, xdr_free() can only safely be used on xdr 2-argument xdr functions, so must be avoided when freeing xdr opaque, byte, and union types. This change makes it safe to use xdr_free() to free xdr strings, but in the future, we should provide a typesafe and less fragile function for freeing xdr strings returned from RPCs. Currently, xdr_free(xdr_string) is only called by the bos client and the tests. Reviewed-on: https://gerrit.openafs.org/15343 Tested-by: BuildBot Reviewed-by: Benjamin Kaduk (cherry picked from commit bbb1e8adfed6804ac6fbae0a073dc6927096e16a) Reviewed-on: https://gerrit.openafs.org/15939 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit fec84e347768080e4370e5aeb05886bfe19ae54b) Change-Id: I9ed51d9dc7956bc25ed2bfe01cb3010d11d2b152 --- src/rx/xdr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rx/xdr.c b/src/rx/xdr.c index bd224a1631..94a3211ebe 100644 --- a/src/rx/xdr.c +++ b/src/rx/xdr.c @@ -537,7 +537,7 @@ xdr_string(XDR * xdrs, char **cpp, u_int maxsize) if (!xdr_u_int(xdrs, &size)) { return (FALSE); } - if (size > maxsize) { + if (xdrs->x_op != XDR_FREE && size > maxsize) { return (FALSE); } nodesize = size + 1; -- 2.45.2 From 9a36cf913ded91eeeb389e4a25db6a8724d8183d Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 13 Jun 2024 15:28:38 -0500 Subject: [PATCH 04/11] OPENAFS-SA-2024-003: xdr: Avoid prealloc'd string OUT args CVE-2024-10397 Currently, several callers call RPCs with string OUT arguments, and provide preallocated memory for those arguments. This can easily allow a response from the server to overrun the allocated buffer, stomping over stack or heap memory. We could simply make our preallocated buffers larger than the maximum size that the RPC allows, but relying on that is error prone, and there's no way for XDR to check if a string buffer is large enough. Instead, to make sure we don't overrun a given preallocated buffer, avoid giving a preallocated buffer to such RPCs, and let XDR allocate the memory for us. Specifically, this commit changes several callers to RXAFS_GetVolumeStatus(), and one caller of BOZO_GetInstanceParm(), to avoid passing in a preallocated string buffer. All other callers of RPCs with string OUT args already let XDR allocate the buffers for them. FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15918 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 00a1b266af51a828a022c23e7bb006a39740eaad) Reviewed-on: https://gerrit.openafs.org/15940 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 40440c3eb628ff1772588bdc99d7496292097bbd) [1.6: In afs_bosAdmin.c, use the locally-defined admin_strlcpy(), since we do not have the normal strlcpy() available here.] Change-Id: Ib861a35e86322d33e06dfc93b3c8deed945ed0a2 --- src/WINNT/afsd/cm_ioctl.c | 27 +++++++++++++++++++++------ src/WINNT/afsd/cm_volume.c | 19 +++++++++---------- src/afs/afs_pioctl.c | 20 ++++++++++---------- src/libadmin/bos/afs_bosAdmin.c | 16 +++++++++++++--- 4 files changed, 53 insertions(+), 29 deletions(-) diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c index 0f524ffe0d..2f5512210a 100644 --- a/src/WINNT/afsd/cm_ioctl.c +++ b/src/WINNT/afsd/cm_ioctl.c @@ -742,9 +742,6 @@ cm_IoctlGetVolumeStatus(struct cm_ioctl *ioctlp, struct cm_user *userp, cm_scach cm_conn_t *connp; AFSFetchVolumeStatus volStat; char *cp; - char *Name; - char *OfflineMsg; - char *MOTD; struct rx_connection * rxconnp; #ifdef AFS_FREELANCE_CLIENT @@ -761,13 +758,18 @@ cm_IoctlGetVolumeStatus(struct cm_ioctl *ioctlp, struct cm_user *userp, cm_scach } else #endif { - Name = volName; - OfflineMsg = offLineMsg; - MOTD = motd; + char *Name = NULL; + char *OfflineMsg = NULL; + char *MOTD = NULL; + do { code = cm_ConnFromFID(&scp->fid, userp, reqp, &connp); if (code) continue; + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); + rxconnp = cm_GetRxConn(connp); code = RXAFS_GetVolumeStatus(rxconnp, scp->fid.volume, &volStat, &Name, &OfflineMsg, &MOTD); @@ -775,6 +777,19 @@ cm_IoctlGetVolumeStatus(struct cm_ioctl *ioctlp, struct cm_user *userp, cm_scach } while (cm_Analyze(connp, userp, reqp, &scp->fid, 0, NULL, NULL, NULL, code)); code = cm_MapRPCError(code, reqp); + + strncpy(volName, Name, sizeof(volName)); + volName[sizeof(volName) - 1] = '\0'; + + strncpy(offLineMsg, OfflineMsg, sizeof(offLineMsg)); + offLineMsg[sizeof(offLineMsg) - 1] = '\0'; + + strncpy(motd, MOTD, sizeof(motd)); + motd[sizeof(motd) - 1] = '\0'; + + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); } if (code) diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c index 62b30284ea..5b5eb8e845 100644 --- a/src/WINNT/afsd/cm_volume.c +++ b/src/WINNT/afsd/cm_volume.c @@ -1228,22 +1228,12 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 cm_conn_t *connp; long code; AFSFetchVolumeStatus volStat; - char *Name; - char *OfflineMsg; - char *MOTD; cm_req_t req; struct rx_connection * rxconnp; - char volName[32]; - char offLineMsg[256]; - char motd[256]; long alldown, alldeleted; cm_serverRef_t *serversp; cm_fid_t fid; - Name = volName; - OfflineMsg = offLineMsg; - MOTD = motd; - if (statep->ID != 0 && (!volID || volID == statep->ID)) { /* create fid for volume root so that VNOVOL and VMOVED errors can be processed */ cm_SetFid(&fid, volp->cellp->cellID, statep->ID, 1, 1); @@ -1285,6 +1275,10 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 lock_ReleaseWrite(&volp->rw); do { + char *Name = NULL; + char *OfflineMsg = NULL; + char *MOTD = NULL; + code = cm_ConnFromVolume(volp, statep->ID, cm_rootUserp, &req, &connp); if (code) continue; @@ -1293,6 +1287,11 @@ cm_CheckOfflineVolumeState(cm_volume_t *volp, cm_vol_state_t *statep, afs_uint32 code = RXAFS_GetVolumeStatus(rxconnp, statep->ID, &volStat, &Name, &OfflineMsg, &MOTD); rx_PutConnection(rxconnp); + + xdr_free((xdrproc_t) xdr_string, &Name); + xdr_free((xdrproc_t) xdr_string, &OfflineMsg); + xdr_free((xdrproc_t) xdr_string, &MOTD); + } while (cm_Analyze(connp, cm_rootUserp, &req, &fid, 0, NULL, NULL, NULL, code)); code = cm_MapRPCError(code, &req); diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index f24b03b07e..dffece3466 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1971,32 +1971,31 @@ DECL_PIOCTL(PSetTokens) */ DECL_PIOCTL(PGetVolumeStatus) { - char volName[32]; - char *offLineMsg = afs_osi_Alloc(256); - char *motd = afs_osi_Alloc(256); + char *volName = NULL; + char *offLineMsg = NULL; + char *motd = NULL; struct afs_conn *tc; afs_int32 code = 0; struct AFSFetchVolumeStatus volstat; - char *Name; struct rx_connection *rxconn; XSTATS_DECLS; - osi_Assert(offLineMsg != NULL); - osi_Assert(motd != NULL); AFS_STATCNT(PGetVolumeStatus); if (!avc) { code = EINVAL; goto out; } - Name = volName; do { tc = afs_Conn(&avc->f.fid, areq, SHARED_LOCK, &rxconn); if (tc) { XSTATS_START_TIME(AFS_STATS_FS_RPCIDX_GETVOLUMESTATUS); RX_AFS_GUNLOCK(); + xdr_free((xdrproc_t) xdr_string, &volName); + xdr_free((xdrproc_t) xdr_string, &offLineMsg); + xdr_free((xdrproc_t) xdr_string, &motd); code = RXAFS_GetVolumeStatus(rxconn, avc->f.fid.Fid.Volume, &volstat, - &Name, &offLineMsg, &motd); + &volName, &offLineMsg, &motd); RX_AFS_GLOCK(); XSTATS_END_TIME; } else @@ -2017,8 +2016,9 @@ DECL_PIOCTL(PGetVolumeStatus) if (afs_pd_putString(aout, motd) != 0) return E2BIG; out: - afs_osi_Free(offLineMsg, 256); - afs_osi_Free(motd, 256); + xdr_free((xdrproc_t) xdr_string, &volName); + xdr_free((xdrproc_t) xdr_string, &offLineMsg); + xdr_free((xdrproc_t) xdr_string, &motd); return code; } diff --git a/src/libadmin/bos/afs_bosAdmin.c b/src/libadmin/bos/afs_bosAdmin.c index b1d221c982..de37b82b6a 100644 --- a/src/libadmin/bos/afs_bosAdmin.c +++ b/src/libadmin/bos/afs_bosAdmin.c @@ -1353,6 +1353,8 @@ bos_ProcessNotifierGet(const void *serverHandle, const char *processName, int rc = 0; afs_status_t tst = 0; bos_server_p b_handle = (bos_server_p) serverHandle; + char *tnotif = NULL; + size_t len; if (!isValidServerHandle(b_handle, &tst)) { goto fail_bos_ProcessNotifierGet; @@ -1369,14 +1371,22 @@ bos_ProcessNotifierGet(const void *serverHandle, const char *processName, } tst = BOZO_GetInstanceParm(b_handle->server, (char *)processName, - 999, ¬ifier); + 999, &tnotif); + if (tst != 0) { + goto fail_bos_ProcessNotifierGet; + } - if (tst == 0) { - rc = 1; + len = admin_strlcpy(notifier, tnotif, BOS_MAX_NAME_LEN); + if (len >= BOS_MAX_NAME_LEN) { + tst = ADMRPCTOOBIG; + goto fail_bos_ProcessNotifierGet; } + rc = 1; + fail_bos_ProcessNotifierGet: + xdr_free((xdrproc_t)xdr_string, &tnotif); if (st != NULL) { *st = tst; } -- 2.45.2 From 42730cd42b18875ded900f604c627b0fdd1fec70 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 13 Jun 2024 15:30:50 -0500 Subject: [PATCH 05/11] OPENAFS-SA-2024-003: xdr: Set _len for prealloc'd opaque/array OUT args CVE-2024-10397 Currently, a few RPCs with arrays or opaque OUT arguments are called with preallocated memory for the arg, but also provide a _len of 0 (or an uninitialized _len). This makes it impossible for the xdr routine to tell whether we have allocated enough space to actually hold the response from the server. To help this situation, either specify an appropriate _len for the preallocated value (cm_IoctlGetACL, fsprobe_LWP), or don't provide a preallocated buffer at all and let xdr allocate a buffer for us (PGetAcl). Note that this commit doesn't change xdr to actually check the value of the given _len; but now a future commit can do so without breaking callers. FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15919 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit b2b1110ddd9e19670dbc6a3217dc2a74af432f82) Reviewed-on: https://gerrit.openafs.org/15941 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit c18640c6b98b10cd6f78c63195ff822689cb5348) Change-Id: I1c781b5a0147456e2ef460cb321f4c2f762799ac --- src/WINNT/afsd/cm_ioctl.c | 2 +- src/afs/afs_pioctl.c | 8 ++++---- src/fsprobe/fsprobe.c | 1 + 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/WINNT/afsd/cm_ioctl.c b/src/WINNT/afsd/cm_ioctl.c index 2f5512210a..b11d6cdf43 100644 --- a/src/WINNT/afsd/cm_ioctl.c +++ b/src/WINNT/afsd/cm_ioctl.c @@ -413,7 +413,7 @@ cm_IoctlGetACL(cm_ioctl_t *ioctlp, cm_user_t *userp, cm_scache_t *scp, cm_req_t afid.Unique = scp->fid.unique; do { acl.AFSOpaque_val = ioctlp->outDatap; - acl.AFSOpaque_len = 0; + acl.AFSOpaque_len = SMB_IOCTL_MAXDATA - (ioctlp->outDatap - ioctlp->outAllocp); code = cm_ConnFromFID(&scp->fid, userp, reqp, &connp); if (code) continue; diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index dffece3466..37c52e982f 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1590,11 +1590,10 @@ DECL_PIOCTL(PGetAcl) return ERANGE; Fid.Vnode |= (ain->remaining << 30); } - acl.AFSOpaque_val = aout->ptr; + memset(&acl, 0, sizeof(acl)); do { tconn = afs_Conn(&avc->f.fid, areq, SHARED_LOCK, &rxconn); if (tconn) { - acl.AFSOpaque_val[0] = '\0'; XSTATS_START_TIME(AFS_STATS_FS_RPCIDX_FETCHACL); RX_AFS_GUNLOCK(); code = RXAFS_FetchACL(rxconn, &Fid, &acl, &OutStatus, &tsync); @@ -1608,7 +1607,7 @@ DECL_PIOCTL(PGetAcl) if (code == 0) { if (acl.AFSOpaque_len == 0) { - afs_pd_skip(aout, 1); /* leave the NULL */ + code = afs_pd_putBytes(aout, "\0", 1); /* leave a single NUL byte */ } else if (memchr(acl.AFSOpaque_val, '\0', acl.AFSOpaque_len) == NULL) { /* Do not return an unterminated ACL string. */ @@ -1619,9 +1618,10 @@ DECL_PIOCTL(PGetAcl) code = EINVAL; } else { - afs_pd_skip(aout, acl.AFSOpaque_len); /* Length of the ACL */ + code = afs_pd_putBytes(aout, acl.AFSOpaque_val, acl.AFSOpaque_len); } } + xdr_free((xdrproc_t) xdr_AFSOpaque, &acl); return code; } diff --git a/src/fsprobe/fsprobe.c b/src/fsprobe/fsprobe.c index f5008ac5be..88a47532ed 100644 --- a/src/fsprobe/fsprobe.c +++ b/src/fsprobe/fsprobe.c @@ -225,6 +225,7 @@ fsprobe_LWP(void *unused) struct ProbeViceStatistics *curr_stats; /*Current stats region */ int *curr_probeOK; /*Current probeOK field */ ViceStatistics64 stats64; /*Current stats region */ + stats64.ViceStatistics64_len = STATS64_VERSION; stats64.ViceStatistics64_val = (afs_uint64 *)malloc(STATS64_VERSION * sizeof(afs_uint64)); while (1) { /*Service loop */ -- 2.45.2 From 8b07563fe96e67ec012f6517476690c1a1f4b090 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Sun, 4 Oct 2020 23:04:06 -0500 Subject: [PATCH 06/11] OPENAFS-SA-2024-003: xdr: Prevent XDR_DECODE buffer overruns CVE-2024-10397 When making an RPC call from a client, output arguments that use arrays (or array-like objects like strings and opaques) can be allocated by XDR, like so: { struct idlist ids; ids.idlist_val = NULL; ids.idlist_len = 0; code = PR_NameToID(rxconn, names, &ids); /* data inside ids.idlist_val[...] */ xdr_free((xdrproc_t) xdr_idlist, &ids); } With this approach, during XDR_DECODE, xdr_array() reads in the number of array elements from the peer, then allocates enough memory to hold that many elements, and then reads in the array elements. Alternatively, the caller can provide preallocated memory, like so: { struct idlist ids; afs_int32 ids_buf[30]; ids.idlist_val = ids_buf; ids.idlist_len = 30; code = PR_NameToID(rxconn, names, &ids); /* data inside ids.idlist_val[...] */ } With this approach, during XDR_DECODE, xdr_array() reads in the number of array elements from the peer, and then reads in the array elements into the supplied buffer. However, in this case, xdr_array() never checks that the number of array elements will actually fit into the supplied buffer; the _len field provided by the caller is just ignored. In this example, if the ptserver responds with 50 elements for the 'ids' output argument, xdr_array() will write 50 afs_int32's into 'ids.idlist_val', going beyond the end of the 30 elements that are actually allocated. It's also possible, and in fact very easy, to use xdr-allocated buffers and then reuse them as a preallocated buffer, possibly accidentally. For example: { struct idlist ids; ids.idlist_val = NULL; ids.idlist_len = 0; while (some_condition) { code = PR_NameToID(rxconn, names, &ids); } } In this case, the first call to PR_NameToID can cause the buffer for 'ids' to be allocated by XDR, which will then be reused by the subsequent calls to PR_NameToId. Note that this can happen even if the first PR_NameToID call fails; the call can be aborted after the output array is allocated. Retrying an RPC in this way is effectively what all ubik_Call* codepaths do (including all ubik_* wrappers, e.g. ubik_PR_NameToID). Or some callers retry effectively the same RPC when falling back to earlier versions (e.g. VL_ListAttributesN2 -> VL_ListAttributesN). To prevent this for arrays and opaques, change xdr_array (and xdr_bytes) to check if the _len field for preallocated buffers is large enough, and return failure if it's not. Also perform the same check for the ka_CBS and ka_BBS structures. These are mostly the same as opaques, but they have custom serialization functions in src/kauth/kaaux.c. ka_BBS also has two lengths: the actual length of bytes, and a 'max' length. ka_CBS isn't used for any RPC output arguments, but fix it for consistency. For strings, the situation is complicated by the fact that callers cannot pass in how much space was allocated for the string, since callers only provide a char**. So for strings, just refuse to use a preallocated buffer at all, and return failure if one is provided. Note that for some callers using preallocated arrays or strings, the described buffer overruns are not possible, since the preallocated buffers are larger than the max length specified in the relevant RPC-L. For example, afs_DoBulkStat() allocates AFSCBMAX entries for the output args for RXAFS_InlineBulkStatus, which is the max length specified in the RPC-L, so a buffer overrun is impossible. But since it is so easy to allow a buffer overrun, enforce the length checks for everyone. FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15920 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 13413eceed80d106cbed5ffb91c4dfbc8cccf55c) Reviewed-on: https://gerrit.openafs.org/15942 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit d253a52d3b59bd691eae8863ea2f06d99ad18550) Change-Id: Icd1e713ecbd5b957c9de54a1536b9a5480210cf5 --- src/kauth/kaaux.c | 33 +++++++++++++++++++++++++++++++-- src/rx/xdr.c | 36 +++++++++++++++++++++++++++++++----- src/rx/xdr_array.c | 24 +++++++++++++++++++++--- src/rx/xdr_arrayn.c | 24 +++++++++++++++++++++--- 4 files changed, 104 insertions(+), 13 deletions(-) diff --git a/src/kauth/kaaux.c b/src/kauth/kaaux.c index e6f013d7b6..cc42d93a71 100644 --- a/src/kauth/kaaux.c +++ b/src/kauth/kaaux.c @@ -33,7 +33,17 @@ xdr_ka_CBS(XDR * x, struct ka_CBS *abbs) xdr_afs_int32(x, &len); if (len < 0 || len > MAXBS) return FALSE; - if (!abbs->SeqBody) + if (abbs->SeqBody != NULL) { + if (len > abbs->SeqLen) { + /* + * We've been given a preallocated buffer to decode into, but + * we're decoding 'len' bytes, which is larger than the + * provided buffer (only abbs->SeqLen bytes large). This won't + * work. + */ + return FALSE; + } + } else abbs->SeqBody = (char *)malloc(len); abbs->SeqLen = len; xdr_opaque(x, abbs->SeqBody, len); @@ -60,7 +70,26 @@ xdr_ka_BBS(XDR * x, struct ka_BBS *abbs) if (!xdr_afs_int32(x, &maxLen) || !xdr_afs_int32(x, &len) || (len < 0) || (len > MAXBS) || (len > maxLen)) return FALSE; - if (!abbs->SeqBody) + if (abbs->SeqBody != NULL) { + if (len > abbs->MaxSeqLen) { + /* + * We've been given a preallocated buffer to decode into, but + * we're decoding 'len' bytes, which is larger than the + * provided buffer (only abbs->MaxSeqLen bytes large). This + * won't work. + */ + return FALSE; + } + if (maxLen > abbs->MaxSeqLen) { + /* + * Our preallocated buffer only has space for MaxSeqLen bytes. + * Don't let the peer change 'abbs' so that it looks like we + * have space for more bytes than that; that could cause us to + * access memory beyond what we've actually allocated. + */ + return FALSE; + } + } else abbs->SeqBody = (char *)malloc(maxLen); abbs->MaxSeqLen = maxLen; abbs->SeqLen = len; diff --git a/src/rx/xdr.c b/src/rx/xdr.c index 94a3211ebe..b05377301a 100644 --- a/src/rx/xdr.c +++ b/src/rx/xdr.c @@ -410,10 +410,28 @@ xdr_bytes(XDR * xdrs, char **cpp, u_int * sizep, /* * first deal with the length since xdr bytes are counted */ - if (!xdr_u_int(xdrs, sizep)) { - return (FALSE); + + if (xdrs->x_op == XDR_DECODE && sp != NULL) { + /* + * We've been given a preallocated array to decode into. Before we + * modify *sizep, check that we have enough space to fit the elements + * that follow. + */ + if (!xdr_u_int(xdrs, &nodesize)) { + return FALSE; + } + if (nodesize > *sizep) { + return FALSE; + } + *sizep = nodesize; + + } else { + if (!xdr_u_int(xdrs, sizep)) { + return (FALSE); + } + nodesize = *sizep; } - nodesize = *sizep; + if ((nodesize > maxsize) && (xdrs->x_op != XDR_FREE)) { return (FALSE); } @@ -548,8 +566,16 @@ xdr_string(XDR * xdrs, char **cpp, u_int maxsize) switch (xdrs->x_op) { case XDR_DECODE: - if (sp == NULL) - *cpp = sp = (char *)osi_alloc(nodesize); + if (sp != NULL) { + /* + * Refuse to use preallocated strings, since we cannot verify + * whether enough memory has been allocated to handle the decoded + * string. + */ + return FALSE; + } + + *cpp = sp = (char *)osi_alloc(nodesize); if (sp == NULL) { return (FALSE); } diff --git a/src/rx/xdr_array.c b/src/rx/xdr_array.c index 886d2c6e4f..8672d7e616 100644 --- a/src/rx/xdr_array.c +++ b/src/rx/xdr_array.c @@ -96,10 +96,28 @@ xdr_array(XDR * xdrs, caddr_t * addrp, u_int * sizep, u_int maxsize, maxsize = i; /* like strings, arrays are really counted arrays */ - if (!xdr_u_int(xdrs, sizep)) { - return (FALSE); + + if (xdrs->x_op == XDR_DECODE && target != NULL) { + /* + * We've been given a preallocated array to decode into. Before we + * modify *sizep, check that we have enough space to fit the elements + * that follow. + */ + if (!xdr_u_int(xdrs, &c)) { + return FALSE; + } + if (c > *sizep) { + return FALSE; + } + *sizep = c; + + } else { + if (!xdr_u_int(xdrs, sizep)) { + return (FALSE); + } + c = *sizep; } - c = *sizep; + if ((c > maxsize) && (xdrs->x_op != XDR_FREE)) { return (FALSE); } diff --git a/src/rx/xdr_arrayn.c b/src/rx/xdr_arrayn.c index 5b66598874..504075beba 100644 --- a/src/rx/xdr_arrayn.c +++ b/src/rx/xdr_arrayn.c @@ -97,10 +97,28 @@ xdr_arrayN(XDR * xdrs, caddr_t * addrp, u_int * sizep, u_int maxsize, maxsize = i; /* like strings, arrays are really counted arrays */ - if (!xdr_u_int(xdrs, sizep)) { - return (FALSE); + + if (xdrs->x_op == XDR_DECODE && target != NULL) { + /* + * We've been given a preallocated array to decode into. Before we + * modify *sizep, check that we have enough space to fit the elements + * that follow. + */ + if (!xdr_u_int(xdrs, &c)) { + return FALSE; + } + if (c > *sizep) { + return FALSE; + } + *sizep = c; + + } else { + if (!xdr_u_int(xdrs, sizep)) { + return (FALSE); + } + c = *sizep; } - c = *sizep; + if ((c > maxsize) && (xdrs->x_op != XDR_FREE)) { return (FALSE); } -- 2.45.2 From 42723c709cbcf2d08ced65134c91abbdee8cf22a Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 15 Oct 2020 23:18:53 -0500 Subject: [PATCH 07/11] OPENAFS-SA-2024-003: Check sanity on lengths of RPC returned arrays CVE-2024-10397 Various RPCs return a variable-length array in an OUT argument, but are only supposed to return specific sizes. A few instances of this include the following (but this is not an exhaustive list): - AFSVolListOneVolume should only return a single volintInfo. - PR_NameToID should return the same number of IDs as names given. - VL_GetAddrsU should return the same number of addresses as the 'nentries' OUT argument. Some callers of these RPCs just assume that the server has not violated these rules. If the server responds with a nonsensical array size, this could cause us to read beyond the end of the array, or cause a NULL dereference or other errors. For example, some callers of VL_GetAddrsU will iterate over 'nentries' addresses, even if the 'blkaddrs' OUT argument contains fewer entries. Or with AFSVolListOneVolume, some callers assume that at least 1 volintInfo has been returned; if 0 have been returned, we can try to access a NULL array. To avoid all of this, add various sanity checks on the relevant returned lengths of these RPCs. For most cases, if the lengths are not sane, return an internal error from the appropriate subsystem (or RXGEN_CC_UNMARSHAL if there isn't one). For VL_GetAddrsU, if 'nentries' is too long, just set it to the length of the returned array. FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15921 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit c732715e4ee78ed1e2414c813ae5a4b3574107a0) Reviewed-on: https://gerrit.openafs.org/15943 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 0ff2cd9e0f5656e8327c5fe47935998de3669678) [1.6: PRINTERNAL does not exist in this branch; instead use the naked hard-coded constant from 1.8 in ptuser.c and host.c.] Change-Id: I0a79a51cba4c12804947860446f46de1342bc85b --- src/WINNT/afsd/cm_vnodeops.c | 7 +++++ src/WINNT/afsd/cm_volume.c | 2 ++ src/afs/afs_volume.c | 4 +++ src/bucoord/ubik_db_if.c | 6 ++++ src/butc/dump.c | 18 ++++++++--- src/libadmin/adminutil/afs_utilAdmin.c | 12 ++++++-- src/libadmin/pts/afs_ptsAdmin.c | 4 +++ src/libadmin/vos/afs_vosAdmin.c | 8 +++++ src/libadmin/vos/vosutils.c | 6 ++++ src/libadmin/vos/vsprocs.c | 22 +++++++++++--- src/libafscp/afscp_server.c | 7 +++++ src/ptserver/ptuser.c | 3 ++ src/venus/cacheout.c | 4 +++ src/viced/host.c | 4 +++ src/vlserver/vlclient.c | 21 +++++++++++++ src/volser/vsprocs.c | 42 +++++++++++++++++--------- src/volser/vsutils.c | 3 ++ 17 files changed, 148 insertions(+), 25 deletions(-) diff --git a/src/WINNT/afsd/cm_vnodeops.c b/src/WINNT/afsd/cm_vnodeops.c index c03dee5833..2837184a7a 100644 --- a/src/WINNT/afsd/cm_vnodeops.c +++ b/src/WINNT/afsd/cm_vnodeops.c @@ -2398,6 +2398,13 @@ cm_TryBulkStatRPC(cm_scache_t *dscp, cm_bulkStat_t *bbp, cm_user_t *userp, cm_re } rx_PutConnection(rxconnp); + if (code == 0) { + if (statStruct.AFSBulkStats_len != filesThisCall || + callbackStruct.AFSCBs_len != filesThisCall) { + code = RXGEN_CC_UNMARSHAL; + } + } + /* * If InlineBulk RPC was called and it succeeded, * then pull out the return code from the status info diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c index 5b5eb8e845..548aa1dd83 100644 --- a/src/WINNT/afsd/cm_volume.c +++ b/src/WINNT/afsd/cm_volume.c @@ -455,6 +455,8 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t * } osi_Log1(afsd_logp, "CALL VL_GetAddrsU serverNumber %u SUCCESS", i); + nentries = min(nentries, addrs.bulkaddrs_len); + addrp = addrs.bulkaddrs_val; for (k = 0; k < nentries && j < NMAXNSERVERS; j++, k++) { serverFlags[j] = uvldbEntry.serverFlags[i]; diff --git a/src/afs/afs_volume.c b/src/afs/afs_volume.c index eee883ad16..4c3626deff 100644 --- a/src/afs/afs_volume.c +++ b/src/afs/afs_volume.c @@ -1158,6 +1158,10 @@ LockAndInstallUVolumeEntry(struct volume *av, struct uvldbentry *ve, int acell, return; } + if (addrs.bulkaddrs_len < nentries) { + nentries = addrs.bulkaddrs_len; + } + addrp = addrs.bulkaddrs_val; for (k = 0; k < nentries; k++) { addrp[k] = htonl(addrp[k]); diff --git a/src/bucoord/ubik_db_if.c b/src/bucoord/ubik_db_if.c index 9be620312b..62fb81108b 100644 --- a/src/bucoord/ubik_db_if.c +++ b/src/bucoord/ubik_db_if.c @@ -126,6 +126,12 @@ afs_int32 bcdb_listDumps (afs_int32 sflags, afs_int32 groupId, free(dumpsList.budb_dumpsList_val); if (flagsList.budb_dumpsList_val) free(flagsList.budb_dumpsList_val); + + if (code == 0 && + dumpsPtr->budb_dumpsList_len != flagsPtr->budb_dumpsList_len) { + code = BUDB_INTERNALERROR; + } + return (code); } diff --git a/src/butc/dump.c b/src/butc/dump.c index 8fcc61a058..6a4cff19ac 100644 --- a/src/butc/dump.c +++ b/src/butc/dump.c @@ -201,6 +201,18 @@ Bind(afs_uint32 server) return (curr_fromconn); } +static int +ListOneVolume(struct rx_connection *aconn, afs_int32 apart, afs_uint32 avolid, + volEntries *entries) +{ + afs_int32 code; + code = AFSVolListOneVolume(aconn, apart, avolid, entries); + if (code == 0 && entries->volEntries_len != 1) { + code = VOLSERFAILEDOP; + } + return code; +} + /* notes * 1) save the chunksize or otherwise ensure tape space remaining is * check frequently enough @@ -245,8 +257,7 @@ dumpVolume(struct tc_dumpDesc * curDump, struct dumpRock * dparamsPtr) /* Determine when the volume was last cloned and updated */ volumeInfo.volEntries_val = (volintInfo *) 0; volumeInfo.volEntries_len = 0; - rc = AFSVolListOneVolume(fromconn, curDump->partition, curDump->vid, - &volumeInfo); + rc = ListOneVolume(fromconn, curDump->partition, curDump->vid, &volumeInfo); if (rc) ERROR_EXIT(rc); updatedate = volumeInfo.volEntries_val[0].updateDate; @@ -574,8 +585,7 @@ xbsaDumpVolume(struct tc_dumpDesc * curDump, struct dumpRock * dparamsPtr) /* Determine when the volume was last cloned and updated */ volumeInfo.volEntries_val = (volintInfo *) 0; volumeInfo.volEntries_len = 0; - rc = AFSVolListOneVolume(fromconn, curDump->partition, curDump->vid, - &volumeInfo); + rc = ListOneVolume(fromconn, curDump->partition, curDump->vid, &volumeInfo); if (rc) ERROR_EXIT(rc); updatedate = volumeInfo.volEntries_val[0].updateDate; diff --git a/src/libadmin/adminutil/afs_utilAdmin.c b/src/libadmin/adminutil/afs_utilAdmin.c index 16e27d8c08..04a1dcac7f 100644 --- a/src/libadmin/adminutil/afs_utilAdmin.c +++ b/src/libadmin/adminutil/afs_utilAdmin.c @@ -2377,6 +2377,9 @@ util_CMClientConfig(struct rx_connection *conn, afs_ClientConfig_p config, afs_uint32 allocbytes; struct cacheConfig tconfig; + tconfig.cacheConfig_val = NULL; + tconfig.cacheConfig_len = 0; + if (conn == NULL) { tst = ADMRXCONNNULL; goto fail_util_CMClientConfig; @@ -2388,8 +2391,6 @@ util_CMClientConfig(struct rx_connection *conn, afs_ClientConfig_p config, } config->clientVersion = AFS_CLIENT_RETRIEVAL_VERSION; - tconfig.cacheConfig_val = NULL; - tconfig.cacheConfig_len = 0; tst = RXAFSCB_GetCacheConfig(conn, config->clientVersion, &config->serverVersion, &allocbytes, &tconfig); @@ -2398,12 +2399,17 @@ util_CMClientConfig(struct rx_connection *conn, afs_ClientConfig_p config, goto fail_util_CMClientConfig; } + if (tconfig.cacheConfig_len != sizeof(cm_initparams_v1)/sizeof(afs_uint32)) { + tst = RXGEN_CC_UNMARSHAL; + goto fail_util_CMClientConfig; + } + UnmarshallCMClientConfig(config->serverVersion, tconfig.cacheConfig_val, &config->c); rc = 1; - free(tconfig.cacheConfig_val); fail_util_CMClientConfig: + free(tconfig.cacheConfig_val); if (st != NULL) { *st = tst; diff --git a/src/libadmin/pts/afs_ptsAdmin.c b/src/libadmin/pts/afs_ptsAdmin.c index 05bf37607f..a25476d3d4 100644 --- a/src/libadmin/pts/afs_ptsAdmin.c +++ b/src/libadmin/pts/afs_ptsAdmin.c @@ -122,6 +122,10 @@ TranslatePTSNames(const afs_cell_handle_p cellHandle, namelist * names, goto fail_TranslatePTSNames; } + if (ids->idlist_len != names->namelist_len) { + tst = ADMPTSFAILEDNAMETRANSLATE; + goto fail_TranslatePTSNames; + } /* * Check to see if the lookup failed diff --git a/src/libadmin/vos/afs_vosAdmin.c b/src/libadmin/vos/afs_vosAdmin.c index 557feb90e7..1b19875081 100644 --- a/src/libadmin/vos/afs_vosAdmin.c +++ b/src/libadmin/vos/afs_vosAdmin.c @@ -1228,6 +1228,10 @@ GetServerRPC(void *rpc_specific, int slot, int *last_item, goto fail_GetServerRPC; } + if (addr_multi.bulkaddrs_len < total_multi) { + total_multi = addr_multi.bulkaddrs_len; + } + /* * Remove any bogus IP addresses which the user may have * been unable to remove. @@ -1380,6 +1384,10 @@ vos_FileServerGetBegin(const void *cellHandle, vos_MessageCallBack_t callBack, goto fail_vos_FileServerGetBegin; } + if (serv->addresses.bulkaddrs_len < serv->total_addresses) { + serv->total_addresses = serv->addresses.bulkaddrs_len; + } + /* * Remove any bogus IP addresses which the user may have * been unable to remove. diff --git a/src/libadmin/vos/vosutils.c b/src/libadmin/vos/vosutils.c index 4ac47bfe3e..2c49cf04e1 100644 --- a/src/libadmin/vos/vosutils.c +++ b/src/libadmin/vos/vosutils.c @@ -320,6 +320,9 @@ VLDB_ListAttributesN2(afs_cell_handle_p cellHandle, nextindexp); if (!tst) { rc = 1; + if (blkentriesp->nbulkentries_len < *nentriesp) { + *nentriesp = blkentriesp->nbulkentries_len; + } } if (st != NULL) { @@ -361,6 +364,9 @@ VLDB_IsSameAddrs(afs_cell_handle_p cellHandle, afs_int32 serv1, *equal = 0; goto fail_VLDB_IsSameAddrs; } + if (addrs.bulkaddrs_len < nentries) { + nentries = addrs.bulkaddrs_len; + } addrp = addrs.bulkaddrs_val; for (i = 0; i < nentries; i++, addrp++) { diff --git a/src/libadmin/vos/vsprocs.c b/src/libadmin/vos/vsprocs.c index 2fb11deb03..c9f60f3dcc 100644 --- a/src/libadmin/vos/vsprocs.c +++ b/src/libadmin/vos/vsprocs.c @@ -605,7 +605,7 @@ UV_MoveVolume(afs_cell_handle_p cellHandle, afs_uint32 afromvol, */ volumeInfo.volEntries_val = (volintInfo *) 0; /*this hints the stub to allocate space */ volumeInfo.volEntries_len = 0; - tst = AFSVolListOneVolume(fromconn, afrompart, afromvol, &volumeInfo); + tst = ListOneVolume(fromconn, afrompart, afromvol, &volumeInfo); if (tst) { goto fail_UV_MoveVolume; } @@ -1167,6 +1167,18 @@ UV_BackupVolume(afs_cell_handle_p cellHandle, afs_int32 aserver, return rc; } +static int +ListOneVolume(struct rx_connection *aconn, afs_int32 apart, afs_uint32 avolid, + volEntries *entries) +{ + afs_int32 code; + code = AFSVolListOneVolume(aconn, apart, avolid, entries); + if (code == 0 && entries->volEntries_len != 1) { + code = VOLSERFAILEDOP; + } + return code; +} + static int DelVol(struct rx_connection *conn, afs_uint32 vid, afs_int32 part, afs_int32 flags) @@ -1422,7 +1434,7 @@ VolumeExists(afs_cell_handle_p cellHandle, afs_int32 server, if (conn) { volumeInfo.volEntries_val = (volintInfo *) 0; volumeInfo.volEntries_len = 0; - tst = AFSVolListOneVolume(conn, partition, volumeid, &volumeInfo); + tst = ListOneVolume(conn, partition, volumeid, &volumeInfo); if (volumeInfo.volEntries_val) free(volumeInfo.volEntries_val); if (tst == VOLSERILLEGAL_PARTITION) { @@ -2893,7 +2905,9 @@ UV_XListOneVolume(struct rx_connection *server, afs_int32 a_partID, volumeXInfo.volXEntries_len = 0; tst = AFSVolXListOneVolume(server, a_partID, a_volID, &volumeXInfo); - + if (tst == 0 && volumeXInfo.volXEntries_len != 1) { + tst = VOLSERFAILEDOP; + } if (tst) { goto fail_UV_XListOneVolume; } else { @@ -2953,7 +2967,7 @@ int UV_ListOneVolume(struct rx_connection *server, afs_int32 a_partID, volumeInfo.volEntries_val = (volintInfo *) 0; volumeInfo.volEntries_len = 0; - tst = AFSVolListOneVolume(server, a_partID, a_volID, &volumeInfo); + tst = ListOneVolume(server, a_partID, a_volID, &volumeInfo); if (tst) { goto fail_UV_ListOneVolume; diff --git a/src/libafscp/afscp_server.c b/src/libafscp/afscp_server.c index e67ee48a9d..5fdbe44451 100644 --- a/src/libafscp/afscp_server.c +++ b/src/libafscp/afscp_server.c @@ -304,6 +304,9 @@ afscp_ServerById(struct afscp_cell *thecell, afsUUID * u) if (code != 0) { return NULL; } + if (addrs.bulkaddrs_len < nentries) { + nentries = addrs.bulkaddrs_len; + } if (nentries > AFS_MAXHOSTS) { nentries = AFS_MAXHOSTS; /* XXX I don't want to do *that* much dynamic allocation */ @@ -404,6 +407,10 @@ afscp_ServerByAddr(struct afscp_cell *thecell, afs_uint32 addr) afsUUID_to_string(&uuid, s, 511); afs_dprintf(("GetServerByAddr 0x%x -> uuid %s\n", addr, s)); + if (addrs.bulkaddrs_len < nentries) { + nentries = addrs.bulkaddrs_len; + } + if (nentries > AFS_MAXHOSTS) { nentries = AFS_MAXHOSTS; /* XXX I don't want to do *that* much dynamic allocation */ diff --git a/src/ptserver/ptuser.c b/src/ptserver/ptuser.c index 501fada050..7838e98049 100644 --- a/src/ptserver/ptuser.c +++ b/src/ptserver/ptuser.c @@ -496,6 +496,9 @@ pr_NameToId(namelist *names, idlist *ids) for (i = 0; i < names->namelist_len; i++) stolower(names->namelist_val[i]); code = ubik_PR_NameToID(pruclient, 0, names, ids); + if (code == 0 && ids->idlist_len != names->namelist_len) { + code = 267281L; /* PRINTERNAL */ + } return code; } diff --git a/src/venus/cacheout.c b/src/venus/cacheout.c index 3d44fd9fc7..03f6e297e8 100644 --- a/src/venus/cacheout.c +++ b/src/venus/cacheout.c @@ -82,6 +82,10 @@ ListServers(void) return 1; } + if (addrs.bulkaddrs_len < server_count) { + server_count = addrs.bulkaddrs_len; + } + for (i = 0; i < server_count; ++i) { ip = addrs.bulkaddrs_val[i]; diff --git a/src/viced/host.c b/src/viced/host.c index 4edcf43e4d..61f698943c 100644 --- a/src/viced/host.c +++ b/src/viced/host.c @@ -44,6 +44,7 @@ #endif #include #include +#include #include #include #include @@ -409,6 +410,9 @@ hpr_NameToId(namelist *names, idlist *ids) for (i = 0; i < names->namelist_len; i++) stolower(names->namelist_val[i]); code = ubik_PR_NameToID(uclient, 0, names, ids); + if (code == 0 && ids->idlist_len != names->namelist_len) { + code = 267281L; /* PRINTERNAL */ + } return code; #else return pr_NameToId(names, ids); diff --git a/src/vlserver/vlclient.c b/src/vlserver/vlclient.c index b29c5d860c..757b0803f1 100644 --- a/src/vlserver/vlclient.c +++ b/src/vlserver/vlclient.c @@ -584,6 +584,9 @@ handleit(struct cmd_syndesc *as, void *arock) printf("VL_ListAttributes returned code = %d\n", code); continue; } + if (entries.bulkentries_len < nentries) { + nentries = entries.bulkentries_len; + } entry = (struct vldbentry *)entries.bulkentries_val; for (i = 0; i < nentries; i++, entry++) display_entry(entry, 0); @@ -619,6 +622,9 @@ handleit(struct cmd_syndesc *as, void *arock) code); break; } + if (entries.nbulkentries_len < nentries) { + nentries = entries.nbulkentries_len; + } t += nentries; entry = (struct nvldbentry *)entries.nbulkentries_val; @@ -787,6 +793,9 @@ handleit(struct cmd_syndesc *as, void *arock) printf("VL_GetAddrs returned code = %d\n", code); continue; } + if (addrs.bulkaddrs_len < nentries) { + nentries = addrs.bulkaddrs_len; + } addrp = addrs.bulkaddrs_val; for (i = 0; i < nentries; i++, addrp++) { if ((*addrp & 0xff000000) == 0xff000000) @@ -812,6 +821,9 @@ handleit(struct cmd_syndesc *as, void *arock) printf("VL_GetAddrs returned code = %d\n", code); continue; } + if (addrs.bulkaddrs_len < nentries) { + nentries = addrs.bulkaddrs_len; + } addrp = addrs.bulkaddrs_val; for (i = 0; i < nentries; i++, addrp++) { if ((*addrp & 0xff000000) == 0xff000000) { @@ -837,6 +849,9 @@ handleit(struct cmd_syndesc *as, void *arock) printf("VL_GetAddrsU returned code = %d\n", code); continue; } + if (mhaddrs.bulkaddrs_len < mhnentries) { + mhnentries = mhaddrs.bulkaddrs_len; + } printf (" [%d]: uuid[%x,%x,%x,%x,%x,%x,%x,%x,%x,%x,%x]\n addrunique=%d, ip address(es):\n", attrs.index, uuid.time_low, uuid.time_mid, @@ -887,6 +902,9 @@ handleit(struct cmd_syndesc *as, void *arock) printf("VL_GetAddrs returned code = %d\n", code); continue; } + if (addrs1.bulkaddrs_len < nentries1) { + nentries1 = addrs1.bulkaddrs_len; + } addrp1 = addrs1.bulkaddrs_val; for (i = 0; i < nentries1; i++, addrp1++) { if ((*addrp1 & 0xff000000) != 0xff000000) { @@ -910,6 +928,9 @@ handleit(struct cmd_syndesc *as, void *arock) printf("VL_GetAddrsU returned code = %d\n", code); break; } + if (addrs2.bulkaddrs_len < nentries2) { + nentries2 = addrs2.bulkaddrs_len; + } addrp2 = addrs2.bulkaddrs_val; for (j = 0; j < nentries2; j++) { diff --git a/src/volser/vsprocs.c b/src/volser/vsprocs.c index c8f68daa89..59916a295c 100644 --- a/src/volser/vsprocs.c +++ b/src/volser/vsprocs.c @@ -2856,6 +2856,18 @@ UV_BackupVolume(afs_uint32 aserver, afs_int32 apart, afs_uint32 avolid) return error; } +static int +ListOneVolume(struct rx_connection *aconn, afs_int32 apart, afs_uint32 avolid, + volEntries *entries) +{ + afs_int32 code; + code = AFSVolListOneVolume(aconn, apart, avolid, entries); + if (code == 0 && entries->volEntries_len != 1) { + code = VOLSERFAILEDOP; + } + return code; +} + /* Make a new clone of volume on and * using volume ID , or a new ID allocated from the VLDB. * The new volume is named by , or by appending ".clone" to @@ -2883,7 +2895,7 @@ UV_CloneVolume(afs_uint32 aserver, afs_int32 apart, afs_uint32 avolid, if (!aname) { volumeInfo.volEntries_val = (volintInfo *) 0; volumeInfo.volEntries_len = 0; - code = AFSVolListOneVolume(aconn, apart, avolid, &volumeInfo); + code = ListOneVolume(aconn, apart, avolid, &volumeInfo); if (code) { fprintf(stderr, "Could not get info for volume %lu\n", (unsigned long)avolid); @@ -3621,9 +3633,8 @@ UV_ReleaseVolume(afs_uint32 afromvol, afs_uint32 afromserver, } volumeInfo.volEntries_val = NULL; volumeInfo.volEntries_len = 0; - code = AFSVolListOneVolume(conn, entry.serverPartition[vldbindex], - entry.volumeId[ROVOL], - &volumeInfo); + code = ListOneVolume(conn, entry.serverPartition[vldbindex], + entry.volumeId[ROVOL], &volumeInfo); if (code) { fprintf(STDERR, "Could not fetch information about RO vol %lu from server %s\n", (unsigned long)entry.volumeId[ROVOL], @@ -3651,8 +3662,7 @@ UV_ReleaseVolume(afs_uint32 afromvol, afs_uint32 afromserver, volEntries volumeInfo; volumeInfo.volEntries_val = NULL; volumeInfo.volEntries_len = 0; - code = AFSVolListOneVolume(fromconn, afrompart, afromvol, - &volumeInfo); + code = ListOneVolume(fromconn, afrompart, afromvol, &volumeInfo); if (code) { fprintf(STDERR, "Could not fetch information about RW vol %lu from server %s\n", (unsigned long)afromvol, @@ -4000,6 +4010,10 @@ UV_ReleaseVolume(afs_uint32 afromvol, afs_uint32 afromserver, nservers = 1; } + if (code == 0 && results.manyResults_len != tr.manyDests_len) { + code = VOLSERFAILEDOP; + } + if (code) { PrintError("Release failed: ", code); } else { @@ -5666,7 +5680,7 @@ UV_ListOneVolume(afs_uint32 aserver, afs_int32 apart, afs_uint32 volid, volumeInfo.volEntries_len = 0; aconn = UV_Bind(aserver, AFSCONF_VOLUMEPORT); - code = AFSVolListOneVolume(aconn, apart, volid, &volumeInfo); + code = ListOneVolume(aconn, apart, volid, &volumeInfo); if (code) { fprintf(STDERR, "Could not fetch the information about volume %lu from the server\n", @@ -5734,6 +5748,9 @@ UV_XListOneVolume(afs_uint32 a_serverID, afs_int32 a_partID, afs_uint32 a_volID, */ rxConnP = UV_Bind(a_serverID, AFSCONF_VOLUMEPORT); code = AFSVolXListOneVolume(rxConnP, a_partID, a_volID, &volumeXInfo); + if (code == 0 && volumeXInfo.volXEntries_len != 1) { + code = VOLSERFAILEDOP; + } if (code) fprintf(STDERR, "[UV_XListOneVolume] Couldn't fetch the volume information\n"); @@ -6355,9 +6372,7 @@ UV_SyncVolume(afs_uint32 aserver, afs_int32 apart, char *avolname, int flags) /* If a volume ID were given, search for it on each partition */ if ((volumeid = atol(avolname))) { for (j = 0; j < pcnt; j++) { - code = - AFSVolListOneVolume(aconn, PartList.partId[j], volumeid, - &volumeInfo); + code = ListOneVolume(aconn, PartList.partId[j], volumeid, &volumeInfo); if (code) { if (code != ENODEV) { fprintf(STDERR, "Could not query server\n"); @@ -6395,9 +6410,8 @@ UV_SyncVolume(afs_uint32 aserver, afs_int32 apart, char *avolname, int flags) for (k = 0; k < pcnt; k++) { /* For each partition */ volumeInfo.volEntries_val = (volintInfo *) 0; volumeInfo.volEntries_len = 0; - code = - AFSVolListOneVolume(aconn, PartList.partId[k], - vldbentry.volumeId[j], &volumeInfo); + code = ListOneVolume(aconn, PartList.partId[k], + vldbentry.volumeId[j], &volumeInfo); if (code) { if (code != ENODEV) { fprintf(STDERR, "Could not query server\n"); @@ -6649,7 +6663,7 @@ VolumeExists(afs_uint32 server, afs_int32 partition, afs_uint32 volumeid) if (conn) { volumeInfo.volEntries_val = (volintInfo *) 0; volumeInfo.volEntries_len = 0; - code = AFSVolListOneVolume(conn, partition, volumeid, &volumeInfo); + code = ListOneVolume(conn, partition, volumeid, &volumeInfo); if (volumeInfo.volEntries_val) free(volumeInfo.volEntries_val); if (code == VOLSERILLEGAL_PARTITION) diff --git a/src/volser/vsutils.c b/src/volser/vsutils.c index 711673f633..e9578f287d 100644 --- a/src/volser/vsutils.c +++ b/src/volser/vsutils.c @@ -389,6 +389,9 @@ VLDB_IsSameAddrs(afs_uint32 serv1, afs_uint32 serv2, afs_int32 *errorp) } code = 0; + if (addrs.bulkaddrs_len < nentries) { + nentries = addrs.bulkaddrs_len; + } if (nentries > GETADDRUCACHESIZE) nentries = GETADDRUCACHESIZE; /* safety check; should not happen */ if (++cacheip_index >= GETADDRUCACHESIZE) -- 2.45.2 From 28ccaefacebbc3ba5f646d9f41d2731b0446cb51 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 15 Oct 2020 20:30:14 -0500 Subject: [PATCH 08/11] OPENAFS-SA-2024-003: xdr: Ensure correct string length in xdr_string CVE-2024-10397 Currently, if a caller calls an RPC with a string output argument, like so: { char *str = NULL; code = RXAFS_SomeCall(&str); /* do something with 'str' */ xdr_free((xdrproc_t) xdr_string, &str); } Normally, xdr_free causes xdr_string to call osi_free, specifying the same size that we allocated for the string. However, since we only have a char*, the amount of space allocated for the string is not recorded separately, and so xdr_string calculates the size of the buffer to free by using strlen(). This works for well-formed strings, but if we fail to decode the payload of the string, or if our peer gave us a string with a NUL byte in the middle of it, then strlen() may be significantly less than the actual allocated size. And so in this case, the size given to osi_free will be wrong. The size given to osi_free is ignored in userspace, and for KERNEL on many platforms like Linux and DARWIN. However, it is notably not ignored for KERNEL on Solaris and some other less supported platforms (HPUX, Irix, NetBSD). At least on Solaris, an incorrect size given to osi_free can cause a system panic or possibly memory corruption. To avoid this, change xdr_string during XDR_DECODE to make sure that strlen() of the string always reflects the allocated size. If we fail to decode the string's payload, replace the payload with non-NUL bytes (fill it with 'z', an arbitrary choice). And if we do successfully decode the payload, check if the strlen() is wrong (that is, if the payload contains NUL '\0' bytes), and fail if so, also filling the payload with 'z'. This is only strictly needed in KERNEL on certain platforms, but do it everywhere so our behavior is consistent. FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15922 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 7d0675e6c6a2f3200a3884fbe46b3ef8ef9ffd24) Reviewed-on: https://gerrit.openafs.org/15944 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit a82212ab20f0635a40c52648a52a1e9eaccc4937) Change-Id: If3b3eac9edd1711cce70ccce8454d3d96f8014f8 --- src/rx/xdr.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/rx/xdr.c b/src/rx/xdr.c index b05377301a..386abd2ab3 100644 --- a/src/rx/xdr.c +++ b/src/rx/xdr.c @@ -580,7 +580,26 @@ xdr_string(XDR * xdrs, char **cpp, u_int maxsize) return (FALSE); } sp[size] = 0; - /* fall into ... */ + + /* Get the actual string. */ + if (!xdr_opaque(xdrs, sp, size)) { + /* Make sure strlen(sp) == size, so we can calculate the correct + * size for osi_free when freeing the string. */ + memset(sp, 'z', size); + return FALSE; + } + + /* + * If the string contains a '\0' character, the string is invalid. + * Don't allow this, because this makes it impossible for us to pass + * the correct size to osi_free later on, when freeing the string. + */ + if (strlen(sp) != size) { + /* Make sure strlen(sp) == size. */ + memset(sp, 'z', size); + return FALSE; + } + return TRUE; case XDR_ENCODE: return (xdr_opaque(xdrs, sp, size)); -- 2.45.2 From a5e85cddab167eb13813f3ef947c09798f0472ec Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Fri, 16 Oct 2020 10:52:03 -0500 Subject: [PATCH 09/11] OPENAFS-SA-2024-003: Run xdr_free for retried RPCs CVE-2024-10397 A few areas of code retry the same RPC, like so: do { code = VL_SomeRPC(rxconn, &array_out); } while (some_condition); xdr_free((xdrproc_t) xdr_foo, &array_out); Or try a different version/variant of an RPC (e.g. VLDB_ListAttributesN2 -> VLDB_ListAttributes). If the first RPC call causes the output array to be allocated with length N, then the subsequent RPC calls may fail if the server responds with an array larger than N. Furthermore, if the subsequent call responds with an array smaller than N, then when we xdr_free the array, our length will be smaller than the actual number of allocated elements. That results in two potential issues: - We'll fail to free the elements at the end of the array. This is only a problem if each element in the array also uses dynamically-allocated memory (e.g. each element contains a string or another array). Fortunately, there are only a few such structures in any of our RPC-L definitions: SysNameList and CredInfos. And neither of those are used in such a retry loop, so this isn't a problem. - We'll give the wrong length to osi_free when freeing the array itself. This only matters for KERNEL, and only on some platforms (such as Solaris), since the length given to osi_free is ignored everywhere else. To avoid these possible issues, change the relevant retry loops to free our xdr-allocated arrays on every iteration of the loop, like this: do { xdr_free((xdrproc_t) xdr_foo, &array_out); code = VL_SomeRPC(rxconn, &array_out); } while (some_condition); xdr_free((xdrproc_t) xdr_foo, &array_out); Or like this: do { code = VL_SomeRPC(rxconn, &array_out); xdr_free((xdrproc_t) xdr_foo, &array_out); } while (some_condition); FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15923 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 1f5e1ef9e35f6b5e8693c91199c976d5e030c0d0) Reviewed-on: https://gerrit.openafs.org/15945 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 25ad3931d5c03ead625a96e6b626febeb3e20453) Change-Id: I3c2e883933195674062ee84c1162a990240e3a44 --- src/WINNT/afsd/cm_volume.c | 1 + src/afs/afs_pioctl.c | 1 + src/afs/afs_volume.c | 1 + src/ptserver/pts.c | 1 + src/ptserver/testpt.c | 1 + src/volser/vos.c | 1 + 6 files changed, 6 insertions(+) diff --git a/src/WINNT/afsd/cm_volume.c b/src/WINNT/afsd/cm_volume.c index 548aa1dd83..f0cb4d564d 100644 --- a/src/WINNT/afsd/cm_volume.c +++ b/src/WINNT/afsd/cm_volume.c @@ -442,6 +442,7 @@ long cm_UpdateVolumeLocation(struct cm_cell *cellp, cm_user_t *userp, cm_req_t * if (code) continue; + xdr_free((xdrproc_t) xdr_bulkaddrs, &addrs); rxconnp = cm_GetRxConn(connp); code = VL_GetAddrsU(rxconnp, &attrs, &uuid, &unique, &nentries, &addrs); rx_PutConnection(rxconnp); diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c index 37c52e982f..36d67f1d3f 100644 --- a/src/afs/afs_pioctl.c +++ b/src/afs/afs_pioctl.c @@ -1596,6 +1596,7 @@ DECL_PIOCTL(PGetAcl) if (tconn) { XSTATS_START_TIME(AFS_STATS_FS_RPCIDX_FETCHACL); RX_AFS_GUNLOCK(); + xdr_free((xdrproc_t) xdr_AFSOpaque, &acl); code = RXAFS_FetchACL(rxconn, &Fid, &acl, &OutStatus, &tsync); RX_AFS_GLOCK(); XSTATS_END_TIME; diff --git a/src/afs/afs_volume.c b/src/afs/afs_volume.c index 4c3626deff..cb74bd09ca 100644 --- a/src/afs/afs_volume.c +++ b/src/afs/afs_volume.c @@ -1138,6 +1138,7 @@ LockAndInstallUVolumeEntry(struct volume *av, struct uvldbentry *ve, int acell, 0, &rxconn); if (tconn) { RX_AFS_GUNLOCK(); + xdr_free((xdrproc_t) xdr_bulkaddrs, &addrs); code = VL_GetAddrsU(rxconn, &attrs, &uuid, &unique, &nentries, &addrs); diff --git a/src/ptserver/pts.c b/src/ptserver/pts.c index 0035f5bacc..93556b07c4 100644 --- a/src/ptserver/pts.c +++ b/src/ptserver/pts.c @@ -734,6 +734,7 @@ CheckEntry(struct cmd_syndesc *as, void *arock) lids.idlist_val[0] = aentry.owner; lids.idlist_val[1] = aentry.creator; + xdr_free((xdrproc_t) xdr_namelist, &lnames); code = pr_IdToName(&lids, &lnames); if (code) { rcode = code; diff --git a/src/ptserver/testpt.c b/src/ptserver/testpt.c index d0a1c93924..d41db77cf6 100644 --- a/src/ptserver/testpt.c +++ b/src/ptserver/testpt.c @@ -126,6 +126,7 @@ ListUsedIds(struct cmd_syndesc *as, void *arock) startId++; } lids.idlist_len = i; + xdr_free((xdrproc_t) xdr_namelist, &lnames); code = pr_IdToName(&lids, &lnames); if (code) { afs_com_err(whoami, code, "converting id to name"); diff --git a/src/volser/vos.c b/src/volser/vos.c index a7b4851fc2..cda14f05b1 100644 --- a/src/volser/vos.c +++ b/src/volser/vos.c @@ -4580,6 +4580,7 @@ ListVLDB(struct cmd_syndesc *as, void *arock) &arrayEntries, &nextindex); if (vcode == RXGEN_OPCODE) { /* Vlserver not running with ListAttributesN2. Fall back */ + xdr_free((xdrproc_t) xdr_nbulkentries, &arrayEntries); vcode = VLDB_ListAttributes(&attributes, ¢ries, &arrayEntries); nextindex = -1; -- 2.45.2 From 74e68017f420454c873277eab868037b4ae99e6f Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Fri, 16 Oct 2020 10:55:15 -0500 Subject: [PATCH 10/11] OPENAFS-SA-2024-003: sys: Don't over-copy RMTSYS_Pioctl output data CVE-2024-10397 Here, 'OutData' only has OutData.rmtbulk_len bytes in it. We know that OutData.rmtbulk_len is at most data->out_size, but it could be smaller. So, only copy OutData.rmtbulk_len bytes, not data->out_size, since data->out_size could be more than the number of bytes we have allocated in OutData. FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15924 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit f31a79d749abc8e64a8d9ac748bb2b5457875099) Reviewed-on: https://gerrit.openafs.org/15946 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 4871f8ad2775e97bb85ff7efc33a4ad8d3f6d9d1) Change-Id: Ia9c6332c2f7b3f9e3b0112b2aa19515626f6962a --- src/sys/rmtsysc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sys/rmtsysc.c b/src/sys/rmtsysc.c index 490626111c..e32718eeea 100644 --- a/src/sys/rmtsysc.c +++ b/src/sys/rmtsysc.c @@ -291,7 +291,7 @@ pioctl(char *path, afs_int32 cmd, struct ViceIoctl *data, afs_int32 follow) errno = EINVAL; errorcode = -1; } else { - memcpy(data->out, OutData.rmtbulk_val, data->out_size); + memcpy(data->out, OutData.rmtbulk_val, OutData.rmtbulk_len); outparam_conversion(cmd, data->out, 1); } } -- 2.45.2 From 916ec99212a0f4ace3733be97f31b272f118dde3 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 15 Oct 2020 21:07:17 -0500 Subject: [PATCH 11/11] OPENAFS-SA-2024-003: xdr: Initialize memory for INOUT args CVE-2024-10397 Currently, there are a few callers of RPCs that specify some data for an INOUT parameter, but do not initialize the memory for that data. This can result in the uninitialized memory being sent to the peer when the argument is processed as an IN argument. Simply clear the relevant data before running the RPC to avoid this. The relevant RPCs and arguments are: - For RMTSYS_Pioctl, the 'OutData' argument. - For BUDB_GetVolumes, the 'volumes' argument. -- via DBLookupByVolume -> bcdb_LookupVolume -> ubik_BUDB_GetVolumes -- and via bc_Restorer -> bcdb_FindVolumes -> ubik_BUDB_GetVolumes - For KAA_Authenticate_old / KAA_Authenticate, this can happen with the 'answer' argument in ka_Authenticate if KAA_AuthenticateV2 or KAA_Authenticate return RXGEN_OPCODE, but the server manages to populate oanswer.SeqLen with non-zero. For all of these, make sure the memory is blanked before running the relevant RPC. For ka_Authenticate, reset oanswer.SeqLen to 0 to avoid sending any data, but still blank 'answer' and 'answer_old' just to be safe. FIXES 135043 Reviewed-on: https://gerrit.openafs.org/15925 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit c4e28c2afe743aa323be57ef3b0faec13027e678) Reviewed-on: https://gerrit.openafs.org/15947 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 37e585f0841803cdf3a1f99770034890ba162d7c) Change-Id: I6677356a9f036c3ee386a354719c4477cbfbd513 --- src/afs/afs_pag_call.c | 2 ++ src/bucoord/commands.c | 2 ++ src/bucoord/restore.c | 2 +- src/kauth/authclient.c | 4 ++++ src/sys/rmtsysc.c | 2 +- 5 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/afs/afs_pag_call.c b/src/afs/afs_pag_call.c index a1f40b5809..edcc301091 100644 --- a/src/afs/afs_pag_call.c +++ b/src/afs/afs_pag_call.c @@ -493,6 +493,8 @@ afs_syscall_pioctl(path, com, cmarg, follow) goto out_idata; } + memset(odata.rmtbulk_val, 0, out_size); + AFS_GUNLOCK(); code = RMTSYS_Pioctl(rmtsys_conn, &ccred, abspath, com, follow, &idata, &odata, &err); diff --git a/src/bucoord/commands.c b/src/bucoord/commands.c index 54a978ce6e..9d2a7d0719 100644 --- a/src/bucoord/commands.c +++ b/src/bucoord/commands.c @@ -2754,6 +2754,8 @@ DBLookupByVolume(char *volumeName) char vname[BU_MAXNAMELEN]; char ds[50]; + memset(volumeEntry, 0, sizeof(volumeEntry)); + for (pass = 0; pass < 2; pass++) { /*p */ /* On second pass, search for backup volume */ diff --git a/src/bucoord/restore.c b/src/bucoord/restore.c index 69bfba294a..6eac5b7b59 100644 --- a/src/bucoord/restore.c +++ b/src/bucoord/restore.c @@ -199,7 +199,7 @@ bc_Restorer(afs_int32 aindex) partitionAll = dumpTaskPtr->destPartition; volumeEntries = (struct budb_volumeEntry *) - malloc(MAXTAPESATONCE * sizeof(struct budb_volumeEntry)); + calloc(MAXTAPESATONCE, sizeof(struct budb_volumeEntry)); if (!volumeEntries) { afs_com_err(whoami, BC_NOMEM, NULL); ERROR(BC_NOMEM); diff --git a/src/kauth/authclient.c b/src/kauth/authclient.c index 0d577714ec..300f9ddf51 100644 --- a/src/kauth/authclient.c +++ b/src/kauth/authclient.c @@ -540,12 +540,15 @@ ka_Authenticate(char *name, char *instance, char *cell, struct ubik_client * con oanswer.MaxSeqLen = sizeof(answer); oanswer.SeqLen = 0; oanswer.SeqBody = (char *)&answer; + memset(&answer, 0, sizeof(answer)); + memset(&answer_old, 0, sizeof(answer_old)); version = 2; code = kawrap_ubik_Call(KAA_AuthenticateV2, conn, 0, name, instance, (void*)(uintptr_t)start, (void*)(uintptr_t)end, &arequest, &oanswer, 0, 0); if (code == RXGEN_OPCODE) { + oanswer.SeqLen = 0; oanswer.MaxSeqLen = sizeof(answer); oanswer.SeqBody = (char *)&answer; version = 1; @@ -553,6 +556,7 @@ ka_Authenticate(char *name, char *instance, char *cell, struct ubik_client * con ubik_KAA_Authenticate(conn, 0, name, instance, start, end, &arequest, &oanswer); if (code == RXGEN_OPCODE) { + oanswer.SeqLen = 0; oanswer.MaxSeqLen = sizeof(answer_old); oanswer.SeqBody = (char *)&answer_old; version = 0; diff --git a/src/sys/rmtsysc.c b/src/sys/rmtsysc.c index e32718eeea..69bae83ef5 100644 --- a/src/sys/rmtsysc.c +++ b/src/sys/rmtsysc.c @@ -245,7 +245,7 @@ pioctl(char *path, afs_int32 cmd, struct ViceIoctl *data, afs_int32 follow) inparam_conversion(cmd, InData.rmtbulk_val, 0); OutData.rmtbulk_len = MAXBUFFERLEN * sizeof(*OutData.rmtbulk_val); - OutData.rmtbulk_val = malloc(OutData.rmtbulk_len); + OutData.rmtbulk_val = calloc(1, OutData.rmtbulk_len); if (!OutData.rmtbulk_val) { free(inbuffer); return -1; -- 2.45.2