From 2fdcf0a563e3547fd2e0e2c5318fe77de5bd96ce Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 21 Mar 2019 14:51:30 -0700 Subject: [PATCH 1/2] CVE-2019-3880 s3: rpc: winreg: Remove implementations of SaveKey/RestoreKey. The were not using VFS backend calls and could only work locally, and were unsafe against symlink races and other security issues. If the incoming handle is valid, return WERR_BAD_PATHNAME. [MS-RRP] states "The format of the file name is implementation-specific" so ensure we don't allow this. As reported by Michael Hanselmann. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13851 Signed-off-by: Jeremy Allison Reviewed-by: Andrew Bartlett --- source3/rpc_server/winreg/srv_winreg_nt.c | 92 +---------------------- 1 file changed, 4 insertions(+), 88 deletions(-) diff --git a/source3/rpc_server/winreg/srv_winreg_nt.c b/source3/rpc_server/winreg/srv_winreg_nt.c index d9ee8d0602d..816c6bb2a12 100644 --- a/source3/rpc_server/winreg/srv_winreg_nt.c +++ b/source3/rpc_server/winreg/srv_winreg_nt.c @@ -639,46 +639,6 @@ WERROR _winreg_AbortSystemShutdown(struct pipes_struct *p, return (ret == 0) ? WERR_OK : WERR_ACCESS_DENIED; } -/******************************************************************* - ********************************************************************/ - -static int validate_reg_filename(TALLOC_CTX *ctx, char **pp_fname ) -{ - char *p = NULL; - int num_services = lp_numservices(); - int snum = -1; - const char *share_path = NULL; - char *fname = *pp_fname; - - /* convert to a unix path, stripping the C:\ along the way */ - - if (!(p = valid_share_pathname(ctx, fname))) { - return -1; - } - - /* has to exist within a valid file share */ - - for (snum=0; snumin.handle ); - char *fname = NULL; - int snum = -1; - if ( !regkey ) + if ( !regkey ) { return WERR_INVALID_HANDLE; - - if ( !r->in.filename || !r->in.filename->name ) - return WERR_INVALID_PARAMETER; - - fname = talloc_strdup(p->mem_ctx, r->in.filename->name); - if (!fname) { - return WERR_NOT_ENOUGH_MEMORY; } - - DEBUG(8,("_winreg_RestoreKey: verifying restore of key [%s] from " - "\"%s\"\n", regkey->key->name, fname)); - - if ((snum = validate_reg_filename(p->mem_ctx, &fname)) == -1) - return WERR_BAD_PATHNAME; - - /* user must posses SeRestorePrivilege for this this proceed */ - - if ( !security_token_has_privilege(p->session_info->security_token, SEC_PRIV_RESTORE)) { - return WERR_ACCESS_DENIED; - } - - DEBUG(2,("_winreg_RestoreKey: Restoring [%s] from %s in share %s\n", - regkey->key->name, fname, lp_servicename(talloc_tos(), snum) )); - - return reg_restorekey(regkey, fname); + return WERR_BAD_PATHNAME; } /******************************************************************* @@ -727,30 +662,11 @@ WERROR _winreg_SaveKey(struct pipes_struct *p, struct winreg_SaveKey *r) { struct registry_key *regkey = find_regkey_by_hnd( p, r->in.handle ); - char *fname = NULL; - int snum = -1; - if ( !regkey ) + if ( !regkey ) { return WERR_INVALID_HANDLE; - - if ( !r->in.filename || !r->in.filename->name ) - return WERR_INVALID_PARAMETER; - - fname = talloc_strdup(p->mem_ctx, r->in.filename->name); - if (!fname) { - return WERR_NOT_ENOUGH_MEMORY; } - - DEBUG(8,("_winreg_SaveKey: verifying backup of key [%s] to \"%s\"\n", - regkey->key->name, fname)); - - if ((snum = validate_reg_filename(p->mem_ctx, &fname)) == -1 ) - return WERR_BAD_PATHNAME; - - DEBUG(2,("_winreg_SaveKey: Saving [%s] to %s in share %s\n", - regkey->key->name, fname, lp_servicename(talloc_tos(), snum) )); - - return reg_savekey(regkey, fname); + return WERR_BAD_PATHNAME; } /******************************************************************* -- 2.21.0.392.gf8f6787159e-goog From ea18f57d9f47b00dc63bba92394f408394877708 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Wed, 27 Mar 2019 12:51:27 -0700 Subject: [PATCH 2/2] CVE-2019-3880 s3: rpc: winreg: Remove implementations of SaveKey/RestoreKey. Remove the now unused code implementations of registry file io. As reported by Michael Hanselmann. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13851 Signed-off-by: Jeremy Allison --- source3/registry/reg_api_regf.c | 302 ---------------------- source3/registry/reg_api_regf.h | 35 --- source3/rpc_server/winreg/srv_winreg_nt.c | 1 - source3/rpc_server/wscript_build | 2 +- source3/wscript_build | 4 - 5 files changed, 1 insertion(+), 343 deletions(-) delete mode 100644 source3/registry/reg_api_regf.c delete mode 100644 source3/registry/reg_api_regf.h diff --git a/source3/registry/reg_api_regf.c b/source3/registry/reg_api_regf.c deleted file mode 100644 index b83404c55e3..00000000000 --- a/source3/registry/reg_api_regf.c +++ /dev/null @@ -1,302 +0,0 @@ -/* - * Unix SMB/CIFS implementation. - * Virtual Windows Registry Layer - * Copyright (C) Volker Lendecke 2006 - * Copyright (C) Michael Adam 2007-2008 - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, see . - */ - -#include "includes.h" -#include "system/filesys.h" -#include "registry.h" -#include "reg_api_regf.h" -#include "reg_cachehook.h" -#include "regfio.h" -#include "reg_util_internal.h" -#include "reg_dispatcher.h" - -#undef DBGC_CLASS -#define DBGC_CLASS DBGC_REGISTRY - -/******************************************************************* - Note: topkeypat is the *full* path that this *key will be - loaded into (including the name of the key) - ********************************************************************/ - -static WERROR reg_load_tree(REGF_FILE *regfile, const char *topkeypath, - REGF_NK_REC *key) -{ - REGF_NK_REC *subkey; - struct registry_key_handle registry_key; - struct regval_ctr *values; - struct regsubkey_ctr *subkeys; - int i; - char *path = NULL; - WERROR result = WERR_OK; - - /* initialize the struct registry_key_handle structure */ - - registry_key.ops = reghook_cache_find(topkeypath); - if (!registry_key.ops) { - DEBUG(0, ("reg_load_tree: Failed to assign registry_ops " - "to [%s]\n", topkeypath)); - return WERR_FILE_NOT_FOUND; - } - - registry_key.name = talloc_strdup(regfile->mem_ctx, topkeypath); - if (!registry_key.name) { - DEBUG(0, ("reg_load_tree: Talloc failed for reg_key.name!\n")); - return WERR_NOT_ENOUGH_MEMORY; - } - - /* now start parsing the values and subkeys */ - - result = regsubkey_ctr_init(regfile->mem_ctx, &subkeys); - W_ERROR_NOT_OK_RETURN(result); - - result = regval_ctr_init(subkeys, &values); - W_ERROR_NOT_OK_RETURN(result); - - /* copy values into the struct regval_ctr */ - - for (i=0; inum_values; i++) { - regval_ctr_addvalue(values, key->values[i].valuename, - key->values[i].type, - key->values[i].data, - (key->values[i].data_size & ~VK_DATA_IN_OFFSET)); - } - - /* copy subkeys into the struct regsubkey_ctr */ - - key->subkey_index = 0; - while ((subkey = regfio_fetch_subkey( regfile, key ))) { - result = regsubkey_ctr_addkey(subkeys, subkey->keyname); - if (!W_ERROR_IS_OK(result)) { - TALLOC_FREE(subkeys); - return result; - } - } - - /* write this key and values out */ - - if (!store_reg_values(®istry_key, values) - || !store_reg_keys(®istry_key, subkeys)) - { - DEBUG(0,("reg_load_tree: Failed to load %s!\n", topkeypath)); - result = WERR_REGISTRY_IO_FAILED; - } - - TALLOC_FREE(subkeys); - - if (!W_ERROR_IS_OK(result)) { - return result; - } - - /* now continue to load each subkey registry tree */ - - key->subkey_index = 0; - while ((subkey = regfio_fetch_subkey(regfile, key))) { - path = talloc_asprintf(regfile->mem_ctx, - "%s\\%s", - topkeypath, - subkey->keyname); - if (path == NULL) { - return WERR_NOT_ENOUGH_MEMORY; - } - result = reg_load_tree(regfile, path, subkey); - if (!W_ERROR_IS_OK(result)) { - break; - } - } - - return result; -} - -/******************************************************************* - ********************************************************************/ - -static WERROR restore_registry_key(struct registry_key_handle *krecord, - const char *fname) -{ - REGF_FILE *regfile; - REGF_NK_REC *rootkey; - WERROR result; - - /* open the registry file....fail if the file already exists */ - - regfile = regfio_open(fname, (O_RDONLY), 0); - if (regfile == NULL) { - DEBUG(0, ("restore_registry_key: failed to open \"%s\" (%s)\n", - fname, strerror(errno))); - return ntstatus_to_werror(map_nt_error_from_unix(errno)); - } - - /* get the rootkey from the regf file and then load the tree - via recursive calls */ - - if (!(rootkey = regfio_rootkey(regfile))) { - regfio_close(regfile); - return WERR_NOT_REGISTRY_FILE; - } - - result = reg_load_tree(regfile, krecord->name, rootkey); - - /* cleanup */ - - regfio_close(regfile); - - return result; -} - -WERROR reg_restorekey(struct registry_key *key, const char *fname) -{ - return restore_registry_key(key->key, fname); -} - -/******************************************************************** -********************************************************************/ - -static WERROR reg_write_tree(REGF_FILE *regfile, const char *keypath, - REGF_NK_REC *parent) -{ - REGF_NK_REC *key; - struct regval_ctr *values; - struct regsubkey_ctr *subkeys; - int i, num_subkeys; - char *key_tmp = NULL; - char *keyname, *parentpath; - char *subkeypath = NULL; - char *subkeyname; - struct registry_key_handle registry_key; - WERROR result = WERR_OK; - struct security_descriptor *sec_desc = NULL; - - if (!regfile) { - return WERR_GEN_FAILURE; - } - - if (!keypath) { - return WERR_BAD_PATHNAME; - } - - /* split up the registry key path */ - - key_tmp = talloc_strdup(regfile->mem_ctx, keypath); - if (!key_tmp) { - return WERR_NOT_ENOUGH_MEMORY; - } - if (!reg_split_key(key_tmp, &parentpath, &keyname)) { - return WERR_BAD_PATHNAME; - } - - if (!keyname) { - keyname = parentpath; - } - - /* we need a registry_key_handle object here to enumerate subkeys and values */ - - ZERO_STRUCT(registry_key); - - registry_key.name = talloc_strdup(regfile->mem_ctx, keypath); - if (registry_key.name == NULL) { - return WERR_NOT_ENOUGH_MEMORY; - } - - registry_key.ops = reghook_cache_find(registry_key.name); - if (registry_key.ops == NULL) { - return WERR_FILE_NOT_FOUND; - } - - /* lookup the values and subkeys */ - - result = regsubkey_ctr_init(regfile->mem_ctx, &subkeys); - W_ERROR_NOT_OK_RETURN(result); - - result = regval_ctr_init(subkeys, &values); - W_ERROR_NOT_OK_RETURN(result); - - fetch_reg_keys(®istry_key, subkeys); - fetch_reg_values(®istry_key, values); - - result = regkey_get_secdesc(regfile->mem_ctx, ®istry_key, &sec_desc); - if (!W_ERROR_IS_OK(result)) { - goto done; - } - - /* write out this key */ - - key = regfio_write_key(regfile, keyname, values, subkeys, sec_desc, - parent); - if (key == NULL) { - result = WERR_CAN_NOT_COMPLETE; - goto done; - } - - /* write each one of the subkeys out */ - - num_subkeys = regsubkey_ctr_numkeys(subkeys); - for (i=0; imem_ctx, "%s\\%s", - keypath, subkeyname); - if (subkeypath == NULL) { - result = WERR_NOT_ENOUGH_MEMORY; - goto done; - } - result = reg_write_tree(regfile, subkeypath, key); - if (!W_ERROR_IS_OK(result)) - goto done; - } - - DEBUG(6, ("reg_write_tree: wrote key [%s]\n", keypath)); - -done: - TALLOC_FREE(subkeys); - TALLOC_FREE(registry_key.name); - - return result; -} - -static WERROR backup_registry_key(struct registry_key_handle *krecord, - const char *fname) -{ - REGF_FILE *regfile; - WERROR result; - - /* open the registry file....fail if the file already exists */ - - regfile = regfio_open(fname, (O_RDWR|O_CREAT|O_EXCL), - (S_IRUSR|S_IWUSR)); - if (regfile == NULL) { - DEBUG(0,("backup_registry_key: failed to open \"%s\" (%s)\n", - fname, strerror(errno) )); - return ntstatus_to_werror(map_nt_error_from_unix(errno)); - } - - /* write the registry tree to the file */ - - result = reg_write_tree(regfile, krecord->name, NULL); - - /* cleanup */ - - regfio_close(regfile); - - return result; -} - -WERROR reg_savekey(struct registry_key *key, const char *fname) -{ - return backup_registry_key(key->key, fname); -} diff --git a/source3/registry/reg_api_regf.h b/source3/registry/reg_api_regf.h deleted file mode 100644 index c68261fa95f..00000000000 --- a/source3/registry/reg_api_regf.h +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Unix SMB/CIFS implementation. - * - * Virtual Windows Registry Layer - * - * Copyright (C) Volker Lendecke 2006 - * Copyright (C) Michael Adam 2007-2008 - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, see . - */ - -/* - * Portion of reg_api that references regfio.c code. - * These are the savekey and restorekey calls. - * These calls are currently only used in the WINREG rpc server. - */ - -#ifndef _REG_API_REGF_H -#define _REG_API_REGF_H - -WERROR reg_restorekey(struct registry_key *key, const char *fname); -WERROR reg_savekey(struct registry_key *key, const char *fname); - -#endif /* _REG_API_REGF_H */ diff --git a/source3/rpc_server/winreg/srv_winreg_nt.c b/source3/rpc_server/winreg/srv_winreg_nt.c index 816c6bb2a12..e42f0ba203e 100644 --- a/source3/rpc_server/winreg/srv_winreg_nt.c +++ b/source3/rpc_server/winreg/srv_winreg_nt.c @@ -25,7 +25,6 @@ #include "../librpc/gen_ndr/srv_winreg.h" #include "registry.h" #include "registry/reg_api.h" -#include "registry/reg_api_regf.h" #include "registry/reg_perfcount.h" #include "rpc_misc.h" #include "auth.h" diff --git a/source3/rpc_server/wscript_build b/source3/rpc_server/wscript_build index ae75e567ace..85655d340a3 100644 --- a/source3/rpc_server/wscript_build +++ b/source3/rpc_server/wscript_build @@ -131,7 +131,7 @@ bld.SAMBA3_SUBSYSTEM('RPC_SVCCTL', bld.SAMBA3_SUBSYSTEM('RPC_WINREG', source='''winreg/srv_winreg_nt.c ../../librpc/gen_ndr/srv_winreg.c''', - deps='REG_FULL REGFIO REG_API_REGF NDR_PERFCOUNT') + deps='REG_FULL REGFIO NDR_PERFCOUNT') bld.SAMBA3_SUBSYSTEM('RPC_WKSSVC', source='''wkssvc/srv_wkssvc_nt.c diff --git a/source3/wscript_build b/source3/wscript_build index 317896eee46..a8c0fac513a 100644 --- a/source3/wscript_build +++ b/source3/wscript_build @@ -209,10 +209,6 @@ bld.SAMBA_BINARY('test_registry_regfio', local_include=False, install=False) -bld.SAMBA3_SUBSYSTEM('REG_API_REGF', - source='registry/reg_api_regf.c', - deps='samba-util') - # Do not link against this use 'smbconf' bld.SAMBA3_SUBSYSTEM('SMBREGISTRY', source=''' -- 2.21.0.392.gf8f6787159e-goog