From 9c3d24f7347f6f9f7a8c5fdf0c7f649ce5fb927a Mon Sep 17 00:00:00 2001
From: Michael Orlitzky <michael@orlitzky.com>
Date: Tue, 11 Dec 2018 00:40:17 -0500
Subject: [PATCH] src/libadacl.c: only update entries with matching qualifiers.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

This fixes the bug reported by Michał Bartoszkiewicz wherein multiple
named user/group entries clobber one another. Now, when updating an
existing ACL entry with a given one, we check that their qualifiers
match. This can mean one of three things:

  1. Both entries are named user entries, and their UIDs match.

  2. Both entries are named group entries, and their GIDs match.

  3. Both entries are neither named user or group entries, and
     their qualifiers "match vacuously." That is to say: they
     don't have any qualifiers that should match in the first
     place, so we want to update the entry regardless.

This passes the regression test case that I recently added, and (I
hope) fixes the problem entirely.
---
 src/libadacl.c | 64 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 7 deletions(-)

diff --git a/src/libadacl.c b/src/libadacl.c
index 5bca89f..aa473c4 100644
--- a/src/libadacl.c
+++ b/src/libadacl.c
@@ -259,6 +259,17 @@ int acl_update_entry(acl_t aclp, acl_entry_t entry) {
     return ACL_ERROR;
   }
 
+  /* This can allocate memory, so from here on out we have to jump to
+     the "cleanup" label to exit. */
+  void* entry_qualifier = acl_get_qualifier(entry);
+  if (entry_qualifier == NULL &&
+      (entry_tag == ACL_USER || entry_tag == ACL_GROUP)) {
+    /* acl_get_qualifier() can return NULL, but it shouldn't for
+       ACL_USER or ACL_GROUP entries. */
+    perror("acl_update_entry (acl_get_qualifier)");
+    return ACL_ERROR;
+  }
+
   /* Our return value. Default to failure, and change to success if we
      actually update something. */
   int result = ACL_FAILURE;
@@ -277,15 +288,53 @@ int acl_update_entry(acl_t aclp, acl_entry_t entry) {
     }
 
     if (existing_tag == entry_tag) {
-      /* If we update something, we're done and return ACL_SUCCESS */
-      if (acl_set_permset(existing_entry, entry_permset) == ACL_ERROR) {
-	perror("acl_update_entry (acl_set_permset)");
-	result = ACL_ERROR;
-	goto cleanup;
+      /* Our tag types match, but if we have a named user or group
+         entry, then we need to check that the user/group (that is,
+         the qualifier) matches too. */
+      bool qualifiers_match = false;
+
+      /* There are three ways the qualifiers can match... */
+      void* existing_qualifier = acl_get_qualifier(existing_entry);
+      if (existing_qualifier == NULL) {
+        if (existing_tag == ACL_USER || existing_tag == ACL_GROUP) {
+          perror("acl_update_entry (acl_get_qualifier)");
+          result = ACL_ERROR;
+          goto cleanup;
+        }
+        else {
+          /* First, we could be dealing with an entry that isn't a
+             named user or group, in which case they "match
+             vacuously." */
+          qualifiers_match = true;
+        }
       }
 
-      result = ACL_SUCCESS;
-      goto cleanup;
+      /* Otherwise, we have to have matching UIDs or GIDs. */
+      if (entry_tag == ACL_USER) {
+        qualifiers_match = ( *((uid_t*)existing_qualifier)
+                             ==
+                             *((uid_t*)entry_qualifier) );
+      }
+      else if (entry_tag == ACL_GROUP) {
+        qualifiers_match = ( *((gid_t*)existing_qualifier)
+                             ==
+                             *((gid_t*)entry_qualifier) );
+      }
+
+      /* Be sure to free this inside the loop, where memory is allocated. */
+      acl_free(existing_qualifier);
+
+      if (qualifiers_match) {
+        /* If we update something, we're done and return ACL_SUCCESS */
+        if (acl_set_permset(existing_entry, entry_permset) == ACL_ERROR) {
+          perror("acl_update_entry (acl_set_permset)");
+          result = ACL_ERROR;
+          goto cleanup;
+        }
+
+	result = ACL_SUCCESS;
+        goto cleanup;
+      }
     }
 
     get_entry_result = acl_get_entry(aclp, ACL_NEXT_ENTRY, &existing_entry);
@@ -299,6 +348,7 @@ int acl_update_entry(acl_t aclp, acl_entry_t entry) {
   }
 
  cleanup:
+  acl_free(entry_qualifier);
   return result;
 }
 
-- 
2.43.2