commit 31cb137c931a03545e86c786180d294d90f68466 Author: Benjamin Kaduk Date: Fri Nov 8 14:12:32 2024 -0800 Make OpenAFS 1.6.25 Update version strings for the 1.6.25 release. Change-Id: I6bb0666523655ff16b9883632c1eddd624fcfcbd commit e6d1fddc354991c6dd8c2337576af2524ed25cef Author: Benjamin Kaduk Date: Fri Nov 8 14:07:44 2024 -0800 Update NEWS for OpenAFS 1.6.25 Change-Id: Ice8064c14a30405cf4e3b7db15147636c9bd52c3 commit 916ec99212a0f4ace3733be97f31b272f118dde3 Author: Andrew Deason Date: Thu Oct 15 21:07:17 2020 -0500 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 commit 74e68017f420454c873277eab868037b4ae99e6f Author: Andrew Deason Date: Fri Oct 16 10:55:15 2020 -0500 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 commit a5e85cddab167eb13813f3ef947c09798f0472ec Author: Andrew Deason Date: Fri Oct 16 10:52:03 2020 -0500 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 commit 28ccaefacebbc3ba5f646d9f41d2731b0446cb51 Author: Andrew Deason Date: Thu Oct 15 20:30:14 2020 -0500 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 commit 42723c709cbcf2d08ced65134c91abbdee8cf22a Author: Andrew Deason Date: Thu Oct 15 23:18:53 2020 -0500 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 commit 8b07563fe96e67ec012f6517476690c1a1f4b090 Author: Andrew Deason Date: Sun Oct 4 23:04:06 2020 -0500 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 commit 42730cd42b18875ded900f604c627b0fdd1fec70 Author: Andrew Deason Date: Thu Jun 13 15:30:50 2024 -0500 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 commit 9a36cf913ded91eeeb389e4a25db6a8724d8183d Author: Andrew Deason Date: Thu Jun 13 15:28:38 2024 -0500 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 commit 2ecbbe7f5bbcda1ec560041aef6b7a8ae8ce6035 Author: Michael Meffie Date: Fri Mar 10 17:51:17 2023 -0500 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 commit e8f28a7c6d6aca2ed2376d23984a80707b8e77a7 Author: Michael Meffie Date: Tue Jun 22 20:02:18 2021 -0400 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 commit 08f8b6c581d6feebedfba4556cb0b4ff573900e2 Author: Michael Meffie Date: Fri May 21 12:38:01 2021 -0400 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 commit 11706b3bcacf80862ff454c55f9ace62426d7c2f Author: Andrew Deason Date: Tue Nov 5 23:40:24 2024 -0600 OPENAFS-SA-2024-002: Avoid uninitialized memory when parsing ACLs CVE-2024-10396 Several places in the tree parse ACLs using sscanf() calls that look similar to this: sscanf(str, "%d dfs:%d %s", &nplus, &dfs, cell); sscanf(str, "%100s %d", tname, &trights); Some callers check whether the scanf() returns negative or 0, but some callers do not check the return code at all. If only some of the fields are present in the sscanf()'d string (because, for instance, the ACL is malformed), some of the arguments are left alone, and may be set to garbage if the relevant variable was never initialized. If the parsed ACL is copied to another ACL, this can result in the copied ACL containing uninitialized memory. To avoid this, make sure all of the variables passed to sscanf() and similar calls are initialized before parsing. This commit does not guarantee that the results make sense, but at least the results do not contain uninitialized memory. Reviewed-on: https://gerrit.openafs.org/15917 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit ac602a0a5624b0f0ab04df86f618d09f2a4ad063) Reviewed-on: https://gerrit.openafs.org/15938 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 21941c0ab2d28fa3a074f46e4d448d518a7c1b8a) Change-Id: I6b5af8544f5d14597b8143fa6e349b30bf604777 commit 8bba6910e1f2feb21782dd92c9a3c60c3f2a360a Author: Benjamin Kaduk Date: Mon Nov 4 20:50:50 2024 -0800 OPENAFS-SA-2024-002: make VIOCGETAL consumers stay within string bounds CVE-2024-10396 After the preceding commits, the data returned by the VIOCGETAL pioctl (a RXAFS_FetchAcl wrapper) will safely be NUL-terminated. However, the callers that attempt to parse the ACL string make assumptions that the returned data will be properly formatted, and implement a "skip to next line" functionality (under various names) that blindly increments a char* until it finds a newline character, which can read past the end of even a properly NUL-terminated string if there is not a newline where one is expected. Adjust the various "skip to next line" functionality to keep the current string pointer at the trailing NUL if the end of the string is reached while searching for a newline. Reviewed-on: https://gerrit.openafs.org/15916 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit a4ecb050540528a1bff840ff08d21f99e6ef3fbf) Reviewed-on: https://gerrit.openafs.org/15937 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit a9ede52673b8c8abbfc2577ac6987a8a5686206f) Change-Id: I2e22cc3613d9e182418a0a79ab584cc3b317c6a5 commit d559dad5a76ca977e643c870f413890e8dee7e80 Author: Benjamin Kaduk Date: Mon Nov 4 20:50:50 2024 -0800 OPENAFS-SA-2024-002: verify FetchACL returned only a string CVE-2024-10396 Supplement the previous commit by additionally verifying that the returned ACL string occupies the entire XDR opaque, rejecting any values returned that have an internal NUL prior to the end of the opaque. Reviewed-on: https://gerrit.openafs.org/15915 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 7e13414e8ea995d438cde3e60988225f3ab4cbcd) Reviewed-on: https://gerrit.openafs.org/15936 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit a96a3160f5425125588f39f5ac612df3ef9b9a8a) Change-Id: I7cb16fa73e4b1ad511cdb3b2c2d55aa7e93a1f7b commit ab1ac1cfeba4a7a74499407319e8120737d7351c Author: Benjamin Kaduk Date: Mon Nov 4 20:33:16 2024 -0800 OPENAFS-SA-2024-002: verify FetchACL returned a valid string CVE-2024-10396 Analogously to how a call to RXAFS_StoreACL() with a malformed ACL string can cause a fileserver to perform invalid memory operations, a malformed ACL string returned in response to a call to RXAFS_FetchACL() can cause a client to perform invalid memory operations. Modify all the in-tree callers of the RPC to verify that the ACL data, which is conveyed as an XDR 'opaque' but whose contents are actually expected to be a string, is a valid C string. If a zero-length opaque or one without a trailing NUL is received, treat that as an error response from the fileserver rather than returning success. The Unix cache manager's pioctl handler already has logic to cope with a zero-length reply by emitting a single NUL byte to userspace. This special-casing seems to have been in place from the original IBM import, though it does so by confusingly "skipping over" a NUL byte already put in place. For historical compatibility, preserve that behavior rather than treating the zero-length reply as an error as we do for the other callers. It seems likely that this location should treat a zero-length reply as an error just as the other call sites do, but that can be done as a later change. Reviewed-on: https://gerrit.openafs.org/15914 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 0b1ccb0dbc3b7673558eceff3d672971f5bb0197) Reviewed-on: https://gerrit.openafs.org/15935 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 64068705b15661a8d4e0b9f9f2ad4aec34ed51a7) Change-Id: I2a26ac00fd62466c4ebaf1323fc03ff236b15bee commit 1db8e2f9c720ee2dda6eb65d4460137f3d92cbb2 Author: Andrew Deason Date: Wed Aug 21 00:41:49 2024 -0500 OPENAFS-SA-2024-002: viced: Avoid unchecked ACL in StoreACL audit log CVE-2024-10396 Currently in SRXAFS_StoreACL, if CallPreamble() or check_acl() fail, we will jump to Bad_StoreACL, which will pass the ACL string from the client to osi_auditU. Since check_acl() hasn't yet checked if the given ACL contains a NUL byte, the ACL may be an unterminated string. If auditing is enabled, this can cause garbage to be logged to the audit log, or cause the fileserver to crash. To avoid this, set 'rawACL' to NULL at first, only setting it to the actual ACL string after check_acl() has succeeded. This ensures that all code accessing 'rawACL' is guaranteed to be using a terminated string. This may mean that we pass a NULL AUD_ACL to osi_auditU. Our auditing code explicitly checks for and handles handles NULL strings, so this is fine. FIXES 135445 Reviewed-on: https://gerrit.openafs.org/15913 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit c9eae1e8b26144063e5d1db23d47ee82c4b9ef3a) Reviewed-on: https://gerrit.openafs.org/15934 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit bb01d76a2095baa65880bdc5d504e7a198958265) Change-Id: I2e078a2642b4b8d9c66bcebaee4f1977eef05ca5 commit 4ffe7b60f1510affaad43cd152098e54e8831580 Author: Andrew Deason Date: Wed Aug 21 00:29:34 2024 -0500 OPENAFS-SA-2024-002: viced: Introduce 'rawACL' in StoreACL CVE-2024-10396 Change our StoreACL implementation to refer to the 'AccessList' argument via a new local variable called 'rawACL'. This makes it clearer to users that the data is a string, and makes it easier for future commits to make sure we don't access the 'AccessList' argument in certain situations. Update almost all users in StoreACL to refer to 'rawACL' instead of 'AccessList'. Change the name of 'AccessList' to 'uncheckedACL' to make sure we don't miss any users. Update our check_acl() call to use 'uncheckedACL' (and not 'rawACL'), because it must use an AFSOpaque to check the ACL. Change RXStore_AccessList() and printableACL() to accept a plain char* instead of a struct AFSOpaque. This commit should not incur any noticeable behavior change. Technically printableACL() is changed to run strlen() on the given string, but this should not cause any noticeable change in behavior: This change could cause printableACL() to process less of the string than before, if the string contains a NUL byte before the end of the AFSOpaque buffer. But this doesn't matter, since the all of our code after this treats the ACL as a plain string, and so doesn't look at any data beyond the first NUL. It's not possible for printableACL() to process more data than before, because check_acl() has already checked that the ACL string contains a NUL byte, so we must process AFSOpaque_len bytes or fewer. FIXES 135445 Reviewed-on: https://gerrit.openafs.org/15912 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit eb8b93a971c6293cdfbf8cd3d9a6351a8cb76f81) [1.8: printableACL() does not exist in this branch.] Reviewed-on: https://gerrit.openafs.org/15933 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit ee020f7cba7d82bc3d4b468210b5052af53c5db5) Change-Id: Icf18d600168b7ba5e2356e86479597ae650f67fa commit 32ef2dcedb78832e7d40e5e1997ccdb61a4d0e2d Author: Andrew Deason Date: Tue Sep 19 15:55:42 2023 -0500 OPENAFS-SA-2024-002: acl: Error on missing newlines when parsing ACL CVE-2024-10396 In acl_Internalize_pr(), each line in an ACL granting rights (positive or negative) is sscanf()'d with "%63s\t%d\n", and then we try to advance 'nextc' beyond the next newline character. However, sscanf()'ing "%63s\t%d\n" does not guarantee that there is a newline in the given string. Whitespace characters in sscanf() are not matched exactly, and may match any amount of whitespace (including none at all). For example, a string like "foo 4" may be parsed by sscanf(), but does not contain any newlines. If this happens, strchr(nextc, '\n') will return NULL, and we'll advance 'nextc' to 0x1, causing a segfault when we next try to dereference 'nextc'. To avoid this, check if 'nextc' is NULL after the strchr() call, and return an error if so. FIXES 135445 Reviewed-on: https://gerrit.openafs.org/15911 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 96ab2c6f8a614d597a523b45871c5f64a50a7040) Reviewed-on: https://gerrit.openafs.org/15932 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit d66caf8c04878724001839317637445708edef2c) Change-Id: Ie904de0abdb72d812abeca92b3bd9cda667c63a4 commit a1a1c258c9c9825e038eec0df923b77c4728f6a9 Author: Andrew Deason Date: Tue Sep 19 15:44:08 2023 -0500 OPENAFS-SA-2024-002: acl: Do not parse beyond end of ACL CVE-2024-10396 The early parsing code in acl_Internalize_pr() tries to advance 'nextc' to go beyond the first two newlines in the given ACL string. But if the given ACL string has no newlines, or only 1 newline, then 'nextc' will point beyond the end of the ACL string, potentially pointing to garbage. Intuitively, it may look like the ACL string must contain at least 2 newlines because we have sscanf()'d the string with "%d\n%\d". However, whitespace characters in sscanf() are not matched exactly like non-whitespace characters are; a sequence of whitespace characters matches any amount of whitespace (including none). So, a string like "1 2" will be parsed by "%d\n%d\n", but will not contain any newline characters. Usually this should result in a parse error from acl_Internalize_pr(), but if the garbage happens to parse successfully, this could result in unrelated memory getting stored to the ACL. To fix this, don't advance 'nextc' if we're already at the end of the ACL string. FIXES 135445 Reviewed-on: https://gerrit.openafs.org/15910 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 35d218c1d17973c1412ea5dff1e23d9aae50c4c7) Reviewed-on: https://gerrit.openafs.org/15931 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 1e6e813188ecce62eb7af19385d911f63469bdb6) Change-Id: Id70301ae16ab0d2aa0b7183cd9b469d7ff0b889a commit cbc56aaa8af0b422c9545e66b1823e73244d2535 Author: Andrew Deason Date: Mon Sep 18 16:14:07 2023 -0500 OPENAFS-SA-2024-002: viced: Free ACL on acl_Internalize_pr error CVE-2024-10396 Currently, we don't free 'newACL' if acl_Internalize_pr() fails. If acl_Internalize_pr() has already allocated 'newACL', then the memory associated with newACL will be leaked. This can happen if parsing the given ACL fails at any point after successfully parsing the first couple of lines in the ACL. Change acl_FreeACL() to make freeing a NULL acl a no-op, to make it easier to make sure the acl has been freed. FIXES 135445 Reviewed-on: https://gerrit.openafs.org/15909 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit f4dfc2d7183f126bc4a45b5cabc78c3de020925f) Reviewed-on: https://gerrit.openafs.org/15930 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit a07e50726df09c49dfe7b953c3e49eb98f310c09) Change-Id: I24fc132a91ba9867f7c5ba0fdbd5ae150bc358a7 commit 19f8f35949cf8fc7e6b91afc0d29deb0d4db4d99 Author: Andrew Deason Date: Mon Sep 18 16:13:57 2023 -0500 OPENAFS-SA-2024-002: viced: Refuse ACLs without '\0' in SRXAFS_StoreACL CVE-2024-10396 Currently, the fileserver treats the ACL given in RXAFS_StoreACL as a string, even though it is technically an AFSOpaque and could be not NUL-terminated. We give the ACL opaque/string to acl_Internalize_pr() to parse, which will run off the end of the allocated buffer if the given ACL does not contain a '\0' character. Usually this will result in a parse error since we'll encounter garbage, but if the partially-garbage ACL happens to parse successfully, some uninitialized data could make it into the stored ACL. In addition, if the given ACL is an opaque of length 0, we'll still give the opaque pointer to acl_Internalize_pr(). In this case, the pointer will point to &memZero, which happens to contain a NUL byte, and so is treated like an empty string (which is not a valid ACL). But the fact that this causes no problems is somewhat a coincidence, and so should also be avoided. To avoid both of these situations, just check if the given ACL string contains a NUL byte. If it doesn't, or if it has length 0, refuse to look at it and abort the call with EINVAL. FIXES 135445 Reviewed-on: https://gerrit.openafs.org/15908 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit e15decb318797f1d471588dc669c3e3b26f1b8b3) Reviewed-on: https://gerrit.openafs.org/15929 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit f74f960a18f559e683d6a1f5104e43c3ca93ecb8) Change-Id: I451b0c7b1e983c464dc05ed6bf7ef603bd7ad3ac commit 7722383313efdd0e749b42efd14c465347f0b73b Author: Andrew Deason Date: Fri Jan 10 12:40:15 2020 -0600 OPENAFS-SA-2024-001: afs: Throttle PAG creation in afs_genpag() CVE-2024-10394 Currently, we only throttle PAG creation in afs_setpag(). But there are several callers that call setpag() directly, not via afs_setpag; notably _settok_setParentPag in afs_pioctl.c. When setpag() is called with a PAG value of -1, it generates a new PAG internally without any throttling. So, those callers effectively bypass the PAG throttling mechanism, which allows a calling user to create PAGs without any delay. To avoid this, move our afs_pag_wait call from afs_setpag() to afs_genpag(), which all code uses to generate a new PAG value. This ensures that PAG creation is always throttled for unprivileged users. FIXES 135062 Reviewed-on: https://gerrit.openafs.org/15907 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 0358648dbed7656e7bda30f6f0ea6e8e01bf6527) Reviewed-on: https://gerrit.openafs.org/15928 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 57b655e4837d8660ebcc25d95efb09118adaff07) [1.6: In this branch, afs_pag_wait() cannot return an error, and afs_setpag() assumes it will not return an error.] Change-Id: I1d678f24ded708200f7d14061f9a874df7fbbb2c commit 62ea1fe973eaf1652b52d613c24a9a0a544d3ae4 Author: Andrew Deason Date: Fri Jan 10 12:01:50 2020 -0600 OPENAFS-SA-2024-001: afs: Introduce afs_genpag() CVE-2024-10394 Currently, several areas in the code call genpag() to generate a new PAG id, but the signature of genpag() is very limited. To allow for the code in genpag() to return errors and to examine the calling user's credentials, introduce a new function, afs_genpag(), that does the same thing as genpag(), but accepts creds and allows errors to be returned. Convert all existing callers to use afs_genpag() and to handle any errors, though no errors are ever returned in this commit on its own. To ensure there are no old callers of genpag() left around, change the existing genpag() to be called genpagval(), and declare it static. FIXES 135062 Reviewed-on: https://gerrit.openafs.org/14090 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit f701f704c7bc93cf5fd7cffaaa043cef6a99e77f) Reviewed-on: https://gerrit.openafs.org/15927 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk (cherry picked from commit 20c22347b41eea2ebbdc0ab15f16c822af44df51) [1.6: In this branch, afs_genpag() never returns an error, and afs_setpag() is not able to handle errors from afs_genpag(). So, afs_setpag() does not check for errors from afs_genpag() in this branch. Also, the signature of afs_genpag() says it accepts an afs_ucred_t** (not an afs_ucred_t*), even though 'acred' must actually be an 'afs_ucred_t*'. Though this is confusing, we keep it this way to match afs_pag_wait() and existing callers. Add some casts to avoid warnings in some places.] Change-Id: I2e7ab827d15d33dbee3195068da6c87b38f30695