diff options
| author | Amar Tumballi <amarts@redhat.com> | 2018-11-01 07:02:11 +0530 |
|---|---|---|
| committer | Amar Tumballi <amarts@redhat.com> | 2018-11-01 15:04:45 +0530 |
| commit | 5fdb7ae37f602894f81a2cadc5a4c609a4c85427 (patch) | |
| tree | d9f7db1c6f6b66772475f3f796fdf02b2b766a6d | |
| parent | 7fac81aeab5805fb2bd719d7489636633bb5e32a (diff) | |
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
Fixes: bz#1644755
Change-Id: I5dc0da0da2713452ff2b65ac2ddbccf1a267dc20
Signed-off-by: Amar Tumballi <amarts@redhat.com>
| -rw-r--r-- | xlators/protocol/server/src/server-resolve.c | 21 | ||||
| -rw-r--r-- | xlators/storage/posix/src/posix-handle.h | 4 |
2 files changed, 19 insertions, 6 deletions
diff --git a/xlators/protocol/server/src/server-resolve.c b/xlators/protocol/server/src/server-resolve.c index ba4d2f16a93..26260a5ee2c 100644 --- a/xlators/protocol/server/src/server-resolve.c +++ b/xlators/protocol/server/src/server-resolve.c @@ -294,19 +294,32 @@ resolve_entry_simple(call_frame_t *frame) goto out; } + 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: 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 (strstr(resolve->bname, "../")) { - /* Resolving outside the parent's tree is not allowed */ + 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: path sent by client not allowed", resolve->bname); + "%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 7c79b569128..c4d7cb14503 100644 --- a/xlators/storage/posix/src/posix-handle.h +++ b/xlators/storage/posix/src/posix-handle.h @@ -150,9 +150,9 @@ 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; \ } \ |