From 0d6b9defb36cb94f3d34b058f00055e9e99d85fc Mon Sep 17 00:00:00 2001 From: Michael Meffie Date: Fri, 21 May 2021 12:38:01 -0400 Subject: [PATCH] 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. Change-Id: I963e1b790417863c036e897811c86a634d1d4e7f Reviewed-on: https://gerrit.openafs.org/14626 Reviewed-by: Benjamin Kaduk Tested-by: BuildBot --- src/libadmin/adminutil/afs_AdminUtilErrors.et | 1 + src/libadmin/adminutil/afs_utilAdmin.c | 26 ++++++-- src/libadmin/bos/afs_bosAdmin.c | 94 +++++++++++++++++++++------ 3 files changed, 94 insertions(+), 27 deletions(-) diff --git a/src/libadmin/adminutil/afs_AdminUtilErrors.et b/src/libadmin/adminutil/afs_AdminUtilErrors.et index e60c2e5..d031638 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 5183654..390af4a 100644 --- a/src/libadmin/adminutil/afs_utilAdmin.c +++ b/src/libadmin/adminutil/afs_utilAdmin.c @@ -2002,14 +2002,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); @@ -2018,7 +2018,11 @@ ListCellsRPC(void *rpc_specific, int slot, int *last_item, if (tst) { goto fail_ListCellsRPC; } - strcpy(t->cell[slot].cellname, name); + len = 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]; @@ -2038,6 +2042,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; @@ -2235,7 +2240,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; @@ -2247,15 +2253,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 = 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 da37919..76436c0 100644 --- a/src/libadmin/bos/afs_bosAdmin.c +++ b/src/libadmin/bos/afs_bosAdmin.c @@ -526,6 +526,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; @@ -547,18 +549,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 = 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; } @@ -707,11 +715,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 = 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; @@ -720,6 +734,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; } @@ -935,8 +952,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; @@ -959,13 +975,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; @@ -1000,6 +1015,7 @@ bos_ProcessInfoGet(const void *serverHandle, char *processName, fail_bos_ProcessInfoGet: + xdr_free((xdrproc_t)xdr_string, &type); if (st != NULL) { *st = tst; } @@ -1024,13 +1040,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 = 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; @@ -1039,6 +1061,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; } @@ -1691,23 +1716,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 = 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; } @@ -2277,6 +2311,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; @@ -2287,14 +2323,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 = 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; } @@ -2416,11 +2461,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 = 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; @@ -2429,6 +2480,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; } -- 1.9.4