From 60be338c2de58f2a33c64bdb41c006574ba7fd0e Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sat, 1 Aug 2015 09:32:35 -0400 Subject: [PATCH] vlserver: ListAttributesN2 volume name safety The vlserver ListAttributesN2 RPC permits filtering the result set by volume name in addition by site or volume id. Two issues identified by Andrew Deason (Sine Nomine Associates) are addressed by this patch. First, the size of the volumename[] buffer is insufficient to store the valid input read over the network. The buffer needs to be able to store VL_MAXNAMELEN characters of the volume name, two characters for the regular expression '^' and '$', and the trailing NUL. Second, sprintf() is used to write to the buffer and even with valid input from the caller SVL_ListAttributesN2 can overflow the buffer when ".backup" and ".readonly" are appended to the volume name. If there is an overflow the search name is invalid and there can not be a valid match. This patch increases the size of volumename[] to VL_MAXNAMELEN+3. It also uses snprintf() instead of sprintf() and performs error checking. The error VL_BADNAME is returned when the network input is invalid. Reviewed-on: http://gerrit.openafs.org/11969 Reviewed-by: Mark Vitale Reviewed-by: Nathaniel Filardo Reviewed-by: Daria Brashear Tested-by: BuildBot (cherry picked from commit c9f430fd8f479bbfe28829f7032ecd325a4f833d) Change-Id: I1b48cc8ed1a52afc36465f2fbd5bfd5345e90c41 Reviewed-on: http://gerrit.openafs.org/11976 Tested-by: BuildBot Reviewed-by: Mark Vitale Reviewed-by: Michael Meffie Reviewed-by: Benjamin Kaduk Tested-by: Benjamin Kaduk Reviewed-by: Stephan Wiesand --- src/vlserver/vlprocs.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/vlserver/vlprocs.c b/src/vlserver/vlprocs.c index 2f56b19d8..7f486c828 100644 --- a/src/vlserver/vlprocs.c +++ b/src/vlserver/vlprocs.c @@ -1450,7 +1450,8 @@ SVL_ListAttributesN2(struct rx_call *rxcall, int pollcount = 0; int namematchRWBK, namematchRO, thismatch; int matchtype = 0; - char volumename[VL_MAXNAMELEN+2]; /* regex anchors */ + int size; + char volumename[VL_MAXNAMELEN+3]; /* regex anchors */ char rxstr[AFS_RXINFO_LEN]; #ifdef HAVE_POSIX_REGEX regex_t re; @@ -1519,7 +1520,11 @@ SVL_ListAttributesN2(struct rx_call *rxcall, errorcode = VL_PERM; goto done; } - sprintf(volumename, "^%s$", name); + size = snprintf(volumename, sizeof(volumename), "^%s$", name); + if (size < 0 || size >= sizeof(volumename)) { + errorcode = VL_BADNAME; + goto done; + } #ifdef HAVE_POSIX_REGEX if (regcomp(&re, volumename, REG_NOSUB) != 0) { errorcode = VL_BADNAME; @@ -1564,7 +1569,12 @@ SVL_ListAttributesN2(struct rx_call *rxcall, /* Does the name match the RW name */ if (tentry.flags & VLF_RWEXISTS) { if (findname) { - sprintf(volumename, "%s", tentry.name); + size = snprintf(volumename, sizeof(volumename), + "%s", tentry.name); + if (size < 0 || size >= sizeof(volumename)) { + errorcode = VL_BADNAME; + goto done; + } #ifdef HAVE_POSIX_REGEX if (regexec(&re, volumename, 0, NULL, 0) == 0) { thismatch = VLSF_RWVOL; @@ -1582,7 +1592,13 @@ SVL_ListAttributesN2(struct rx_call *rxcall, /* Does the name match the BK name */ if (!thismatch && (tentry.flags & VLF_BACKEXISTS)) { if (findname) { - sprintf(volumename, "%s.backup", tentry.name); + /* If this fails, the tentry.name is invalid */ + size = snprintf(volumename, sizeof(volumename), + "%s.backup", tentry.name); + if (size < 0 || size >= sizeof(volumename)) { + errorcode = VL_BADNAME; + goto done; + } #ifdef HAVE_POSIX_REGEX if (regexec(&re, volumename, 0, NULL, 0) == 0) { thismatch = VLSF_BACKVOL; @@ -1611,8 +1627,13 @@ SVL_ListAttributesN2(struct rx_call *rxcall, thismatch = ((namematchRO == 1) ? VLSF_ROVOL : 0); } else { - sprintf(volumename, "%s.readonly", - tentry.name); + /* If this fails, the tentry.name is invalid */ + size = snprintf(volumename, sizeof(volumename), + "%s.readonly", tentry.name); + if (size < 0 || size >= sizeof(volumename)) { + errorcode = VL_BADNAME; + goto done; + } #ifdef HAVE_POSIX_REGEX if (regexec(&re, volumename, 0, NULL, 0) == 0) { thismatch = VLSF_ROVOL; -- 2.39.5