From 362eace83c2ff7c9235978d782bc5405f55f9fb9 Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Sun, 17 Mar 2019 13:04:52 +0100 Subject: [PATCH 1/6] Fix typos in "valid" s/vald/valid/ BUG: https://bugzilla.samba.org/show_bug.cgi?id=13840 Signed-off-by: Michael Hanselmann Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 305346d360d3c13fd315c1af27b037f46fd10650) --- source3/registry/regfio.c | 2 +- source3/torture/torture.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source3/registry/regfio.c b/source3/registry/regfio.c index 9bb89ff11d4..7e323da9dab 100644 --- a/source3/registry/regfio.c +++ b/source3/registry/regfio.c @@ -178,7 +178,7 @@ static int read_block( REGF_FILE *file, prs_struct *ps, uint32_t file_offset, ui return False; } if ( (returned == 0) && (bytes_read < block_size) ) { - DEBUG(0,("read_block: not a vald registry file ?\n" )); + DEBUG(0,("read_block: not a valid registry file ?\n" )); return False; } diff --git a/source3/torture/torture.c b/source3/torture/torture.c index 018ebba6c52..4abd80253e5 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -9651,7 +9651,7 @@ static bool run_uid_regression_test(int dummy) goto out; } - /* Now try a SMBtdis with the invald vuid set to zero. */ + /* Now try a SMBtdis with the invalid vuid set to zero. */ cli_state_set_uid(cli, 0); /* This should succeed. */ -- 2.21.0.392.gf8f6787159e-goog From 184adf74ccfa49c22858db07963666fd49436b60 Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Sun, 17 Mar 2019 16:20:47 +0100 Subject: [PATCH 2/6] regfio: Use correct function names in debug information BUG: https://bugzilla.samba.org/show_bug.cgi?id=13840 Signed-off-by: Michael Hanselmann Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit aa6b355858a0d8b77bf49384e5329642add1a5ff) --- source3/registry/regfio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source3/registry/regfio.c b/source3/registry/regfio.c index 7e323da9dab..ebc586c50be 100644 --- a/source3/registry/regfio.c +++ b/source3/registry/regfio.c @@ -305,7 +305,7 @@ static bool prs_hbin_block( const char *desc, prs_struct *ps, int depth, REGF_HB { uint32_t block_size2; - prs_debug(ps, depth, desc, "prs_regf_block"); + prs_debug(ps, depth, desc, "prs_hbin_block"); depth++; if ( !prs_uint8s( True, "header", ps, depth, (uint8_t*)hbin->header, sizeof( hbin->header )) ) @@ -1019,7 +1019,7 @@ static bool hbin_prs_key( REGF_FILE *file, REGF_HBIN *hbin, REGF_NK_REC *nk ) int depth = 0; REGF_HBIN *sub_hbin; - prs_debug(&hbin->ps, depth, "", "fetch_key"); + prs_debug(&hbin->ps, depth, "", "prs_key"); depth++; /* get the initial nk record */ @@ -1238,7 +1238,7 @@ out: ZERO_STRUCTP( rb ); rb->fd = -1; - if ( !(rb->mem_ctx = talloc_init( "read_regf_block" )) ) { + if ( !(rb->mem_ctx = talloc_init( "regfio_open" )) ) { regfio_close( rb ); return NULL; } -- 2.21.0.392.gf8f6787159e-goog From a9397a3f73978d77aa931499080b4c07d1a30626 Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Tue, 19 Mar 2019 00:47:52 +0100 Subject: [PATCH 3/6] regfio: Add trivial unit test An upcoming commit will resolve two cases of insufficient handling of mangled registry hive files and will include unit tests. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13840 Signed-off-by: Michael Hanselmann Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 9b2cb845b23cd1c91ab3b5ea8ad791b18b3ab733) --- selftest/tests.py | 2 + source3/registry/tests/test_regfio.c | 139 +++++++++++++++++++++++++++ source3/wscript_build | 6 ++ 3 files changed, 147 insertions(+) create mode 100644 source3/registry/tests/test_regfio.c diff --git a/selftest/tests.py b/selftest/tests.py index 89e5ff43507..7dbc0a9871f 100644 --- a/selftest/tests.py +++ b/selftest/tests.py @@ -208,3 +208,5 @@ plantestsuite("samba.unittests.ms_fnmatch", "none", [os.path.join(bindir(), "default/lib/util/test_ms_fnmatch")]) plantestsuite("samba.unittests.ntlm_check", "none", [os.path.join(bindir(), "default/libcli/auth/test_ntlm_check")]) +plantestsuite("samba.unittests.test_registry_regfio", "none", + [os.path.join(bindir(), "default/source3/test_registry_regfio")]) diff --git a/source3/registry/tests/test_regfio.c b/source3/registry/tests/test_regfio.c new file mode 100644 index 00000000000..ba557a34c98 --- /dev/null +++ b/source3/registry/tests/test_regfio.c @@ -0,0 +1,139 @@ +/* + * Unix SMB/CIFS implementation. + * + * Copyright (C) 2019 Michael Hanselmann + * + * 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 +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "includes.h" +#include "lib/replace/replace.h" +#include "lib/util/samba_util.h" +#include "registry/regfio.h" + +struct test_ctx { + char *tmp_regfile; + int tmp_regfile_fd; + REGF_FILE *rb; +}; + +static int setup_context(void **state) +{ + struct test_ctx *test_ctx; + + test_ctx = talloc_zero(NULL, struct test_ctx); + assert_non_null(test_ctx); + + test_ctx->tmp_regfile_fd = -1; + + *state = test_ctx; + + return 0; +} + +static int setup_context_tempfile(void **state) +{ + struct test_ctx *test_ctx; + int ret; + + ret = setup_context(state); + + if (ret == 0) { + test_ctx = talloc_get_type_abort(*state, struct test_ctx); + + test_ctx->tmp_regfile = talloc_strdup(test_ctx, "/tmp/regfio.XXXXXX"); + assert_non_null(test_ctx->tmp_regfile); + + test_ctx->tmp_regfile_fd = mkstemp(test_ctx->tmp_regfile); + assert_return_code(test_ctx->tmp_regfile_fd, errno); + } + + return ret; +} + +static int teardown_context(void **state) +{ + struct test_ctx *test_ctx = + talloc_get_type_abort(*state, struct test_ctx); + + if (test_ctx->rb) { + regfio_close(test_ctx->rb); + } + + if (test_ctx->tmp_regfile) { + unlink(test_ctx->tmp_regfile); + } + + if (test_ctx->tmp_regfile_fd != -1) { + close(test_ctx->tmp_regfile_fd); + } + + talloc_free(test_ctx); + + return 0; +} + +static void test_regfio_open_new_file(void **state) +{ + struct test_ctx *test_ctx = + talloc_get_type_abort(*state, struct test_ctx); + REGF_NK_REC *root; + struct regval_ctr *values; + struct regsubkey_ctr *subkeys; + WERROR werr; + + test_ctx->rb = regfio_open(test_ctx->tmp_regfile, + O_RDWR | O_CREAT | O_TRUNC, 0600); + assert_non_null(test_ctx->rb); + + root = regfio_rootkey(test_ctx->rb); + assert_null(root); + + werr = regsubkey_ctr_init(NULL, &subkeys); + assert_true(W_ERROR_IS_OK(werr)); + + werr = regval_ctr_init(subkeys, &values); + assert_true(W_ERROR_IS_OK(werr)); + + // Write root key + regfio_write_key(test_ctx->rb, "", values, subkeys, NULL, NULL); + + root = regfio_rootkey(test_ctx->rb); + assert_non_null(root); + assert_memory_equal(root->header, "nk", sizeof(root->header)); + assert_int_equal(root->key_type, NK_TYPE_ROOTKEY); +} + +int main(void) { + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_regfio_open_new_file, + setup_context_tempfile, + teardown_context), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/source3/wscript_build b/source3/wscript_build index f0d85969692..f67ce59fe52 100644 --- a/source3/wscript_build +++ b/source3/wscript_build @@ -204,6 +204,12 @@ bld.SAMBA3_SUBSYSTEM('REGFIO', source='registry/regfio.c', deps='samba-util REG_PARSE_PRS') +bld.SAMBA_BINARY('test_registry_regfio', + source='registry/tests/test_regfio.c', + deps='cmocka samba3-util smbconf REGFIO', + local_include=False, + install=False) + bld.SAMBA3_SUBSYSTEM('REG_API_REGF', source='registry/reg_api_regf.c', deps='samba-util') -- 2.21.0.392.gf8f6787159e-goog From 28a98c6fbf4eed8998f55f6270139ff8eeb76349 Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Sun, 17 Mar 2019 13:49:20 +0100 Subject: [PATCH 4/6] regfio: Improve handling of malformed registry hive files * next_record: A malformed file can lead to an endless loop. * regfio_rootkey: Supplying a malformed registry hive file to the registry hive I/O code can lead to out-of-bounds reads. Test cases are included. Both issues resolved have been identified using AddressSanitizer. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13840 Signed-off-by: Michael Hanselmann Reviewed-by: Douglas Bagnall Reviewed-by: Andrew Bartlett (cherry picked from commit 601afd690346087fbd53819dba9b1afa81560064) --- source3/registry/regfio.c | 7 ++- source3/registry/tests/test_regfio.c | 45 ++++++++++++++++++ testdata/samba3/regfio_corrupt_hbin1.dat | Bin 0 -> 5120 bytes testdata/samba3/regfio_corrupt_lf_subkeys.dat | Bin 0 -> 5120 bytes 4 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 testdata/samba3/regfio_corrupt_hbin1.dat create mode 100644 testdata/samba3/regfio_corrupt_lf_subkeys.dat diff --git a/source3/registry/regfio.c b/source3/registry/regfio.c index ebc586c50be..33b24489e97 100644 --- a/source3/registry/regfio.c +++ b/source3/registry/regfio.c @@ -1132,6 +1132,10 @@ static bool next_record( REGF_HBIN *hbin, const char *hdr, bool *eob ) record_size = (record_size ^ 0xffffffff) + 1; } + if ( record_size < sizeof(REC_HDR_SIZE) ) { + return False; + } + if ( memcmp( header, hdr, REC_HDR_SIZE ) == 0 ) { found = True; curr_off += sizeof(uint32_t); @@ -1433,7 +1437,8 @@ REGF_NK_REC* regfio_rootkey( REGF_FILE *file ) /* see if there is anything left to report */ - if ( !nk || (nk->subkeys_off==REGF_OFFSET_NONE) || (nk->subkey_index >= nk->num_subkeys) ) + if ( !nk || !nk->subkeys.hashes || nk->subkey_index >= nk->subkeys.num_keys || + (nk->subkeys_off==REGF_OFFSET_NONE) || (nk->subkey_index >= nk->num_subkeys) ) return NULL; /* find the HBIN block which should contain the nk record */ diff --git a/source3/registry/tests/test_regfio.c b/source3/registry/tests/test_regfio.c index ba557a34c98..228ce27d15a 100644 --- a/source3/registry/tests/test_regfio.c +++ b/source3/registry/tests/test_regfio.c @@ -95,6 +95,17 @@ static int teardown_context(void **state) return 0; } +static void open_testfile(struct test_ctx *test_ctx, const char *filename) +{ + char *path; + + path = talloc_asprintf(test_ctx, "%s/testdata/samba3/%s", SRCDIR, filename); + assert_non_null(path); + + test_ctx->rb = regfio_open(path, O_RDONLY, 0600); + assert_non_null(test_ctx->rb); +} + static void test_regfio_open_new_file(void **state) { struct test_ctx *test_ctx = @@ -126,11 +137,45 @@ static void test_regfio_open_new_file(void **state) assert_int_equal(root->key_type, NK_TYPE_ROOTKEY); } +static void test_regfio_corrupt_hbin(void **state) +{ + struct test_ctx *test_ctx = + talloc_get_type_abort(*state, struct test_ctx); + REGF_NK_REC *root; + + open_testfile(test_ctx, "regfio_corrupt_hbin1.dat"); + + root = regfio_rootkey(test_ctx->rb); + assert_null(root); +} + +static void test_regfio_corrupt_lf_subkeys(void **state) +{ + struct test_ctx *test_ctx = + talloc_get_type_abort(*state, struct test_ctx); + REGF_NK_REC *root, *subkey; + + open_testfile(test_ctx, "regfio_corrupt_lf_subkeys.dat"); + + root = regfio_rootkey(test_ctx->rb); + assert_non_null(root); + + root->subkey_index = 0; + while ((subkey = regfio_fetch_subkey(test_ctx->rb, root))) { + } +} + int main(void) { const struct CMUnitTest tests[] = { cmocka_unit_test_setup_teardown(test_regfio_open_new_file, setup_context_tempfile, teardown_context), + cmocka_unit_test_setup_teardown(test_regfio_corrupt_hbin, + setup_context, + teardown_context), + cmocka_unit_test_setup_teardown(test_regfio_corrupt_lf_subkeys, + setup_context, + teardown_context), }; cmocka_set_message_output(CM_OUTPUT_SUBUNIT); diff --git a/testdata/samba3/regfio_corrupt_hbin1.dat b/testdata/samba3/regfio_corrupt_hbin1.dat new file mode 100644 index 0000000000000000000000000000000000000000..e74d678423971a2fe752bd6971e64d14249ea264 GIT binary patch literal 5120 zcmeHLO=uHQ5S~r#A5^O`O4EvLRnUXT#-s@mv6QypL2OA|z4TzKNh3`+F-apx(XAeA zp+}(?4=sW}AX4$-K`@>~D*drNh*a9`&B)&k;wJJsUXPW5yI&o+vYpQ3oSQv-Sd>Y!c9DoE?sw4i*GO7bb!UN>lxnSpi4Kw=$ynl8-@m>F2z z47l8iqPQH68js@ix?Enx8d8yk zv;Wgn!|eRAS2L0YR%198>nG!dVu|?p%#)c%@b$rbcOJ9?=zL90d<{S&-_vQ8WIFAP z^u3>+5#KB_^o-A>T{GfVU^3(G%nTczrI6|(k;a5yjmL0xAwyn-Z73*F5*HaCA3fa5 zaj4zD_cy0y{2j#H=Il<%o7 zryy(dllM7@7M7EnR!C#}<&~~9&vRSs!7-2dT6=a826d@aDH75-E zS}Hf_Wudoa^qDWhLaV?q=(r>x18zC{!ju}vVUN?}Rorg3*X{7sc)e`83H5hqwV@|x zPaK_Vo*ph6?JSqZ?UygzPJVee{K0;!Y$A7kybdYH|46nDS1c1^KQW|?hExa&R^_jI zWp+H5@f_449u0@&<_^^#f}8?=qPvursj)3MVoA2O1j85W!@X)85xF|82@H%O~qKVHC>GuOTmMM>z~q)L{y51lvVTMA+23EcAVXISBtV5>!5*` zD9}TPBnXQLrEB1J48uR^VS@rq2v1>9WFl<8_htuXY&+Q=g8B5#_vd}@_r3S~W)Jg% z?jFxRB4R@m0fsg1Fk57HRthn@lj`u-Ney%Y&vputn}T?DQX?!6bx|Kl6o!9}+A!Zr zP2{4Zn5~CTv@uh#ymfwU@6fobjaEZB+kIB**S4HsZ@4#g@a3VoXG=n}$JN93Vam=K zxN`4vZ09H@g2{l%fXRT#z<|`^bB)h;r!uNJxmN`^bV!(-5EGGL(X})o3<>dQC!Gg5OG$Q(wqeuI6TT4M2~PBl|?^s0a*vU0CFw^djO6pNVo;FyQ8;sJQ#p zX~Q!eUVR|Y)bEzVAzWRE@YjJac?F8#BIEn3sl)0y{$6icQR|grZDd#=tzJKTCS@2! z+#*7c{6%`cbQ35XYP?LYw`2>wuzas z2V=#nKcw87XEMS;za+MF$!-a2qUb@tFP(4{h2cTtypi@+um5U;zfTV1pl@eS`!|p` zkNs;sOv8(Ir*I^|;v2_^TH?K2_gIL%_YUMf}9s0Z)z{|4KdyxJ=c00@Cko&zX1;s7(W02 literal 0 HcmV?d00001 -- 2.21.0.392.gf8f6787159e-goog From b085b582e359551caa7fa9ce82f708483be310fa Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 20 Mar 2019 17:32:39 +1300 Subject: [PATCH 5/6] regfio: Update code near recent changes to match README.Coding This file long predates our current code conventions. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13840 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit acbf103fcaa4150a57bfbab2450e36b5b39e399b) --- source3/registry/regfio.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/source3/registry/regfio.c b/source3/registry/regfio.c index 33b24489e97..32e166a1223 100644 --- a/source3/registry/regfio.c +++ b/source3/registry/regfio.c @@ -1127,16 +1127,16 @@ static bool next_record( REGF_HBIN *hbin, const char *hdr, bool *eob ) if ( !prs_uint8s( True, "header", ps, 0, header, REC_HDR_SIZE ) ) return False; - if ( record_size & 0x80000000 ) { + if (record_size & 0x80000000) { /* absolute_value(record_size) */ record_size = (record_size ^ 0xffffffff) + 1; } - if ( record_size < sizeof(REC_HDR_SIZE) ) { - return False; + if (record_size < sizeof(REC_HDR_SIZE)) { + return false; } - if ( memcmp( header, hdr, REC_HDR_SIZE ) == 0 ) { + if (memcmp(header, hdr, REC_HDR_SIZE) == 0) { found = True; curr_off += sizeof(uint32_t); } @@ -1437,13 +1437,19 @@ REGF_NK_REC* regfio_rootkey( REGF_FILE *file ) /* see if there is anything left to report */ - if ( !nk || !nk->subkeys.hashes || nk->subkey_index >= nk->subkeys.num_keys || - (nk->subkeys_off==REGF_OFFSET_NONE) || (nk->subkey_index >= nk->num_subkeys) ) + if (nk == NULL || + nk->subkeys.hashes == NULL || + nk->subkey_index >= nk->subkeys.num_keys || + (nk->subkeys_off == REGF_OFFSET_NONE) || + (nk->subkey_index >= nk->num_subkeys)) { return NULL; + } /* find the HBIN block which should contain the nk record */ - - if ( !(hbin = lookup_hbin_block( file, nk->subkeys.hashes[nk->subkey_index].nk_off )) ) { + + hbin = lookup_hbin_block(file, + nk->subkeys.hashes[nk->subkey_index].nk_off); + if (hbin == NULL) { DEBUG(0,("hbin_prs_key: Failed to find HBIN block containing offset [0x%x]\n", nk->subkeys.hashes[nk->subkey_index].nk_off)); return NULL; -- 2.21.0.392.gf8f6787159e-goog From 40745b0bb8f117766cd2e8a3573c540531bdfbe0 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 20 Mar 2019 17:33:46 +1300 Subject: [PATCH 6/6] regfio tests: Update comment style to match README.Coding BUG: https://bugzilla.samba.org/show_bug.cgi?id=13840 Signed-off-by: Andrew Bartlett Reviewed-by: Douglas Bagnall (cherry picked from commit 68c0fc4335d0c3c526a38481538a33290be6d58a) --- source3/registry/tests/test_regfio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/registry/tests/test_regfio.c b/source3/registry/tests/test_regfio.c index 228ce27d15a..86a217661f3 100644 --- a/source3/registry/tests/test_regfio.c +++ b/source3/registry/tests/test_regfio.c @@ -128,7 +128,7 @@ static void test_regfio_open_new_file(void **state) werr = regval_ctr_init(subkeys, &values); assert_true(W_ERROR_IS_OK(werr)); - // Write root key + /* Write root key */ regfio_write_key(test_ctx->rb, "", values, subkeys, NULL, NULL); root = regfio_rootkey(test_ctx->rb); -- 2.21.0.392.gf8f6787159e-goog