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 <public@hansmi.ch>
Reviewed-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
(cherry picked from commit 601afd690346087fbd53819dba9b1afa81560064)

4 files changed, 51 insertions, 1 deletions

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 00000000000..e74d6784239
--- /dev/null
+++ b/testdata/samba3/regfio_corrupt_hbin1.dat
diff --git a/testdata/samba3/regfio_corrupt_lf_subkeys.dat b/testdata/samba3/regfio_corrupt_lf_subkeys.dat
new file mode 100644
index 00000000000..c540051f7f1
--- /dev/null
+++ b/testdata/samba3/regfio_corrupt_lf_subkeys.dat