- Allow extra data after the SID.  Recent Windows 10 images have been
  reported to contain DACLs with an ACCESS_ALLOWED_CALLBACK_ACE with
  extra data after the SID.  The ACE is not necessarily at the end of
  the DACL, so the recent fix to allow extra data for the last ACE only
  was insufficient.  I also suspect extra data is sometimes used by
  other ACE types.  In fact, SetFileSecurity() on Windows permits extra
  data for all ACE types.  So make NTFS-3G do the same.

- Validate the SID at the correct offset for "object" ACEs.  (Most
  likely this bug wasn't noticed because object ACEs are rarely used.)

- Only validate the SID for recognized ACE types.  The placement or
  presence of the SID should not be assumed for future ACE types.

Signed-off-by: Eric Biggers <ebigge...@gmail.com>
---
 libntfs-3g/acls.c | 114 +++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 82 insertions(+), 32 deletions(-)

diff --git a/libntfs-3g/acls.c b/libntfs-3g/acls.c
index b91e041..06c44d9 100644
--- a/libntfs-3g/acls.c
+++ b/libntfs-3g/acls.c
@@ -536,45 +536,95 @@ gid_t ntfs_find_group(const struct MAPPING* groupmapping, 
const SID * gsid)
 }
 
 /*
+ *             Does the ACE have the same format as ACCESS_ALLOWED_ACE?
+ */
+
+static BOOL is_regular_ace(const ACE_HEADER *pace)
+{
+       switch (pace->type) {
+       case ACCESS_ALLOWED_ACE_TYPE:
+       case ACCESS_DENIED_ACE_TYPE:
+       case SYSTEM_AUDIT_ACE_TYPE:
+       case ACCESS_ALLOWED_CALLBACK_ACE_TYPE:
+       case ACCESS_DENIED_CALLBACK_ACE_TYPE:
+       case SYSTEM_AUDIT_CALLBACK_ACE_TYPE:
+       case SYSTEM_MANDATORY_LABEL_ACE_TYPE:
+       case SYSTEM_RESOURCE_ATTRIBUTE_ACE_TYPE:
+       case SYSTEM_SCOPED_POLICY_ID_ACE_TYPE:
+       case SYSTEM_PROCESS_TRUST_LABEL_ACE_TYPE:
+               return TRUE;
+       default:
+               return FALSE;
+       }
+}
+
+/*
+ *             Does the ACE have the same format as ACCESS_ALLOWED_OBJECT_ACE?
+ */
+
+static BOOL is_object_ace(const ACE_HEADER *pace)
+{
+       switch (pace->type) {
+       case ACCESS_ALLOWED_OBJECT_ACE_TYPE:
+       case ACCESS_DENIED_OBJECT_ACE_TYPE:
+       case SYSTEM_AUDIT_OBJECT_ACE_TYPE:
+       case ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE:
+       case ACCESS_DENIED_CALLBACK_OBJECT_ACE_TYPE:
+       case SYSTEM_AUDIT_CALLBACK_OBJECT_ACE_TYPE:
+               return TRUE;
+       default:
+               return FALSE;
+       }
+}
+
+/*
  *             Check the validity of the ACEs in a DACL or SACL
+ *
+ *     If an ACE is recognized, we validate its SID.
+ *     Otherwise, we validate its size only.
  */
 
 static BOOL valid_acl(const ACL *pacl, unsigned int end)
 {
-       const ACCESS_ALLOWED_ACE *pace;
-       unsigned int offace;
-       unsigned int acecnt;
-       unsigned int acesz;
-       unsigned int nace;
-       unsigned int wantsz;
-       BOOL ok;
+       unsigned int ace_count;
+       unsigned int ace_offset;
+       unsigned int ace_size;
+       unsigned int sid_offset;
+       const ACE_HEADER *pace;
+       const SID *psid;
 
-       ok = TRUE;
-       acecnt = le16_to_cpu(pacl->ace_count);
-       offace = sizeof(ACL);
-       for (nace = 0; (nace < acecnt) && ok; nace++) {
-               /* be sure the beginning is within range */
-               if ((offace + sizeof(ACCESS_ALLOWED_ACE)) > end)
-                       ok = FALSE;
-               else {
-                       pace = (const ACCESS_ALLOWED_ACE*)
-                               &((const char*)pacl)[offace];
-                       acesz = le16_to_cpu(pace->size);
-                       if (((offace + acesz) > end)
-                          || !ntfs_valid_sid(&pace->sid))
-                                ok = FALSE;
-                       else {
-                               /* Win10 may insert garbage in the last ACE */
-                               wantsz = ntfs_sid_size(&pace->sid) + 8;
-                               if (((nace < (acecnt - 1))
-                                       && (wantsz != acesz))
-                                   || (wantsz > acesz))
-                                       ok = FALSE;
-                       }
-                       offace += acesz;
-               }
+       for (ace_count = le16_to_cpu(pacl->ace_count), ace_offset = sizeof(ACL);
+            ace_count != 0;
+            ace_count--, ace_offset += ace_size)
+       {
+               if (sizeof(ACE_HEADER) > end - ace_offset)
+                       return FALSE;
+
+               pace = (const ACE_HEADER *)((char *)pacl + ace_offset);
+               ace_size = le16_to_cpu(pace->size);
+               if (ace_size < sizeof(ACE_HEADER) ||
+                   ace_size > end - ace_offset)
+                       return FALSE;
+
+               if (is_regular_ace(pace))
+                       sid_offset = offsetof(ACCESS_ALLOWED_ACE, sid);
+               else if (is_object_ace(pace))
+                       sid_offset = offsetof(ACCESS_ALLOWED_OBJECT_ACE, sid);
+               else
+                       continue;
+
+               if (ace_size < sid_offset + sizeof(SID))
+                       return FALSE;
+
+               psid = (const SID *)((char *)pace + sid_offset);
+
+               /* Note: there may be additional data after the SID. */
+               if (!ntfs_valid_sid(psid) ||
+                   ace_size < sid_offset + ntfs_sid_size(psid))
+                       return FALSE;
        }
-       return (ok);
+
+       return TRUE;
 }
 
 /*
-- 
2.9.3


------------------------------------------------------------------------------
_______________________________________________
ntfs-3g-devel mailing list
ntfs-3g-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel

Reply via email to