From 7d0675e6c6a2f3200a3884fbe46b3ef8ef9ffd24 Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 15 Oct 2020 20:30:14 -0500 Subject: [PATCH] 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 Change-Id: I90c419a7ef0ede247187172a182863dcb4250578 Reviewed-on: https://gerrit.openafs.org/15922 Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk --- 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 53a9449a7c..63d808db78 100644 --- a/src/rx/xdr.c +++ b/src/rx/xdr.c @@ -573,7 +573,26 @@ xdr_string(XDR * xdrs, char **cpp, u_int maxsize) return (FALSE); } sp[size] = 0; - AFS_FALLTHROUGH; + + /* 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));