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>

diff --git a/source3/registry/regfio.c b/source3/registry/regfio.c
index ebc586c50be138ae60bb087ed92596b995e6a507..33b24489e973de4aff112ec1bba2637ed4adf32b 100644 (file)
--- 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 ba557a34c98d38d0bb6331c1e5214224a079cdaf..228ce27d15a43386ee0ec758b32e1a12b8ca1c43 100644 (file)
--- 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 (file)
index 0000000..e74d678
Binary files /dev/null and b/testdata/samba3/regfio_corrupt_hbin1.dat differ

diff --git a/testdata/samba3/regfio_corrupt_lf_subkeys.dat b/testdata/samba3/regfio_corrupt_lf_subkeys.dat
new file mode 100644 (file)
index 0000000..c540051
Binary files /dev/null and b/testdata/samba3/regfio_corrupt_lf_subkeys.dat differ