From 11eb8ba870457337c6067284dde4277e09764c0a Mon Sep 17 00:00:00 2001 From: Pranith Kumar K Date: Wed, 12 Jul 2017 09:18:02 +0530 Subject: storage/posix: Don't allow gfid/volume-id xattr to be removed Problem: Bulk xattr removal doesn't check if the xattrs that are coming in xdata have gfid/volume-id xattrs, so there is potential for bulkremovexattr removing gfid/volume-id. I also observed that bulkremovexattr is not available for fremovexattr. Fix: Do proper checks in bulk removexattr to remove gfid/volume-id. Refactor [f]removexattr to reduce the differences. BUG: 1470489 Change-Id: Ia845b31846a149500111c0996646e648f72cdce6 Signed-off-by: Pranith Kumar K Reviewed-on: https://review.gluster.org/17765 Smoke: Gluster Build System Reviewed-by: Jeff Darcy CentOS-regression: Gluster Build System Reviewed-by: Anuradha Talur Reviewed-by: Niels de Vos Reviewed-by: Krutika Dhananjay --- libglusterfs/src/dict.c | 28 +++ libglusterfs/src/dict.h | 2 + .../posix/disallow-gfid-volumeid-fremovexattr.c | 98 +++++++++ .../posix/disallow-gfid-volumeid-fremovexattr.t | 21 ++ .../posix/disallow-gfid-volumeid-removexattr.t | 26 +++ xlators/storage/posix/src/posix-helpers.c | 28 +-- xlators/storage/posix/src/posix.c | 244 ++++++++++----------- xlators/storage/posix/src/posix.h | 2 + 8 files changed, 309 insertions(+), 140 deletions(-) create mode 100644 tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.c create mode 100755 tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.t create mode 100644 tests/bugs/posix/disallow-gfid-volumeid-removexattr.t diff --git a/libglusterfs/src/dict.c b/libglusterfs/src/dict.c index 04d627dde39..aa06dcb2a8d 100644 --- a/libglusterfs/src/dict.c +++ b/libglusterfs/src/dict.c @@ -3077,3 +3077,31 @@ dict_for_key_value (const char *name, const char *value, size_t size) return xattr; } + +/* + * "strings" should be NULL terminated strings array. + */ +int +dict_has_key_from_array (dict_t *dict, char **strings, gf_boolean_t *result) +{ + int i = 0; + uint32_t hash = 0; + + if (!dict || !strings || !result) + return -EINVAL; + + LOCK (&dict->lock); + { + for (i = 0; strings[i]; i++) { + hash = SuperFastHash (strings[i], strlen (strings[i])); + if (dict_lookup_common (dict, strings[i], hash)) { + *result = _gf_true; + goto unlock; + } + } + *result = _gf_false; + } +unlock: + UNLOCK (&dict->lock); + return 0; +} diff --git a/libglusterfs/src/dict.h b/libglusterfs/src/dict.h index bdc003ea373..b1ba3c20234 100644 --- a/libglusterfs/src/dict.h +++ b/libglusterfs/src/dict.h @@ -263,4 +263,6 @@ are_dicts_equal (dict_t *one, dict_t *two, gf_boolean_t (*match) (dict_t *d, char *k, data_t *v, void *data), gf_boolean_t (*value_ignore) (char *k)); +int +dict_has_key_from_array (dict_t *dict, char **strings, gf_boolean_t *result); #endif diff --git a/tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.c b/tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.c new file mode 100644 index 00000000000..325edbbed97 --- /dev/null +++ b/tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.c @@ -0,0 +1,98 @@ +#include +#include +#include +#include +#include + +int +main (int argc, char *argv[]) +{ + glfs_t *fs = NULL; + int ret = 0; + int i = 0; + glfs_fd_t *fd = NULL; + char *logfile = NULL; + char *hostname = NULL; + + if (argc != 4) { + fprintf (stderr, + "Expect following args %s \n" + , argv[0]); + return -1; + } + + hostname = argv[1]; + logfile = argv[3]; + + fs = glfs_new (argv[2]); + if (!fs) { + fprintf (stderr, "glfs_new: returned NULL (%s)\n", + strerror (errno)); + return -1; + } + + ret = glfs_set_volfile_server (fs, "tcp", hostname, 24007); + if (ret < 0) { + fprintf (stderr, "glfs_set_volfile_server failed ret:%d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_set_logging (fs, logfile, 7); + if (ret < 0) { + fprintf (stderr, "glfs_set_logging failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + ret = glfs_init (fs); + if (ret < 0) { + fprintf (stderr, "glfs_init failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + + fd = glfs_opendir (fs, "/"); + if (!fd) { + fprintf (stderr, "glfs_opendir failed with (%s)\n", + strerror (errno)); + return -1; + } + + ret = glfs_fremovexattr (fd, "trusted.gfid"); + if (ret == 0 || errno != EPERM) { + fprintf (stderr, "glfs_fremovexattr gfid exited with ret: " + "%d (%s)\n", ret, strerror (errno)); + return -1; + } + + ret = glfs_fremovexattr (fd, "trusted.glusterfs.volume-id"); + if (ret == 0 || errno != EPERM) { + fprintf (stderr, "glfs_fremovexattr volume-id exited with ret: " + "%d (%s)\n", ret, strerror (errno)); + return -1; + } + + ret = glfs_fsetxattr (fd, "trusted.abc", "abc", 3, 0); + if (ret < 0) { + fprintf (stderr, "glfs_fsetxattr trusted.abc exited with ret: " + "%d (%s)\n", ret, strerror (errno)); + return -1; + } + + ret = glfs_fremovexattr (fd, "trusted.abc"); + if (ret < 0) { + fprintf (stderr, "glfs_fremovexattr trusted.abc exited with " + "ret: %d (%s)\n", ret, strerror (errno)); + return -1; + } + + (void) glfs_closedir(fd); + ret = glfs_fini (fs); + if (ret < 0) { + fprintf (stderr, "glfs_fini failed with ret: %d (%s)\n", + ret, strerror (errno)); + return -1; + } + return 0; +} diff --git a/tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.t b/tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.t new file mode 100755 index 00000000000..b9fd44ae0d7 --- /dev/null +++ b/tests/bugs/posix/disallow-gfid-volumeid-fremovexattr.t @@ -0,0 +1,21 @@ +#!/bin/bash +. $(dirname $0)/../../include.rc +. $(dirname $0)/../../volume.rc + +cleanup; + +## Start and create a volume +TEST glusterd; +TEST pidof glusterd; +TEST $CLI volume info; + +TEST $CLI volume create $V0 $H0:$B0/${V0}; +TEST $CLI volume start $V0; +logdir=`gluster --print-logdir` + + +TEST build_tester $(dirname $0)/disallow-gfid-volumeid-fremovexattr.c -lgfapi +TEST $(dirname $0)/disallow-gfid-volumeid-fremovexattr $H0 $V0 $logdir/disallow-gfid-volumeid-fremovexattr.log + +cleanup_tester $(dirname $0)/disallow-gfid-volumeid-fremovexattr +cleanup; diff --git a/tests/bugs/posix/disallow-gfid-volumeid-removexattr.t b/tests/bugs/posix/disallow-gfid-volumeid-removexattr.t new file mode 100644 index 00000000000..d26eb21ccc5 --- /dev/null +++ b/tests/bugs/posix/disallow-gfid-volumeid-removexattr.t @@ -0,0 +1,26 @@ +#!/bin/bash + +#This test checks that gfid/volume-id removexattrs are not allowed. +. $(dirname $0)/../../include.rc +. $(dirname $0)/../../volume.rc + +cleanup; + +#Basic checks +TEST glusterd +TEST pidof glusterd +TEST $CLI volume info + +#Create a distributed volume +TEST $CLI volume create $V0 $H0:$B0/${V0}{1..2}; +TEST $CLI volume start $V0 + +# Mount FUSE +TEST glusterfs -s $H0 --volfile-id $V0 $M0 + +TEST ! setfattr -x trusted.gfid $M0 +TEST ! setfattr -x trusted.glusterfs.volume-id $M0 +TEST setfattr -n trusted.abc -v abc $M0 +TEST setfattr -x trusted.abc $M0 + +cleanup; diff --git a/xlators/storage/posix/src/posix-helpers.c b/xlators/storage/posix/src/posix-helpers.c index 8c1a789f70b..53c8a86101c 100644 --- a/xlators/storage/posix/src/posix-helpers.c +++ b/xlators/storage/posix/src/posix-helpers.c @@ -148,26 +148,10 @@ out: return ret; } - -static gf_boolean_t -_is_in_array (char **str_array, char *str) -{ - int i = 0; - - if (!str) - return _gf_false; - - for (i = 0; str_array[i]; i++) { - if (strcmp (str, str_array[i]) == 0) - return _gf_true; - } - return _gf_false; -} - static gf_boolean_t posix_xattr_ignorable (char *key) { - return _is_in_array (posix_ignore_xattrs, key); + return gf_get_index_by_elem (posix_ignore_xattrs, key) >= 0; } static int @@ -763,7 +747,7 @@ _handle_list_xattr (dict_t *xattr_req, const char *real_path, int fdnum, while (remaining_size > 0) { key = list + list_offset; - if (_is_in_array (list_xattr_ignore_xattrs, key)) + if (gf_get_index_by_elem (list_xattr_ignore_xattrs, key) >= 0) goto next; if (posix_special_xattr (marker_xattrs, key)) @@ -2359,3 +2343,11 @@ posix_inode_ctx_get_all (inode_t *inode, xlator_t *this, return ret; } + +gf_boolean_t +posix_is_bulk_removexattr (char *name, dict_t *xdata) +{ + if (name && (strlen (name) == 0) && xdata) + return _gf_true; + return _gf_false; +} diff --git a/xlators/storage/posix/src/posix.c b/xlators/storage/posix/src/posix.c index 5f65172cdb8..e3fa184c1f2 100644 --- a/xlators/storage/posix/src/posix.c +++ b/xlators/storage/posix/src/posix.c @@ -103,6 +103,12 @@ extern char *marker_xattrs[]; (lutimes (path, tv)) #endif +static char *disallow_removexattrs[] = { + GF_XATTR_VOL_ID_KEY, + GFID_XATTR_KEY, + NULL +}; + dict_t* posix_dict_set_nlink (dict_t *req, dict_t *res, int32_t nlink) @@ -5350,7 +5356,11 @@ _posix_remove_xattr (dict_t *dict, char *key, data_t *value, void *data) * treated as success. */ - op_ret = sys_lremovexattr (filler->real_path, key); + if (filler->real_path) + op_ret = sys_lremovexattr (filler->real_path, key); + else + op_ret = sys_fremovexattr (filler->fdnum, key); + if (op_ret == -1) { if (errno == ENODATA || errno == ENOATTR) op_ret = 0; @@ -5358,10 +5368,13 @@ _posix_remove_xattr (dict_t *dict, char *key, data_t *value, void *data) if (op_ret == -1) { filler->op_errno = errno; - if (errno != ENOATTR && errno != ENODATA && errno != EPERM) + if (errno != ENOATTR && errno != ENODATA && errno != EPERM) { gf_msg (this->name, GF_LOG_ERROR, errno, - P_MSG_XATTR_FAILED, "removexattr failed on %s" - " (for %s)", filler->real_path, key); + P_MSG_XATTR_FAILED, "removexattr failed on " + "file/dir %s with gfid: %s (for %s)", + filler->real_path?filler->real_path:"", + uuid_utoa (filler->inode->gfid), key); + } } #ifdef GF_DARWIN_HOST_OS GF_FREE(newkey); @@ -5369,92 +5382,132 @@ _posix_remove_xattr (dict_t *dict, char *key, data_t *value, void *data) return op_ret; } - -int32_t -posix_removexattr (call_frame_t *frame, xlator_t *this, - loc_t *loc, const char *name, dict_t *xdata) +int +posix_common_removexattr (call_frame_t *frame, loc_t *loc, fd_t *fd, + const char *name, dict_t *xdata, int *op_errno, + dict_t **xdata_rsp) { - int32_t op_ret = -1; - int32_t op_errno = 0; - int32_t ret = -1; - char * real_path = NULL; - struct iatt stbuf = {0}; - dict_t *xattr = NULL; - posix_xattr_filler_t filler = {0,}; + gf_boolean_t bulk_removexattr = _gf_false; + gf_boolean_t disallow = _gf_false; + char *real_path = NULL; + struct posix_fd *pfd = NULL; + int op_ret = 0; + struct iatt stbuf = {0}; + int ret = 0; + int _fd = -1; + xlator_t *this = frame->this; + inode_t *inode = NULL; + posix_xattr_filler_t filler = {0}; DECLARE_OLD_FS_ID_VAR; - MAKE_INODE_HANDLE (real_path, this, loc, NULL); - if (!real_path) { - op_ret = -1; - op_errno = ESTALE; - goto out; - } - + SET_FS_ID (frame->root->uid, frame->root->gid); - if (!strcmp (GFID_XATTR_KEY, name)) { - gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED, - "Remove xattr called on gfid for file %s", real_path); - op_ret = -1; - goto out; + if (loc) { + MAKE_INODE_HANDLE (real_path, this, loc, NULL); + if (!real_path) { + op_ret = -1; + *op_errno = ESTALE; + goto out; + } + inode = loc->inode; + } else { + op_ret = posix_fd_ctx_get (fd, this, &pfd, op_errno); + if (op_ret < 0) { + gf_msg (this->name, GF_LOG_WARNING, *op_errno, + P_MSG_PFD_NULL, "pfd is NULL from fd=%p", fd); + goto out; + } + _fd = pfd->fd; + inode = fd->inode; } - if (!strcmp (GF_XATTR_VOL_ID_KEY, name)) { + + if (gf_get_index_by_elem (disallow_removexattrs, (char *)name) >= 0) { gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED, - "Remove xattr called on volume-id for file %s", - real_path); + "Remove xattr called on %s for file/dir %s with gfid: " + "%s", name, real_path?real_path:"", + uuid_utoa(inode->gfid)); op_ret = -1; + *op_errno = EPERM; goto out; + } else if (posix_is_bulk_removexattr ((char *)name, xdata)) { + bulk_removexattr = _gf_true; + (void) dict_has_key_from_array (xdata, disallow_removexattrs, + &disallow); + if (disallow) { + gf_msg (this->name, GF_LOG_WARNING, 0, + P_MSG_XATTR_NOT_REMOVED, + "Bulk removexattr has keys that shouldn't be " + "removed for file/dir %s with gfid: %s", + real_path?real_path:"", uuid_utoa(inode->gfid)); + op_ret = -1; + *op_errno = EPERM; + goto out; + } } - - SET_FS_ID (frame->root->uid, frame->root->gid); - - /** - * sending an empty key name with xdata containing the - * list of key(s) to be removed implies "bulk remove request" - * for removexattr. - */ - if (name && (strcmp (name, "") == 0) && xdata) { + if (bulk_removexattr) { filler.real_path = real_path; filler.this = this; + filler.fdnum = _fd; + filler.inode = inode; op_ret = dict_foreach (xdata, _posix_remove_xattr, &filler); if (op_ret) { - op_errno = filler.op_errno; + *op_errno = filler.op_errno; + goto out; + } + } else { + if (loc) + op_ret = sys_lremovexattr (real_path, name); + else + op_ret = sys_fremovexattr (_fd, name); + if (op_ret == -1) { + *op_errno = errno; + if (*op_errno != ENOATTR && *op_errno != ENODATA && + *op_errno != EPERM) { + gf_msg (this->name, GF_LOG_ERROR, *op_errno, + P_MSG_XATTR_FAILED, + "removexattr on %s with gfid %s " + "(for %s)", real_path, + uuid_utoa (inode->gfid), name); + } + goto out; } - - goto out; - } - - op_ret = sys_lremovexattr (real_path, name); - if (op_ret == -1) { - op_errno = errno; - if (op_errno != ENOATTR && op_errno != ENODATA && - op_errno != EPERM) - gf_msg (this->name, GF_LOG_ERROR, errno, - P_MSG_XATTR_FAILED, "removexattr on %s " - "(for %s)", real_path, name); - goto out; } if (xdata && dict_get (xdata, DHT_IATT_IN_XDATA_KEY)) { - ret = posix_pstat(this, loc->gfid, real_path, &stbuf); + if (loc) + ret = posix_pstat(this, loc->gfid, real_path, &stbuf); + else + ret = posix_fdstat (this, _fd, &stbuf); if (ret) goto out; - xattr = dict_new(); - if (!xattr) + *xdata_rsp = dict_new(); + if (!*xdata_rsp) goto out; - ret = posix_set_iatt_in_dict (xattr, &stbuf); + ret = posix_set_iatt_in_dict (*xdata_rsp, &stbuf); } op_ret = 0; - out: SET_TO_OLD_FS_ID (); + return op_ret; +} - STACK_UNWIND_STRICT (removexattr, frame, op_ret, op_errno, xattr); +int32_t +posix_removexattr (call_frame_t *frame, xlator_t *this, + loc_t *loc, const char *name, dict_t *xdata) +{ + int op_ret = 0; + int op_errno = 0; + dict_t *xdata_rsp = NULL; - if (xattr) - dict_unref (xattr); + op_ret = posix_common_removexattr (frame, loc, NULL, name, xdata, + &op_errno, &xdata_rsp); + STACK_UNWIND_STRICT (removexattr, frame, op_ret, op_errno, xdata_rsp); + + if (xdata_rsp) + dict_unref (xdata_rsp); return 0; } @@ -5463,69 +5516,16 @@ int32_t posix_fremovexattr (call_frame_t *frame, xlator_t *this, fd_t *fd, const char *name, dict_t *xdata) { - int32_t op_ret = -1; - int32_t op_errno = 0; - struct posix_fd * pfd = NULL; - struct iatt stbuf = {0,}; - dict_t *xattr = NULL; - int _fd = -1; - int ret = -1; - - DECLARE_OLD_FS_ID_VAR; + int32_t op_ret = 0; + int32_t op_errno = 0; + dict_t *xdata_rsp = NULL; - if (!strcmp (GFID_XATTR_KEY, name)) { - gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED, - "Remove xattr called on gfid for file"); - goto out; - } - if (!strcmp (GF_XATTR_VOL_ID_KEY, name)) { - gf_msg (this->name, GF_LOG_WARNING, 0, P_MSG_XATTR_NOT_REMOVED, - "Remove xattr called on volume-id for file"); - goto out; - } + op_ret = posix_common_removexattr (frame, NULL, fd, name, xdata, + &op_errno, &xdata_rsp); + STACK_UNWIND_STRICT (fremovexattr, frame, op_ret, op_errno, xdata_rsp); - ret = posix_fd_ctx_get (fd, this, &pfd, &op_errno); - if (ret < 0) { - gf_msg (this->name, GF_LOG_WARNING, op_errno, P_MSG_PFD_NULL, - "pfd is NULL from fd=%p", fd); - goto out; - } - _fd = pfd->fd; - - - - SET_FS_ID (frame->root->uid, frame->root->gid); - - op_ret = sys_fremovexattr (_fd, name); - if (op_ret == -1) { - op_errno = errno; - if (op_errno != ENOATTR && op_errno != ENODATA && - op_errno != EPERM) - gf_msg (this->name, GF_LOG_ERROR, errno, - P_MSG_XATTR_FAILED, "fremovexattr (for %s)", - name); - goto out; - } - - if (xdata && dict_get (xdata, DHT_IATT_IN_XDATA_KEY)) { - ret = posix_fdstat (this, pfd->fd, &stbuf); - if (ret) - goto out; - xattr = dict_new(); - if (!xattr) - goto out; - - ret = posix_set_iatt_in_dict (xattr, &stbuf); - } - op_ret = 0; - -out: - SET_TO_OLD_FS_ID (); - - STACK_UNWIND_STRICT (fremovexattr, frame, op_ret, op_errno, xattr); - - if (xattr) - dict_unref (xattr); + if (xdata_rsp) + dict_unref (xdata_rsp); return 0; } diff --git a/xlators/storage/posix/src/posix.h b/xlators/storage/posix/src/posix.h index 332c0ab4f49..480566a5340 100644 --- a/xlators/storage/posix/src/posix.h +++ b/xlators/storage/posix/src/posix.h @@ -293,4 +293,6 @@ posix_get_objectsignature (char *, dict_t *); int32_t posix_fdget_objectsignature (int, dict_t *); +gf_boolean_t +posix_is_bulk_removexattr (char *name, dict_t *dict); #endif /* _POSIX_H */ -- cgit