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.

Change-Id: Id65b83e0dd14c6f41af73c6868975ae53c4975a7
Reviewed-on: http://gerrit.openafs.org/11969
Reviewed-by: Mark Vitale <mvitale@sinenomine.net>
Reviewed-by: Nathaniel Filardo <nwfilardo@gmail.com>
Reviewed-by: Daria Brashear <shadow@your-file-system.com>
Tested-by: BuildBot <buildbot@rampaginggeek.com>

diff --git a/src/vlserver/vlprocs.c b/src/vlserver/vlprocs.c
index 6a72c94..7eb99fe 100644 (file)
--- a/src/vlserver/vlprocs.c
+++ b/src/vlserver/vlprocs.c
@@ -1660,7 +1660,8 @@ 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;
@@ -1733,7 +1734,11 @@ ListAttributesN2(struct rx_call *rxcall,
                code = VL_PERM;
                goto done;
            }
-           sprintf(volumename, "^%s$", name);
+           size = snprintf(volumename, sizeof(volumename), "^%s$", name);
+           if (size < 0 || size >= sizeof(volumename)) {
+               code = VL_BADNAME;
+               goto done;
+           }
 #ifdef HAVE_POSIX_REGEX
            if (regcomp(&re, volumename, REG_NOSUB) != 0) {
                code = VL_BADNAME;
@@ -1778,7 +1783,12 @@ 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)) {
+                               code = VL_BADNAME;
+                               goto done;
+                           }
 #ifdef HAVE_POSIX_REGEX
                            if (regexec(&re, volumename, 0, NULL, 0) == 0) {
                                thismatch = VLSF_RWVOL;
@@ -1796,7 +1806,13 @@ 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)) {
+                               code = VL_BADNAME;
+                               goto done;
+                           }
 #ifdef HAVE_POSIX_REGEX
                            if (regexec(&re, volumename, 0, NULL, 0) == 0) {
                                thismatch = VLSF_BACKVOL;
@@ -1825,8 +1841,13 @@ 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)) {
+                                   code = VL_BADNAME;
+                                   goto done;
+                               }
 #ifdef HAVE_POSIX_REGEX
                            if (regexec(&re, volumename, 0, NULL, 0) == 0) {
                                thismatch = VLSF_ROVOL;