From fe704e0f997444d74966aa7c5bfca484ce54f6a4 Mon Sep 17 00:00:00 2001
From: Amar Tumballi <amarts@redhat.com>
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 <amarts@redhat.com>
Reviewed-on: https://code.engineering.redhat.com/gerrit/151169
Reviewed-by: Xavi Hernandez <xhernandez@redhat.com>
Reviewed-by: Shyam Ranganathan <srangana@redhat.com>
Reviewed-by: Atin Mukherjee <amukherj@redhat.com>
---
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