Merge pull request #110 from garlick/issue#109

handle some malformed 9P requests gracefully

diod/xattr.c

@@ -21,6 +21,7 @@
 #include <string.h>
 #include <inttypes.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <pthread.h>
 #include <errno.h>
 #include <sys/types.h>
@@ -106,39 +107,36 @@ static Xattr _xattr_create (Npstr *name, size_t size, int flags, u32 setflags)
     return NULL;
 }
 
+static bool
+bounds_check (size_t bufsize, size_t count, off_t offset)
+{
+    if (offset < 0 || offset > bufsize || count > bufsize - offset)
+        return false;
+    return true;
+}
 
 int
 xattr_pwrite (Xattr x, void *buf, size_t count, off_t offset)
 {
-    int len = count;
-
-    if (!(x->flags & XATTR_FLAGS_SET)) {
-        np_uerror (EINVAL);
+    if (!(x->flags & XATTR_FLAGS_SET)
+        || !bounds_check (x->len, count, offset)) {
+        errno = EINVAL;
         return -1;
     }
-    if (len > x->len - offset)
-        len = x->len - offset;
-    if (len < 0)
-        len = 0;
-    memcpy (x->buf + offset, buf, len);
-    return len;
+    memcpy (x->buf + offset, buf, count);
+    return count;
 }
 
 int
 xattr_pread (Xattr x, void *buf, size_t count, off_t offset)
 {
-    int len = x->len - offset;
-
-    if (!(x->flags & XATTR_FLAGS_GET)) {
-        np_uerror (EINVAL);
+    if (!(x->flags & XATTR_FLAGS_GET)
+        || !bounds_check (x->len, count, offset)) {
+        errno = EINVAL;
         return -1;
     }
-    if (len > count)
-        len = count;
-    if (len < 0)
-        len = 0;
-    memcpy (buf, x->buf + offset, len);
-    return len;
+    memcpy (buf, x->buf + offset, count);
+    return count;
 }
 
 static int

libnpfs/np.c

@@ -47,7 +47,7 @@ static inline int
 buf_check_size(struct cbuf *buf, int len)
 {
 	if (buf->p+len > buf->ep) {
-		if (buf->p < buf->ep)
+		if (buf->p <= buf->ep)
 			buf->p = buf->ep + 1;
 
 		return 0;

tests/misc/t07.exp

@@ -122,3 +122,4 @@ test_tremove(122): 11
 P9_TREMOVE tag 42 fid 1
 test_rremove(123): 7
 P9_RREMOVE tag 42
+OK np_deserialize failed on truncated Tlink (issue#109)

tests/misc/tserialize.c

@@ -65,6 +65,8 @@ static void test_twrite (void);         static void test_rwrite (void);
 static void test_tclunk (void);         static void test_rclunk (void);
 static void test_tremove (void);        static void test_rremove (void);
 
+static void test_truncated_tlink (void);
+
 static void
 usage (void)
 {
@@ -112,6 +114,8 @@ main (int argc, char *argv[])
     test_tclunk ();     test_rclunk ();
     test_tremove ();    test_rremove ();
 
+    test_truncated_tlink ();
+
     exit (0);
 }
 
@@ -1194,6 +1198,28 @@ test_rremove (void)
     free (fc2);
 }
 
+static void test_truncated_tlink (void)
+{
+    Npfcall *fc;
+
+    if (!(fc = np_create_tlink (1, 2, "xyz")))
+        msg_exit ("out of memory");
+
+    // truncate fc->pkt so entire name is missing
+    fc->size -= 3;
+    fc->pkt[0] = fc->size;
+    fc->pkt[1] = fc->size >> 8;
+    fc->pkt[2] = fc->size >> 16;
+    fc->pkt[3] = fc->size >> 24;
+
+    if (np_deserialize (fc) == 0)
+        printf ("OK np_deserialize failed on truncated Tlink (issue#109)\n");
+    else
+        printf ("FAIL np_deserialize worked on truncated Tlink (issue#109)\n");
+
+    free (fc);
+}
+
 /*
  * vi:tabstop=4 shiftwidth=4 expandtab
  */

tests/user/Makefile.am

@@ -9,6 +9,7 @@ check_PROGRAMS = \
 	tflush \
 	tgetxattr \
 	tsetxattr \
+	tsetxattr_wildoffset \
 	tremovexattr \
 	txattr \
 	testopenfid

tests/user/t19

@@ -6,4 +6,5 @@ touch $PATH_EXPDIR/testfile
 ./tsetxattr "$@" testfile +user.foo bazval && exit 1
 ./tremovexattr "$@" testfile user.foo
 ./tsetxattr "$@" testfile /user.foo bazval && exit 1
+./tsetxattr_wildoffset "$@" testfile user.bar barval
 exit 0

tests/user/t19.exp

@@ -1,4 +1,5 @@
 tsetxattr: npc_setxattr user.foo=bazval: File exists
 tsetxattr: npc_setxattr user.foo=bazval: No data available
+tsetxattr_wildoffset: OK: Twrite with crazy offset failed with Invalid argument
 conjoin: t19 exited with rc=0
 conjoin: diod exited with rc=0

tests/user/tsetxattr_wildoffset.c

@@ -0,0 +1,100 @@
+/************************************************************\
+ * Copyright 2010 Lawrence Livermore National Security, LLC
+ * (c.f. AUTHORS, NOTICE.LLNS, COPYING)
+ *
+ * This file is part of the diod 9P server project.
+ * For details, see https://github.com/chaos/diod.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+\************************************************************/
+
+/* tsetxattr_wildoffset.c - pass a crazy Twrite offset to an xattr */
+
+#if HAVE_CONFIG_H
+#include "config.h"
+#endif
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <pthread.h>
+#include <errno.h>
+#include <stdint.h>
+#include <inttypes.h>
+#include <stdarg.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <libgen.h>
+#if HAVE_SYS_XATTR_H
+    #include <sys/xattr.h>
+#else
+    #include <attr/xattr.h>
+#endif
+
+
+#include "9p.h"
+#include "npfs.h"
+#include "npclient.h"
+
+#include "diod_log.h"
+#include "diod_auth.h"
+
+static void
+usage (void)
+{
+    fprintf (stderr, "Usage: tsetxattr_wildoffset aname path attr value\n");
+    exit (1);
+}
+
+int
+main (int argc, char *argv[])
+{
+    Npcfsys *fs;
+    Npcfid *afid, *root, *fid;
+    uid_t uid = geteuid ();
+    char *aname, *path, *attr, *value;
+    u64 offset = 2684469248;
+    int fd = 0; /* stdin */
+    int n;
+
+    diod_log_init (argv[0]);
+
+    if (argc != 5)
+        usage ();
+    aname = argv[1];
+    path = argv[2];
+    attr = argv[3];
+    value = argv[4];
+
+    if (!(fs = npc_start (fd, fd, 65536+24, 0)))
+        errn_exit (np_rerror (), "npc_start");
+    if (!(afid = npc_auth (fs, aname, uid, diod_auth)) && np_rerror () != 0)
+        errn_exit (np_rerror (), "npc_auth");
+    if (!(root = npc_attach (fs, afid, aname, uid)))
+        errn_exit (np_rerror (), "npc_attach");
+    if (afid && npc_clunk (afid) < 0)
+        errn (np_rerror (), "npc_clunk afid");
+    if (!(fid = npc_walk (root, path)))
+        errn (np_rerror (), "npc_walk %s", path);
+    if (npc_xattrcreate (fid, attr, strlen (value), XATTR_CREATE) < 0)
+        errn_exit (np_rerror (), "npc_xattrcreate");
+    n = npc_pwrite (fid, value, strlen (value), offset);
+    if (n >= 0)
+        msg_exit ("FAIL: Twrite with crazy offset to xattr succeeded");
+    else {
+        msg ("OK: Twrite with crazy offset failed with %s",
+             strerror (np_rerror ()));
+    }
+    if (npc_clunk (fid) < 0)
+        errn_exit (np_rerror (), "npc_clunk %s", path);
+    if (npc_clunk (root) < 0)
+        errn_exit (np_rerror (), "npc_clunk root");
+    npc_finish (fs);
+
+    exit (0);
+}
+
+/*
+ * vi:tabstop=4 shiftwidth=4 expandtab
+ */