[PATCH 0/3] gssd, idmapd: validate names before using them

[PATCH 0/3] gssd, idmapd: validate names before using them

[Prabhakar Pujeri]

These fixes validate externally supplied names before treating them as C
strings.  They correct an uninitialized read in an error path, keep idmapd
inside the fixed NAMETOID field, and reject server hostnames that do not
fit in gssd's destination buffer.

The series is based on current nfs-utils master.  A fresh autotools build
and make check complete successfully; the statd integration test is skipped
when its required service environment is unavailable.

Prabhakar Pujeri (3):
  gssd: fix use of uninitialized memory in error message
  idmapd: validate NAMETOID names within their fixed-size field
  gssd: reject server hostnames that do not fit

 utils/gssd/gssd.c      | 2 +-
 utils/gssd/krb5_util.c | 9 +++++++--
 utils/idmapd/idmapd.c  | 7 +++----
 3 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.55.0

[PATCH 1/3] gssd: fix use of uninitialized memory in error message

[Prabhakar Pujeri]

gssd_get_topdir() prints tdi->name in the inotify_add_watch() error
path, but that flexible array member is only filled in by the strcpy()
after the call.  The message ends up dumping uninitialized heap memory
and can even read past the allocation if no NUL byte is nearby.

Print the name argument passed in by the caller instead; it is exactly
the directory the watch was being added for.

Signed-off-by: Prabhakar Pujeri <prabhakar.pujeri@dell.com>
---
 utils/gssd/gssd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index bad92321..a0a65014 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -769,7 +769,7 @@ gssd_get_topdir(const char *name)
 	tdi->wd = inotify_add_watch(inotify_fd, name, IN_CREATE);
 	if (tdi->wd < 0) {
 		printerr(0, "ERROR: %s: inotify_add_watch failed for top dir %s: %s\n",
-			 __FUNCTION__, tdi->name, strerror(errno));
+			 __FUNCTION__, name, strerror(errno));
 		free(tdi);
 		return NULL;
 	}
-- 
2.55.0

[PATCH 2/3] idmapd: validate NAMETOID names within their fixed-size field

[Prabhakar Pujeri]

imconv() limits len to IDMAP_NAMESZ - 1 and then reads
im_name[len + 1].  An unterminated name makes this read beyond im_name,
while a terminated name can be rejected based on the unrelated byte after
its NUL character.

Use strnlen() to require a terminator within im_name.  Mark an
unterminated request as invalid before returning it to the kernel.

Signed-off-by: Prabhakar Pujeri <prabhakar.pujeri@dell.com>
---
 utils/idmapd/idmapd.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index 5231f56d..c41ae1f0 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -767,8 +767,6 @@ nfsdcb(int UNUSED(fd), short which, void *data)
 static void
 imconv(struct idmap_client *ic, struct idmap_msg *im)
 {
-	u_int32_t len;
-
 	switch (im->im_conv) {
 	case IDMAP_CONV_IDTONAME:
 		idtonameres(im);
@@ -779,10 +777,11 @@ imconv(struct idmap_client *ic, struct idmap_msg *im)
 			    im->im_id, im->im_name);
 		break;
 	case IDMAP_CONV_NAMETOID:
-		len = strnlen(im->im_name, IDMAP_NAMESZ - 1);
 		/* Check for NULL termination just to be careful */
-		if (im->im_name[len+1] != '\0')
+		if (strnlen(im->im_name, IDMAP_NAMESZ) == IDMAP_NAMESZ) {
+			im->im_status |= IDMAP_STATUS_INVALIDMSG;
 			return;
+		}
 		nametoidres(im);
 		if (verbose > 1)
 			xlog_warn("%s %s: (%s) name \"%s\" -> id \"%d\"",
-- 
2.55.0

[PATCH 3/3] gssd: reject server hostnames that do not fit

[Prabhakar Pujeri]

find_keytab_entry() copies a hostname from the kernel upcall into a fixed
NI_MAXHOST buffer with strcpy(), allowing an overlong name to overwrite
the stack.

Use snprintf() and check its result.  Reject a truncated hostname with
ENAMETOOLONG instead of performing a keytab lookup with a different name.
The verified string also fits in the following myhostad copy.

Signed-off-by: Prabhakar Pujeri <prabhakar.pujeri@dell.com>
---
 utils/gssd/krb5_util.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index bc063a8b..7ad67131 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -841,8 +841,13 @@ find_keytab_entry(krb5_context context, krb5_keytab kt,
 
 	/* Get full local hostname */
 	if (srchost) {
-		strcpy(myhostname, srchost);
-	        strcpy(myhostad, myhostname);
+		retval = snprintf(myhostname, sizeof(myhostname), "%s", srchost);
+		if (retval < 0 || (size_t)retval >= sizeof(myhostname)) {
+			retval = ENAMETOOLONG;
+			printerr(1, "source hostname is too long\n");
+			goto out;
+		}
+		strcpy(myhostad, myhostname);
 	} else {
 		/* Borrow myhostad for gethostname(), we need it later anyways */
 		if (gethostname(myhostad, sizeof(myhostad)-1) == -1) {
-- 
2.55.0