From 6e08c0368aa43fa9851556050f2e4bf4e59edaea Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Wed, 7 Aug 2019 20:50:47 -0500 Subject: [PATCH 1/5] OPENAFS-SA-2019-001: Skip server OUT args on error Currently, part of our server-side RPC argument-handling code that's generated from rxgen looks like this (for example): z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync); z_xdrs->x_op = XDR_ENCODE; if ((!xdr_AFSBulkStats(z_xdrs, &StatArray)) || (!xdr_AFSCBs(z_xdrs, &CBArray)) || (!xdr_AFSVolSync(z_xdrs, &Sync))) z_result = RXGEN_SS_MARSHAL; fail: [...] return z_result; When the server routine for implementing the RPC results a non-zero value into z_result, the call will be aborted. However, before we abort the call, we still call the xdr_* routines with XDR_ENCODE for all of our output arguments. If the call has not already been aborted for other reasons, we'll serialize the output argument data into the Rx call. If we push more data than can fit in a single Rx packet for the call, then we'll also send that data to the client. Many server routines for implementing RPCs do not initialize the memory inside their output arguments during certain errors, and so the memory may be leaked to the peer. To avoid this, just jump to the 'fail' label when a nonzero 'z_result' is returned. This means we skip sending the output argument data to the peer, but we still free any argument data that needs freeing, and record the stats for the call (if needed). This makes the above example now look like this: z_result = SRXAFS_BulkStatus(z_call, &FidsArray, &StatArray, &CBArray, &Sync); if (z_result) goto fail; z_xdrs->x_op = XDR_ENCODE; if ((!xdr_AFSBulkStats(z_xdrs, &StatArray)) || (!xdr_AFSCBs(z_xdrs, &CBArray)) || (!xdr_AFSVolSync(z_xdrs, &Sync))) z_result = RXGEN_SS_MARSHAL; fail: [...] return z_result; Reviewed-on: https://gerrit.openafs.org/13913 Reviewed-by: Andrew Deason Tested-by: BuildBot Reviewed-by: Benjamin Kaduk (cherry picked from commit ea276e83e37e5bd27285a3d639f2158639172786) Reviewed-on: https://gerrit.openafs.org/13916 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 5a3d1b62810fc8cc7b37a737b4f5f1912bc614f9) Change-Id: Ia54b06399b1f8a05a560ec7560580783d3e64b9d Reviewed-on: https://gerrit.openafs.org/13921 Reviewed-by: Benjamin Kaduk Tested-by: BuildBot --- src/rxgen/rpc_parse.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c index 5a3f27d845..8ff57d0794 100644 --- a/src/rxgen/rpc_parse.c +++ b/src/rxgen/rpc_parse.c @@ -1369,8 +1369,12 @@ static void ss_Proc_CodeGeneration(definition * defp) { int somefrees = 0; + extern char zflag; - defp->can_fail = 0; + if (zflag) + defp->can_fail = 0; + else + defp->can_fail = 1; ss_ProcName_setup(defp); if (!cflag) { ss_ProcParams_setup(defp, &somefrees); @@ -1610,6 +1614,8 @@ ss_ProcCallRealProc_setup(definition * defp) f_print(fout, ");\n"); if (zflag) { f_print(fout, "\tif (z_result)\n\t\treturn z_result;\n"); + } else { + f_print(fout, "\tif (z_result)\n\t\tgoto fail;\n"); } } From 2a7b4b891bec730a6c4f58e3b5976383e4c179c1 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Wed, 7 Aug 2019 21:19:47 -0500 Subject: [PATCH 2/5] OPENAFS-SA-2019-002: Zero all server RPC args Currently, our server-side RPC argument-handling code generated from rxgen initializes complex arguments like so (for example, in _RXAFS_BulkStatus): AFSCBFids FidsArray; AFSBulkStats StatArray; AFSCBs CBArray; AFSVolSync Sync; FidsArray.AFSCBFids_val = 0; FidsArray.AFSCBFids_len = 0; CBArray.AFSCBs_val = 0; CBArray.AFSCBs_len = 0; StatArray.AFSBulkStats_val = 0; StatArray.AFSBulkStats_len = 0; This is done for any input or output arguments, but only for types we need to free afterwards (arrays, usually). We do not do this for simple types, like single flat structs. In the above example, we do this for the arrays FidsArray, StatArray, and CBArray, but 'Sync' is not initialized to anything. If some server RPC handlers never set a value for an output argument, this means we'll send uninitialized stack memory to our peer. Currently this can happen in, for example, MRXSTATS_RetrieveProcessRPCStats if 'rxi_monitor_processStats' is unset (specifically, the 'clock_sec' and 'clock_usec' arguments are never set when rx_enableProcessRPCStats() has not been called). To make sure we cannot send uninitialized data to our peer, change rxgen to instead 'memset(&arg, 0, sizeof(arg));' for every single parameter. Using memset in this way just makes this a little simpler inside rxgen, since all we need to do this is the name of the argument. With this commit, the rxgen-generated code for the above example now looks like this: AFSCBFids FidsArray; AFSBulkStats StatArray; AFSCBs CBArray; AFSVolSync Sync; memset(&FidsArray, 0, sizeof(FidsArray)); memset(&CBArray, 0, sizeof(CBArray)); memset(&StatArray, 0, sizeof(StatsArray)); memset(&Sync, 0, sizeof(Sync)); Reviewed-on: https://gerrit.openafs.org/13914 Reviewed-by: Andrew Deason Reviewed-by: Benjamin Kaduk Tested-by: BuildBot (cherry picked from commit 93aee3cf40622993b95bd1af77080a31670c24bb) Reviewed-on: https://gerrit.openafs.org/13917 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit fcaac44f845d18d6fd5d2f3685db11118d8f8626) Change-Id: Ic096570e9c894fb05d084ba451beabda3bb295e2 Reviewed-on: https://gerrit.openafs.org/13922 Reviewed-by: Benjamin Kaduk Tested-by: BuildBot --- src/rxgen/rpc_parse.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/rxgen/rpc_parse.c b/src/rxgen/rpc_parse.c index 8ff57d0794..e3731dfc8a 100644 --- a/src/rxgen/rpc_parse.c +++ b/src/rxgen/rpc_parse.c @@ -1508,8 +1508,6 @@ ss_ProcSpecial_setup(definition * defp, int *somefrees) plist->pl.string_name = spec->sdef.string_name; plist->pl.param_flag |= FREETHIS_PARAM; *somefrees = 1; - fprintf(fout, "\n\t%s.%s = 0;", plist->pl.param_name, - spec->sdef.string_name); } } } @@ -1527,22 +1525,13 @@ ss_ProcSpecial_setup(definition * defp, int *somefrees) case REL_ARRAY: plist->pl.string_name = alloc(40); if (brief_flag) { - f_print(fout, "\n\t%s.val = 0;", - plist->pl.param_name); - f_print(fout, "\n\t%s.len = 0;", - plist->pl.param_name); s_print(plist->pl.string_name, "val"); } else { - f_print(fout, "\n\t%s.%s_val = 0;", - plist->pl.param_name, defp1->def_name); - f_print(fout, "\n\t%s.%s_len = 0;", - plist->pl.param_name, defp1->def_name); s_print(plist->pl.string_name, "%s_val", defp1->def_name); } break; case REL_POINTER: - f_print(fout, "\n\t%s = 0;", plist->pl.param_name); plist->pl.string_name = NULL; break; default: @@ -1552,6 +1541,15 @@ ss_ProcSpecial_setup(definition * defp, int *somefrees) } } } + + for (plist = defp->pc.plists; plist; plist = plist->next) { + if (plist->component_kind == DEF_PARAM) { + fprintf(fout, "\n\tmemset(&%s, 0, sizeof(%s));", + plist->pl.param_name, + plist->pl.param_name); + } + } + f_print(fout, "\n"); } From 3915886843a1d4b21bd2539e7e9d4057965a52dd Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Mon, 16 Sep 2019 14:06:53 -0500 Subject: [PATCH 3/5] OPENAFS-SA-2019-003: ubik: Avoid unlocked ubik_currentTrans deref Currently, SVOTE_Debug/SVOTE_DebugOld examine some ubik internal state without any locks, because the speed of these functions is more important than accuracy. However, one of the pieces of data we examine is ubik_currentTrans, which we dereference to get ubik_currentTrans->type. ubik_currentTrans could be set to NULL while this code is running, so there is a small chance of this code causing a segfault, if SVOTE_Debug() is running when the current transaction ends. We only ever initialize ubik_currentTrans as a write transation (via SDISK_Begin), so this check is pointless anyway. Accordingly, skip the type check, and always assume that any active transaction is a write transaction. This means we only ever access ubik_currentTrans once, avoiding any risk of the value changing between accesses (and we no longer need to dereference it, anyway). Note that, since ubik_currentTrans is not marked as 'volatile', some C compilers, with certain options, can and do assume that its value will not change between accesses, and thus only fetch the pointer value once. This avoids the risk of NULL dereference (and thus, crash, if pointer stores/loads are atomic), but the value pointed to by ubik_currentTrans->type would be incorrect when the transaction ends during the execution of SVOTE_Debug(). Reviewed-on: https://gerrit.openafs.org/13915 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 6ec46ba7773089e1549d27a0d345afeca65c9472) Reviewed-on: https://gerrit.openafs.org/13918 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 213b9dc386ff89a14379313ee8ec09280f130a29) Change-Id: I0ddbc2309de09a629150a6b3825e1aa8bda8b910 Reviewed-on: https://gerrit.openafs.org/13923 Tested-by: BuildBot Reviewed-by: Benjamin Kaduk --- src/ubik/vote.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/ubik/vote.c b/src/ubik/vote.c index 5d75a80228..4d6ab44a20 100644 --- a/src/ubik/vote.c +++ b/src/ubik/vote.c @@ -447,10 +447,7 @@ SVOTE_Debug(struct rx_call * rxcall, struct ubik_debug * aparm) if (ubik_currentTrans) { aparm->currentTrans = 1; - if (ubik_currentTrans->type == UBIK_WRITETRANS) - aparm->writeTrans = 1; - else - aparm->writeTrans = 0; + aparm->writeTrans = 1; } else { aparm->currentTrans = 0; } @@ -530,10 +527,7 @@ SVOTE_DebugOld(struct rx_call * rxcall, if (ubik_currentTrans) { aparm->currentTrans = 1; - if (ubik_currentTrans->type == UBIK_WRITETRANS) - aparm->writeTrans = 1; - else - aparm->writeTrans = 0; + aparm->writeTrans = 1; } else { aparm->currentTrans = 0; } From c2496e960bbe35a4bd3a0d26dd5626a747822c77 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Tue, 22 Oct 2019 00:08:36 -0700 Subject: [PATCH 4/5] Update NEWS for 1.6.24 Release notes for the OpenAFS 1.6.24 security release. Change-Id: Id12988da9e71dc338bf259d4ac32ceaa6da70197 --- NEWS | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/NEWS b/NEWS index 2c40253ada..0386904800 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,32 @@ User-Visible OpenAFS Changes +OpenAFS 1.6.24 + +All platforms + + * Fix OPENAFS-SA-2019-001: information leakage in failed RPC output + Generated RPC handler routines ran output variables through XDR encoding + even when the call had failed and would shortly be aborted (and for + which uninitialized output variables is common); any complete packets + assembled in the process would be sent to the peer, leaking the contents + of the uninitialized memory in question. + + * Fix OPENAFS-SA-2019-002: information leakage from uninitialized scalars + Generated RPC handler routines did not initialize output variables of + scalar (fixed-length) type, since they did not require dedicated logic to + free. Such variables allocated on the stack could remain uninitialized + in some cases (including those affected by OPENAFS-SA-2019-001), and the + contents of uninitialized memory would be returned to the peer. + +All server platforms + + * Fix OPENAFS-SA-2019-003: fix crash in database servers + The ubik debugging RPCs prioritize being fast and non-disruptive to + database operations over strict correctness, and do not adhere to the + usual locking protocol for data access. A data race could cause a NULL + dereference if the second memory load was not optimized out by the + compiler. + OpenAFS 1.6.23 All platforms From 418214d1db7b52575afd6a8c478ca4968ba3ba05 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Tue, 22 Oct 2019 09:39:02 -0700 Subject: [PATCH 5/5] Make OpenAFS 1.6.24 Update version strings for the 1.6.24 release. Change-Id: I0a453d32018cc84b491c5d7f897229c365958054 --- configure-libafs.ac | 2 +- configure.ac | 4 ++-- src/config/NTMakefile.amd64_w2k | 2 +- src/config/NTMakefile.i386_nt40 | 2 +- src/config/NTMakefile.i386_w2k | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/configure-libafs.ac b/configure-libafs.ac index 4a02dc5fe3..62bcb33d39 100644 --- a/configure-libafs.ac +++ b/configure-libafs.ac @@ -5,7 +5,7 @@ AC_CONFIG_SRCDIR(src/libafs/Makefile.common.in) AM_INIT_AUTOMAKE AC_CONFIG_HEADER(src/config/afsconfig.h) -MACOS_VERSION=1.6.23 +MACOS_VERSION=1.6.24 AC_SUBST(MACOS_VERSION) diff --git a/configure.ac b/configure.ac index ecc5baeaca..80dca168d3 100644 --- a/configure.ac +++ b/configure.ac @@ -5,8 +5,8 @@ AC_CONFIG_SRCDIR([src/config/stds.h]) AM_INIT_AUTOMAKE AC_CONFIG_HEADER(src/config/afsconfig.h) -MACOS_VERSION=1.6.23 -LINUX_PKGVER=1.6.23 +MACOS_VERSION=1.6.24 +LINUX_PKGVER=1.6.24 dnl Debian wants the release candidate version in the main upstream version, dnl and wants ~ before it. diff --git a/src/config/NTMakefile.amd64_w2k b/src/config/NTMakefile.amd64_w2k index 283a21734a..24cd10a48a 100644 --- a/src/config/NTMakefile.amd64_w2k +++ b/src/config/NTMakefile.amd64_w2k @@ -90,7 +90,7 @@ AFSPRODUCT_VER_MAJOR=1 AFSPRODUCT_VER_MINOR=6 !ENDIF !IF !DEFINED(AFSPRODUCT_VER_PATCH) -AFSPRODUCT_VER_PATCH=2300 +AFSPRODUCT_VER_PATCH=2400 !ENDIF !IF !DEFINED(AFSPRODUCT_VER_BUILD) AFSPRODUCT_VER_BUILD=0 diff --git a/src/config/NTMakefile.i386_nt40 b/src/config/NTMakefile.i386_nt40 index d03c144976..627a547529 100644 --- a/src/config/NTMakefile.i386_nt40 +++ b/src/config/NTMakefile.i386_nt40 @@ -90,7 +90,7 @@ AFSPRODUCT_VER_MAJOR=1 AFSPRODUCT_VER_MINOR=6 !ENDIF !IF !DEFINED(AFSPRODUCT_VER_PATCH) -AFSPRODUCT_VER_PATCH=2300 +AFSPRODUCT_VER_PATCH=2400 !ENDIF !IF !DEFINED(AFSPRODUCT_VER_BUILD) AFSPRODUCT_VER_BUILD=0 diff --git a/src/config/NTMakefile.i386_w2k b/src/config/NTMakefile.i386_w2k index 998c991a90..4c20879d0c 100644 --- a/src/config/NTMakefile.i386_w2k +++ b/src/config/NTMakefile.i386_w2k @@ -94,7 +94,7 @@ AFSPRODUCT_VER_MAJOR=1 AFSPRODUCT_VER_MINOR=6 !ENDIF !IF !DEFINED(AFSPRODUCT_VER_PATCH) -AFSPRODUCT_VER_PATCH=2300 +AFSPRODUCT_VER_PATCH=2400 !ENDIF !IF !DEFINED(AFSPRODUCT_VER_BUILD) AFSPRODUCT_VER_BUILD=0