From fe704e0f997444d74966aa7c5bfca484ce54f6a4 Mon Sep 17 00:00:00 2001 From: Amar Tumballi Date: Thu, 27 Sep 2018 09:55:19 +0530 Subject: [PATCH 389/399] server: don't allow '/' in basename Server stack needs to have all the sort of validation, assuming clients can be compromized. It is possible for a compromized client to send basenames with paths with '/', and with that create files without permission on server. By sanitizing the basename, and not allowing anything other than actual directory as the parent for any entry creation, we can mitigate the effects of clients not able to exploit the server. Fixes: CVE-2018-14651 BUG: 1633013 Change-Id: I98d042a9f8e300161fbc3ee5b6e8de755c9765f9 Signed-off-by: Amar Tumballi Reviewed-on: https://code.engineering.redhat.com/gerrit/151169 Reviewed-by: Xavi Hernandez Reviewed-by: Shyam Ranganathan Reviewed-by: Atin Mukherjee --- xlators/protocol/server/src/server-resolve.c | 31 ++++++++++++++++++++-------- xlators/storage/posix/src/posix-handle.h | 5 +++-- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/xlators/protocol/server/src/server-resolve.c b/xlators/protocol/server/src/server-resolve.c index b3eda0e..25db43f 100644 --- a/xlators/protocol/server/src/server-resolve.c +++ b/xlators/protocol/server/src/server-resolve.c @@ -307,22 +307,35 @@ resolve_entry_simple (call_frame_t *frame) ret = 1; goto out; } - - /* expected @parent was found from the inode cache */ - gf_uuid_copy (state->loc_now->pargfid, resolve->pargfid); - state->loc_now->parent = inode_ref (parent); - - if (strstr (resolve->bname, "../")) { - /* Resolving outside the parent's tree is not allowed */ + if (parent->ia_type != IA_IFDIR) { + /* Parent type should be 'directory', and nothing else */ gf_msg (this->name, GF_LOG_ERROR, EPERM, PS_MSG_GFID_RESOLVE_FAILED, - "%s: path sent by client not allowed", - resolve->bname); + "%s: parent type not directory (%d)", + uuid_utoa (parent->gfid), parent->ia_type); resolve->op_ret = -1; resolve->op_errno = EPERM; ret = 1; goto out; } + + /* expected @parent was found from the inode cache */ + gf_uuid_copy (state->loc_now->pargfid, resolve->pargfid); + state->loc_now->parent = inode_ref (parent); + + if (strchr (resolve->bname, '/')) { + /* basename should be a string (without '/') in a directory, + it can't span multiple levels. This can also lead to + resolving outside the parent's tree, which is not allowed */ + gf_msg (this->name, GF_LOG_ERROR, EPERM, + PS_MSG_GFID_RESOLVE_FAILED, + "%s: basename sent by client not allowed", + resolve->bname); + resolve->op_ret = -1; + resolve->op_errno = EPERM; + ret = 1; + goto out; + } state->loc_now->name = resolve->bname; inode = inode_grep (state->itable, parent, resolve->bname); diff --git a/xlators/storage/posix/src/posix-handle.h b/xlators/storage/posix/src/posix-handle.h index a0f82ec..45ca1d1 100644 --- a/xlators/storage/posix/src/posix-handle.h +++ b/xlators/storage/posix/src/posix-handle.h @@ -223,9 +223,10 @@ break; \ } \ \ - if (strstr (loc->name, "../")) { \ + if (strchr (loc->name, '/')) { \ gf_msg (this->name, GF_LOG_ERROR, 0, P_MSG_ENTRY_HANDLE_CREATE, \ - "'../' in name not allowed: (%s)", loc->name); \ + "'/' in name not allowed: (%s)", loc->name); \ + op_ret = -1; \ break; \ } \ if (LOC_HAS_ABSPATH (loc)) { \ -- 1.8.3.1