The dirsync code is misleading and contains dead code that is never hit (due to a single incorrect line of code). However, if this incorrect line of code were corrected, then it could turn into a security hole.

The problematic piece of code is in dirsync.c, dirsync_filter_entry() (line ~182).

	if (replMetaData == NULL) {
		bool guidfound = false;

		/*
		 * We are in the case of deleted object where we don't have the
		 * right to read it.
		 */
		if (!ldb_msg_find_attr_as_uint(msg, "isDeleted", 0)) {

What's happening here is that acl_read.c has checked the object/attribute and determined that the user doesn't have access rights to see it. acl_read.c makes a special-case for dirsync, and instead of suppressing the search result, it strips off the replPropertyMetaData and passes the object back to dirsync to handle.

The dirsync code looks like it will then suppress non-deleted objects, and return just the objectGUID for deleted objects. However, this isn't actually happening because the check 'ldb_msg_find_attr_as_uint(msg, "isDeleted", 0)' is trying to read a boolean value (i.e. "TRUE") as an integer, which fails and falls back to the default value (zero, or not-deleted).

If this line were corrected, it opena possible security hole, i.e. it would be possible to guess the values of confidential attributes for deleted objects.

The test_search_with_dirsync_deleted_objects() test-case in confidential_attr.py should now exercise this particular behaviour (it'll fail if the code were ever corrected to return the objectGUID).

This bug might be suitable for a new samba developer (with appropriate supervision). Work involved:
- First, the trivial part is to just delete the dead code. Change the 'if (replMetaData == NULL)' block to simply always returns LDB_SUCCESS (i.e. always drop the message).
- Secondly, the more complicated part is to look into the interaction between acl_read.c and dirsync.c more. Is there any reason for this special replPropertyMetaData handling? I can think of reasons why we might want to do this (i.e. updating the highestUSN seen). However, it looks like the dirsync code drops these replPropertyMetaData-less results before it actually updates any of its global state. In other words, the ac->indirsync special cases in acl_read.c might be completely pointless, and could be deleted as well.

Removing embargo now CVE-2018-10919 is public