From 6f4a6140e931222504bdd0bb449e5b7def21276e Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Tue, 6 Jun 2023 15:17:26 +0200 Subject: [PATCH 1/2] CVE-2023-34968: mdssvc: cache and reuse stat info in struct sl_inode_path_map BUG: https://bugzilla.samba.org/show_bug.cgi?id=15388 Signed-off-by: Ralph Boehme --- source3/rpc_server/mdssvc/mdssvc.c | 32 +++++++----------------------- source3/rpc_server/mdssvc/mdssvc.h | 1 + 2 files changed, 8 insertions(+), 25 deletions(-) diff --git a/source3/rpc_server/mdssvc/mdssvc.c b/source3/rpc_server/mdssvc/mdssvc.c index 7dd3c84713f1..a6d09a43b9c8 100644 --- a/source3/rpc_server/mdssvc/mdssvc.c +++ b/source3/rpc_server/mdssvc/mdssvc.c @@ -446,7 +446,10 @@ static int ino_path_map_destr_cb(struct sl_inode_path_map *entry) * entries by calling talloc_free() on the query slq handles. **/ -static bool inode_map_add(struct sl_query *slq, uint64_t ino, const char *path) +static bool inode_map_add(struct sl_query *slq, + uint64_t ino, + const char *path, + struct stat_ex *st) { NTSTATUS status; struct sl_inode_path_map *entry; @@ -493,6 +496,7 @@ static bool inode_map_add(struct sl_query *slq, uint64_t ino, const char *path) entry->ino = ino; entry->mds_ctx = slq->mds_ctx; + entry->st = *st; entry->path = talloc_strdup(entry, path); if (entry->path == NULL) { DEBUG(1, ("talloc failed\n")); @@ -617,7 +621,7 @@ bool mds_add_result(struct sl_query *slq, const char *path) return false; } - ok = inode_map_add(slq, ino64, path); + ok = inode_map_add(slq, ino64, path, &sb); if (!ok) { DEBUG(1, ("inode_map_add error\n")); slq->state = SLQ_STATE_ERROR; @@ -1340,29 +1344,7 @@ static bool slrpc_fetch_attributes(struct mds_ctx *mds_ctx, elem = talloc_get_type_abort(p, struct sl_inode_path_map); path = elem->path; - status = synthetic_pathref(talloc_tos(), - mds_ctx->conn->cwd_fsp, - path, - NULL, - NULL, - 0, - 0, - &smb_fname); - if (!NT_STATUS_IS_OK(status)) { - /* This is not an error, the user may lack permissions */ - DBG_DEBUG("synthetic_pathref [%s]: %s\n", - smb_fname_str_dbg(smb_fname), - nt_errstr(status)); - return true; - } - - status = vfs_stat_fsp(smb_fname->fsp); - if (!NT_STATUS_IS_OK(status)) { - TALLOC_FREE(smb_fname); - return true; - } - - sp = &smb_fname->fsp->fsp_name->st; + sp = &elem->st; } ok = add_filemeta(mds_ctx, reqinfo, fm_array, path, sp); diff --git a/source3/rpc_server/mdssvc/mdssvc.h b/source3/rpc_server/mdssvc/mdssvc.h index 205417c4be1c..ff36b329f2b0 100644 --- a/source3/rpc_server/mdssvc/mdssvc.h +++ b/source3/rpc_server/mdssvc/mdssvc.h @@ -105,6 +105,7 @@ struct sl_inode_path_map { struct mds_ctx *mds_ctx; uint64_t ino; char *path; + struct stat_ex st; }; /* Per process state */ -- 2.40.0 From d679f89252707adc2d61ce782ac01cda5685a363 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Mon, 5 Jun 2023 18:02:20 +0200 Subject: [PATCH 2/2] CVE-2023-34968: mdssvc: return a fake share path Instead of returning the real server-side absolute path of shares and search results, return a fake absolute path replacing the path of the share with the share name, iow for a share "test" with a server-side path of "/foo/bar", we previosly returns /foo/bar and /foo/bar/search/result now return /test and /test/search/result This also changes the mdssvc RPC client to return relative paths instead of the previous absolute paths received from the server. To cope with the changes the relevant mdssvc tests have been adjusted to work with the fake paths. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15388 Signed-off-by: Ralph Boehme --- python/samba/tests/blackbox/mdsearch.py | 8 +-- python/samba/tests/dcerpc/mdssvc.py | 26 +++++----- source3/rpc_client/cli_mdssvc.c | 14 +++++- source3/rpc_server/mdssvc/mdssvc.c | 60 +++++++++++++++++++++-- source3/rpc_server/mdssvc/srv_mdssvc_nt.c | 18 +++++-- 5 files changed, 99 insertions(+), 27 deletions(-) diff --git a/python/samba/tests/blackbox/mdsearch.py b/python/samba/tests/blackbox/mdsearch.py index c9156ae6e0e4..c8e75661f151 100644 --- a/python/samba/tests/blackbox/mdsearch.py +++ b/python/samba/tests/blackbox/mdsearch.py @@ -76,10 +76,7 @@ testfiles = [ self.t.start() time.sleep(1) - pipe = mdssvc.mdssvc('ncacn_np:fileserver[/pipe/mdssvc]', self.get_loadparm()) - conn = mdscli.conn(pipe, 'spotlight', '/foo') - self.sharepath = conn.sharepath() - conn.disconnect(pipe) + self.sharepath = os.environ["LOCAL_PATH"] for file in testfiles: f = open("%s/%s" % (self.sharepath, file), "w") @@ -126,5 +123,4 @@ testfiles = [ output = self.check_output("mdsearch --configfile=%s -U %s%%%s fileserver spotlight '*==\"samba*\"'" % (config, username, password)) actual = output.decode('utf-8').splitlines() - expected = ["%s/%s" % (self.sharepath, file) for file in testfiles] - self.assertEqual(expected, actual) + self.assertEqual(testfiles, actual) diff --git a/python/samba/tests/dcerpc/mdssvc.py b/python/samba/tests/dcerpc/mdssvc.py index b0df509ddc79..5002e5d26d64 100644 --- a/python/samba/tests/dcerpc/mdssvc.py +++ b/python/samba/tests/dcerpc/mdssvc.py @@ -84,10 +84,11 @@ testfiles = [ self.t = threading.Thread(target=MdssvcTests.http_server, args=(self,)) self.t.setDaemon(True) self.t.start() + self.sharepath = os.environ["LOCAL_PATH"] time.sleep(1) conn = mdscli.conn(self.pipe, 'spotlight', '/foo') - self.sharepath = conn.sharepath() + self.fakepath = conn.sharepath() conn.disconnect(self.pipe) for file in testfiles: @@ -105,12 +106,11 @@ testfiles = [ self.server.serve_forever() def run_test(self, query, expect, json_in, json_out): - expect = [s.replace("%BASEPATH%", self.sharepath) for s in expect] self.server.json_in = json_in.replace("%BASEPATH%", self.sharepath) self.server.json_out = json_out.replace("%BASEPATH%", self.sharepath) self.conn = mdscli.conn(self.pipe, 'spotlight', '/foo') - search = self.conn.search(self.pipe, query, self.sharepath) + search = self.conn.search(self.pipe, query, self.fakepath) # Give it some time, the get_results() below returns immediately # what's available, so if we ask to soon, we might get back no results @@ -141,7 +141,7 @@ testfiles = [ ] } }''' - exp_results = ["%BASEPATH%/foo", "%BASEPATH%/bar"] + exp_results = ["foo", "bar"] self.run_test('*=="samba*"', exp_results, exp_json_query, fake_json_response) def test_mdscli_search_escapes(self): @@ -181,14 +181,14 @@ testfiles = [ } }''' exp_results = [ - r"%BASEPATH%/x+x", - r"%BASEPATH%/x*x", - r"%BASEPATH%/x=x", - r"%BASEPATH%/x'x", - r"%BASEPATH%/x?x", - r"%BASEPATH%/x x", - r"%BASEPATH%/x(x", - "%BASEPATH%/x\"x", - r"%BASEPATH%/x\x", + r"x+x", + r"x*x", + r"x=x", + r"x'x", + r"x?x", + r"x x", + r"x(x", + "x\"x", + r"x\x", ] self.run_test(sl_query, exp_results, exp_json_query, fake_json_response) diff --git a/source3/rpc_client/cli_mdssvc.c b/source3/rpc_client/cli_mdssvc.c index 046d37135cbc..01de378ce4d1 100644 --- a/source3/rpc_client/cli_mdssvc.c +++ b/source3/rpc_client/cli_mdssvc.c @@ -725,6 +725,7 @@ static void mdscli_get_path_done(struct tevent_req *subreq) req, struct mdscli_get_path_state); DALLOC_CTX *d = NULL; char *path = NULL; + const char *p = NULL; NTSTATUS status; bool ok; @@ -759,7 +760,18 @@ static void mdscli_get_path_done(struct tevent_req *subreq) tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR); return; } - state->path = talloc_move(state, &path); + + /* Path is prefixed by /SHARENAME/, strip it */ + p = strchr(path + 1, '/'); + if (p == NULL) { + tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR); + return; + } + state->path = talloc_strdup(state, p + 1); + if (state->path == NULL) { + tevent_req_nterror(req, NT_STATUS_NO_MEMORY); + return; + } DBG_DEBUG("path: %s\n", state->path); tevent_req_done(req); diff --git a/source3/rpc_server/mdssvc/mdssvc.c b/source3/rpc_server/mdssvc/mdssvc.c index a6d09a43b9c8..aa4747d98acd 100644 --- a/source3/rpc_server/mdssvc/mdssvc.c +++ b/source3/rpc_server/mdssvc/mdssvc.c @@ -520,10 +520,13 @@ static bool inode_map_add(struct sl_query *slq, bool mds_add_result(struct sl_query *slq, const char *path) { struct smb_filename *smb_fname = NULL; + const char *relative = NULL; + char *fake_path = NULL; struct stat_ex sb; uint64_t ino64; int result; NTSTATUS status; + bool sub; bool ok; /* @@ -598,6 +601,17 @@ bool mds_add_result(struct sl_query *slq, const char *path) } } + sub = subdir_of(slq->mds_ctx->spath, + strlen(slq->mds_ctx->spath), + path, + &relative); + if (!sub) { + DBG_ERR("[%s] is not inside [%s]\n", + path, slq->mds_ctx->spath); + slq->state = SLQ_STATE_ERROR; + return false; + } + /* * Add inode number and filemeta to result set, this is what * we return as part of the result set of a query @@ -610,18 +624,30 @@ bool mds_add_result(struct sl_query *slq, const char *path) slq->state = SLQ_STATE_ERROR; return false; } + + fake_path = talloc_asprintf(slq, + "/%s/%s", + slq->mds_ctx->sharename, + relative); + if (fake_path == NULL) { + slq->state = SLQ_STATE_ERROR; + return false; + } + ok = add_filemeta(slq->mds_ctx, slq->reqinfo, slq->query_results->fm_array, - path, + fake_path, &sb); if (!ok) { DBG_ERR("add_filemeta error\n"); + TALLOC_FREE(fake_path); slq->state = SLQ_STATE_ERROR; return false; } - ok = inode_map_add(slq, ino64, path, &sb); + ok = inode_map_add(slq, ino64, fake_path, &sb); + TALLOC_FREE(fake_path); if (!ok) { DEBUG(1, ("inode_map_add error\n")); slq->state = SLQ_STATE_ERROR; @@ -824,6 +850,32 @@ static void slq_close_timer(struct tevent_context *ev, } } +/** + * Translate a fake scope from the client like /sharename/dir + * to the real server-side path, replacing the "/sharename" part + * with the absolute server-side path of the share. + **/ +static bool mdssvc_real_scope(struct sl_query *slq, const char *fake_scope) +{ + size_t sname_len = strlen(slq->mds_ctx->sharename); + size_t fake_scope_len = strlen(fake_scope); + + if (fake_scope_len < sname_len + 1) { + DBG_ERR("Short scope [%s] for share [%s]\n", + fake_scope, slq->mds_ctx->sharename); + return false; + } + + slq->path_scope = talloc_asprintf(slq, + "%s%s", + slq->mds_ctx->spath, + fake_scope + sname_len + 1); + if (slq->path_scope == NULL) { + return false; + } + return true; +} + /** * Begin a search query **/ @@ -936,8 +988,8 @@ static bool slrpc_open_query(struct mds_ctx *mds_ctx, goto error; } - slq->path_scope = talloc_strdup(slq, scope); - if (slq->path_scope == NULL) { + ok = mdssvc_real_scope(slq, scope); + if (!ok) { goto error; } diff --git a/source3/rpc_server/mdssvc/srv_mdssvc_nt.c b/source3/rpc_server/mdssvc/srv_mdssvc_nt.c index 2fca15cb8a8d..b4970d75033f 100644 --- a/source3/rpc_server/mdssvc/srv_mdssvc_nt.c +++ b/source3/rpc_server/mdssvc/srv_mdssvc_nt.c @@ -81,6 +81,7 @@ void _mdssvc_open(struct pipes_struct *p, struct mdssvc_open *r) loadparm_s3_global_substitution(); int snum; char *outpath = discard_const_p(char, r->out.share_path); + char *fake_path = NULL; char *path; NTSTATUS status; @@ -98,12 +99,21 @@ void _mdssvc_open(struct pipes_struct *p, struct mdssvc_open *r) path = lp_path(talloc_tos(), lp_sub, snum); if (path == NULL) { - DBG_ERR("Couldn't create policy handle for %s\n", + DBG_ERR("Couldn't create path for %s\n", r->in.share_name); p->fault_state = DCERPC_FAULT_CANT_PERFORM; return; } + fake_path = talloc_asprintf(p->mem_ctx, "/%s", r->in.share_name); + if (fake_path == NULL) { + DBG_ERR("Couldn't create fake share path for %s\n", + r->in.share_name); + talloc_free(path); + p->fault_state = DCERPC_FAULT_CANT_PERFORM; + return; + } + status = create_mdssvc_policy_handle(p->mem_ctx, p, snum, r->in.share_name, @@ -112,18 +122,20 @@ void _mdssvc_open(struct pipes_struct *p, struct mdssvc_open *r) if (NT_STATUS_EQUAL(status, NT_STATUS_WRONG_VOLUME)) { ZERO_STRUCTP(r->out.handle); talloc_free(path); + talloc_free(fake_path); return; } if (!NT_STATUS_IS_OK(status)) { DBG_ERR("Couldn't create policy handle for %s\n", r->in.share_name); talloc_free(path); + talloc_free(fake_path); p->fault_state = DCERPC_FAULT_CANT_PERFORM; return; } - strlcpy(outpath, path, 1024); - talloc_free(path); + strlcpy(outpath, fake_path, 1024); + talloc_free(fake_path); return; } -- 2.40.0