From: Eric Biggers Date: Mon, 7 Oct 2013 06:55:07 +0000 (-0500) Subject: Implement workaround for NTFS-3g bug when handling empty DACLs X-Git-Tag: v1.5.1~5 X-Git-Url: https://wimlib.net/git/?p=wimlib;a=commitdiff_plain;h=01be73ad93236a0f6cf7e000b4f8ac91fea6dff3 Implement workaround for NTFS-3g bug when handling empty DACLs --- diff --git a/Makefile.am b/Makefile.am index d2e80a2f..fae1fe8d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -79,6 +79,7 @@ libwim_la_SOURCES = \ include/wimlib/reparse.h \ include/wimlib/resource.h \ include/wimlib/security.h \ + include/wimlib/security_descriptor.h \ include/wimlib/sha1.h \ include/wimlib/timestamp.h \ include/wimlib/types.h \ diff --git a/include/wimlib/security_descriptor.h b/include/wimlib/security_descriptor.h new file mode 100644 index 00000000..415de864 --- /dev/null +++ b/include/wimlib/security_descriptor.h @@ -0,0 +1,116 @@ +#ifndef _WIMLIB_SECURITY_DESCRIPTOR_H +#define _WIMLIB_SECURITY_DESCRIPTOR_H + +#include "wimlib/compiler.h" +#include "wimlib/types.h" + +/* Note: the data types in this header are prefixed with wimlib_ to avoid + * conflicts with the same types being defined in the libntfs-3g headers. */ + +/* Windows NT security descriptor, in self-relative format */ +typedef struct { + /* Security descriptor revision; should be 1 */ + u8 revision; + + /* Padding */ + u8 sbz1; + + /* Bitwise OR of flags defined below, such as SE_DACL_PRESENT */ + le16 control; + + /* Offset of owenr SID structure in the security descriptor */ + le32 owner_offset; + + /* Offset of group SID structure in the security descriptor */ + le32 group_offset; + + /* Offset of System Access Control List (SACL) in security descriptor, + * or 0 if no SACL is present */ + le32 sacl_offset; + + /* Offset of Discretionary Access Control List (DACL) in security + * descriptor, or 0 if no DACL is present */ + le32 dacl_offset; +} _packed_attribute wimlib_SECURITY_DESCRIPTOR_RELATIVE; + +#define wimlib_SE_OWNER_DEFAULTED 0x0001 +#define wimlib_SE_GROUP_DEFAULTED 0x0002 +#define wimlib_SE_DACL_PRESENT 0x0004 +#define wimlib_SE_DACL_DEFAULTED 0x0008 +#define wimlib_SE_SACL_PRESENT 0x0010 +#define wimlib_SE_SACL_DEFAULTED 0x0020 +#define wimlib_SE_DACL_AUTO_INHERIT_REQ 0x0100 +#define wimlib_SE_SACL_AUTO_INHERIT_REQ 0x0200 +#define wimlib_SE_DACL_AUTO_INHERITED 0x0400 +#define wimlib_SE_SACL_AUTO_INHERITED 0x0800 +#define wimlib_SE_DACL_PROTECTED 0x1000 +#define wimlib_SE_SACL_PROTECTED 0x2000 +#define wimlib_SE_RM_CONTROL_VALID 0x4000 +#define wimlib_SE_SELF_RELATIVE 0x8000 + +/* Header of a Windows NT access control entry */ +typedef struct { + /* Type of ACE */ + u8 type; + + /* Bitwise OR of inherit ACE flags */ + u8 flags; + + /* Size of the access control entry, including this header */ + le16 size; +} _packed_attribute wimlib_ACE_HEADER; + +/* Windows NT access control entry to grant rights to a user or group */ +typedef struct { + wimlib_ACE_HEADER hdr; + le32 mask; + le32 sid_start; +} _packed_attribute wimlib_ACCESS_ALLOWED_ACE; + +/* Windows NT access control entry to deny rights to a user or group */ +typedef struct { + wimlib_ACE_HEADER hdr; + le32 mask; + le32 sid_start; +} _packed_attribute wimlib_ACCESS_DENIED_ACE; + +/* Windows NT access control entry to audit access to the object */ +typedef struct { + wimlib_ACE_HEADER hdr; + le32 mask; + le32 sid_start; +} _packed_attribute wimlib_SYSTEM_AUDIT_ACE; + + +/* Header of a Windows NT access control list */ +typedef struct { + /* ACL_REVISION or ACL_REVISION_DS */ + u8 revision; + + /* padding */ + u8 sbz1; + + /* Total size of the ACL, including all access control entries */ + le16 acl_size; + + /* Number of access control entry structures that follow the ACL + * structure */ + le16 ace_count; + + /* padding */ + le16 sbz2; +} _packed_attribute wimlib_ACL; + +/* Windows NT security identifier (user or group) */ +typedef struct { + + u8 revision; + u8 sub_authority_count; + + /* Identifies the authority that issued the SID */ + u8 identifier_authority[6]; + + le32 sub_authority[]; +} _packed_attribute wimlib_SID; + +#endif diff --git a/src/ntfs-3g_apply.c b/src/ntfs-3g_apply.c index 28589698..bf122a81 100644 --- a/src/ntfs-3g_apply.c +++ b/src/ntfs-3g_apply.c @@ -50,6 +50,7 @@ #include "wimlib/ntfs_3g.h" #include "wimlib/paths.h" #include "wimlib/resource.h" +#include "wimlib/security_descriptor.h" static ntfs_volume * ntfs_3g_apply_ctx_get_volume(struct apply_ctx *ctx) @@ -379,6 +380,114 @@ out: return ret; } +static size_t +sid_size(const wimlib_SID *sid) +{ + return offsetof(wimlib_SID, sub_authority) + + sizeof(le32) * sid->sub_authority_count; +} + +/* + * sd_fixup - Fix up a Windows NT security descriptor for libntfs-3g. + * + * libntfs-3g validates security descriptors before setting them, but old + * versions contain bugs causing it to reject unusual but valid security + * descriptors: + * + * - Versions before 2013.1.13 reject security descriptors ending with an empty + * SACL (System Access Control List). This bug can be worked around either by + * moving the empty SACL earlier in the security descriptor or by removing the + * SACL entirely. The latter work-around is valid because an empty SACL is + * equivalent to a "null", or non-existent, SACL. + * - Versions up to and including 2013.1.13 reject security descriptors ending + * with an empty DACL (Discretionary Access Control List). This is very + * similar to the SACL bug and should be fixed in the next release after + * 2013.1.13. However, removing the DACL is not a valid workaround because + * this changes the meaning of the security descriptor--- an empty DACL allows + * no access, whereas a "null" DACL allows all access. + * + * If the security descriptor was fixed, this function returns an allocated + * buffer containing the fixed security descriptor, and its size is updated. + * Otherwise (or if no memory is available) the original descriptor is returned. + */ +static u8 * +sd_fixup(const u8 *_desc, size_t *size_p) +{ + u32 owner_offset, group_offset, dacl_offset, sacl_offset; + bool owner_valid, group_valid; + size_t size = *size_p; + const wimlib_SECURITY_DESCRIPTOR_RELATIVE *desc = + (const wimlib_SECURITY_DESCRIPTOR_RELATIVE*)_desc; + wimlib_SECURITY_DESCRIPTOR_RELATIVE *desc_new; + u32 sid_offset; + const wimlib_SID *owner, *group, *sid; + + /* Don't attempt to fix clearly invalid security descriptors. */ + if (size < sizeof(wimlib_SECURITY_DESCRIPTOR_RELATIVE)) + return (u8*)_desc; + + if (le16_to_cpu(desc->control) & wimlib_SE_DACL_PRESENT) + dacl_offset = le32_to_cpu(desc->dacl_offset); + else + dacl_offset = 0; + + if (le16_to_cpu(desc->control) & wimlib_SE_SACL_PRESENT) + sacl_offset = le32_to_cpu(desc->sacl_offset); + else + sacl_offset = 0; + + /* Check if the security descriptor will be affected by one of the bugs. + * If not, do nothing and return. + * + * Note: HAVE_NTFS_MNT_RDONLY is defined if libntfs-3g is + * version 2013.1.13 or later. */ + if (!( + #if !defined(HAVE_NTFS_MNT_RDONLY) + (sacl_offset != 0 && sacl_offset == size - sizeof(wimlib_ACL)) || + #endif + (dacl_offset != 0 && dacl_offset == size - sizeof(wimlib_ACL)))) + return (u8*)_desc; + + owner_offset = le32_to_cpu(desc->owner_offset); + group_offset = le32_to_cpu(desc->group_offset); + owner = (const wimlib_SID*)((const u8*)desc + owner_offset); + group = (const wimlib_SID*)((const u8*)desc + group_offset); + + /* We'll try to move the owner or group SID to the end of the security + * descriptor to avoid the bug. This is only possible if at least one + * is valid. */ + owner_valid = (owner_offset != 0) && + (owner_offset % 4 == 0) && + (owner_offset <= size - sizeof(SID)) && + (owner_offset + sid_size(owner) <= size) && + (owner_offset >= sizeof(wimlib_SECURITY_DESCRIPTOR_RELATIVE)); + group_valid = (group_offset != 0) && + (group_offset % 4 == 0) && + (group_offset <= size - sizeof(SID)) && + (group_offset + sid_size(group) <= size) && + (group_offset >= sizeof(wimlib_SECURITY_DESCRIPTOR_RELATIVE)); + if (owner_valid) { + sid = owner; + } else if (group_valid) { + sid = group; + } else { + return (u8*)_desc; + } + + desc_new = MALLOC(size + sid_size(sid)); + if (desc_new == NULL) + return (u8*)_desc; + + memcpy(desc_new, desc, size); + if (owner_valid) + desc_new->owner_offset = cpu_to_le32(size); + else if (group_valid) + desc_new->group_offset = cpu_to_le32(size); + memcpy((u8*)desc_new + size, sid, sid_size(sid)); + *size_p = size + sid_size(sid); + return (u8*)desc_new; +} + static int ntfs_3g_set_security_descriptor(const char *path, const u8 *desc, size_t desc_size, struct apply_ctx *ctx) @@ -386,7 +495,8 @@ ntfs_3g_set_security_descriptor(const char *path, const u8 *desc, size_t desc_si ntfs_volume *vol; ntfs_inode *ni; struct SECURITY_CONTEXT sec_ctx; - int ret = 0; + u8 *desc_fixed; + int ret; vol = ntfs_3g_apply_ctx_get_volume(ctx); @@ -397,10 +507,19 @@ ntfs_3g_set_security_descriptor(const char *path, const u8 *desc, size_t desc_si memset(&sec_ctx, 0, sizeof(sec_ctx)); sec_ctx.vol = vol; - if (ntfs_set_ntfs_acl(&sec_ctx, ni, desc, desc_size, 0)) + desc_fixed = sd_fixup(desc, &desc_size); + + ret = 0; + + if (ntfs_set_ntfs_acl(&sec_ctx, ni, desc_fixed, desc_size, 0)) ret = WIMLIB_ERR_SET_SECURITY; + + if (desc_fixed != desc) + FREE(desc_fixed); + if (ntfs_inode_close(ni)) ret = WIMLIB_ERR_WRITE; + return ret; } diff --git a/src/security.c b/src/security.c index 6ed83cd7..aab9808e 100644 --- a/src/security.c +++ b/src/security.c @@ -31,136 +31,16 @@ #include "wimlib/endianness.h" #include "wimlib/error.h" #include "wimlib/security.h" +#include "wimlib/security_descriptor.h" #include "wimlib/sha1.h" #include "wimlib/util.h" -/* At the start of each type of access control entry. */ -typedef struct _ACE_HEADER { - /* enum ace_type, specifies what type of ACE this is. */ - u8 type; - - /* bitwise OR of the inherit ACE flags #defined above */ - u8 flags; - - /* Size of the access control entry. */ - le16 size; -} _packed_attribute ACE_HEADER; - -/* Grants rights to a user or group */ -typedef struct _ACCESS_ALLOWED_ACE { - ACE_HEADER hdr; - le32 mask; - le32 sid_start; -} _packed_attribute ACCESS_ALLOWED_ACE; - -/* Denies rights to a user or group */ -typedef struct _ACCESS_DENIED_ACE { - ACE_HEADER hdr; - le32 mask; - le32 sid_start; -} _packed_attribute ACCESS_DENIED_ACE; - -typedef struct _SYSTEM_AUDIT_ACE { - ACE_HEADER hdr; - le32 mask; - le32 sid_start; -} _packed_attribute SYSTEM_AUDIT_ACE; - - -/* Header of an access control list. */ -typedef struct _ACL { - /* ACL_REVISION or ACL_REVISION_DS */ - u8 revision; - - /* padding */ - u8 sbz1; - - /* Total size of the ACL, including all access control entries */ - le16 acl_size; - - /* Number of access control entry structures that follow the ACL - * structure. */ - le16 ace_count; - - /* padding */ - le16 sbz2; -} _packed_attribute ACL; - -/* A structure used to identify users or groups. */ -typedef struct _SID { - - /* example: 0x1 */ - u8 revision; - u8 sub_authority_count; - - /* Identifies the authority that issued the SID. Can be, but does not - * have to be, one of enum sid_authority_value */ - u8 identifier_authority[6]; - - le32 sub_authority[]; -} _packed_attribute SID; - -typedef struct _SECURITY_DESCRIPTOR_RELATIVE { - /* Example: 0x1 */ - u8 revision; - /* Example: 0x0 */ - u8 sbz1; - - /* Example: 0x4149 */ - le16 security_descriptor_control; - - /* Offset of a SID structure in the security descriptor. */ - /* Example: 0x14 */ - le32 owner_offset; - - /* Offset of a SID structure in the security descriptor. */ - /* Example: 0x24 */ - le32 group_offset; - - /* Offset of an ACL structure in the security descriptor. */ - /* System ACL. */ - /* Example: 0x00 */ - le32 sacl_offset; - - /* Offset of an ACL structure in the security descriptor. */ - /* Discretionary ACL. */ - /* Example: 0x34 */ - le32 dacl_offset; -} _packed_attribute SECURITY_DESCRIPTOR_RELATIVE; - struct wim_security_data_disk { le32 total_length; le32 num_entries; le64 sizes[]; } _packed_attribute; -/* - * This is a hack to work around a problem in libntfs-3g. libntfs-3g validates - * security descriptors with a function named ntfs_valid_descr(). - * ntfs_valid_descr() considers a security descriptor that ends in a SACL - * (Sysetm Access Control List) with no ACE's (Access Control Entries) to be - * invalid. However, a security descriptor like this exists in the Windows 7 - * install.wim. Here, security descriptors matching this pattern are modified - * to have no SACL. This should make no difference since the SACL had no - * entries anyway; however this ensures that that the security descriptors pass - * the validation in libntfs-3g. - */ -static void -empty_sacl_fixup(SECURITY_DESCRIPTOR_RELATIVE *descr, u64 *size_p) -{ - /* No-op if no NTFS-3g support, or if NTFS-3g is version 2013 or later - * */ -#if defined(WITH_NTFS_3G) && !defined(HAVE_NTFS_MNT_RDONLY) - if (*size_p >= sizeof(SECURITY_DESCRIPTOR_RELATIVE)) { - u32 sacl_offset = le32_to_cpu(descr->sacl_offset); - if (sacl_offset == *size_p - sizeof(ACL)) { - descr->sacl_offset = cpu_to_le32(0); - *size_p -= sizeof(ACL); - } - } -#endif -} - struct wim_security_data * new_wim_security_data(void) { @@ -276,8 +156,6 @@ read_wim_security_data(const u8 metadata_resource[], size_t metadata_resource_le if (!sd->descriptors[i]) goto out_of_memory; p += sd->sizes[i]; - empty_sacl_fixup((SECURITY_DESCRIPTOR_RELATIVE*)sd->descriptors[i], - &sd->sizes[i]); } out_align_total_length: total_len = (total_len + 7) & ~7; @@ -338,11 +216,11 @@ write_wim_security_data(const struct wim_security_data * restrict sd, } static void -print_acl(const ACL *acl, const tchar *type, size_t max_size) +print_acl(const wimlib_ACL *acl, const tchar *type, size_t max_size) { const u8 *p; - if (max_size < sizeof(ACL)) + if (max_size < sizeof(wimlib_ACL)) return; u8 revision = acl->revision; @@ -354,11 +232,11 @@ print_acl(const ACL *acl, const tchar *type, size_t max_size) tprintf(T(" ACL Size = %u\n"), acl_size); tprintf(T(" ACE Count = %u\n"), ace_count); - p = (const u8*)acl + sizeof(ACL); + p = (const u8*)acl + sizeof(wimlib_ACL); for (u16 i = 0; i < ace_count; i++) { - if (max_size < p + sizeof(ACCESS_ALLOWED_ACE) - (const u8*)acl) + if (max_size < p + sizeof(wimlib_ACCESS_ALLOWED_ACE) - (const u8*)acl) break; - const ACCESS_ALLOWED_ACE *aaa = (const ACCESS_ALLOWED_ACE*)p; + const wimlib_ACCESS_ALLOWED_ACE *aaa = (const wimlib_ACCESS_ALLOWED_ACE*)p; tprintf(T(" [ACE]\n")); tprintf(T(" ACE type = %d\n"), aaa->hdr.type); tprintf(T(" ACE flags = 0x%x\n"), aaa->hdr.flags); @@ -371,9 +249,9 @@ print_acl(const ACL *acl, const tchar *type, size_t max_size) } static void -print_sid(const SID *sid, const tchar *type, size_t max_size) +print_sid(const wimlib_SID *sid, const tchar *type, size_t max_size) { - if (max_size < sizeof(SID)) + if (max_size < sizeof(wimlib_SID)) return; tprintf(T(" [%"TS" SID]\n"), type); @@ -383,7 +261,7 @@ print_sid(const SID *sid, const tchar *type, size_t max_size) print_byte_field(sid->identifier_authority, sizeof(sid->identifier_authority), stdout); tputchar(T('\n')); - if (max_size < sizeof(SID) + (size_t)sid->sub_authority_count * sizeof(u32)) + if (max_size < sizeof(wimlib_SID) + (size_t)sid->sub_authority_count * sizeof(u32)) return; for (u8 i = 0; i < sid->sub_authority_count; i++) { tprintf(T(" Subauthority %u = %u\n"), @@ -393,11 +271,11 @@ print_sid(const SID *sid, const tchar *type, size_t max_size) } static void -print_security_descriptor(const SECURITY_DESCRIPTOR_RELATIVE *descr, +print_security_descriptor(const wimlib_SECURITY_DESCRIPTOR_RELATIVE *descr, size_t size) { u8 revision = descr->revision; - u16 control = le16_to_cpu(descr->security_descriptor_control); + u16 control = le16_to_cpu(descr->control); u32 owner_offset = le32_to_cpu(descr->owner_offset); u32 group_offset = le32_to_cpu(descr->group_offset); u32 dacl_offset = le32_to_cpu(descr->dacl_offset); @@ -411,19 +289,19 @@ print_security_descriptor(const SECURITY_DESCRIPTOR_RELATIVE *descr, tprintf(T("System ACL offset = %u\n"), sacl_offset); if (owner_offset != 0 && owner_offset <= size) - print_sid((const SID*)((const u8*)descr + owner_offset), + print_sid((const wimlib_SID*)((const u8*)descr + owner_offset), T("Owner"), size - owner_offset); if (group_offset != 0 && group_offset <= size) - print_sid((const SID*)((const u8*)descr + group_offset), + print_sid((const wimlib_SID*)((const u8*)descr + group_offset), T("Group"), size - group_offset); if (dacl_offset != 0 && dacl_offset <= size) - print_acl((const ACL*)((const u8*)descr + dacl_offset), + print_acl((const wimlib_ACL*)((const u8*)descr + dacl_offset), T("Discretionary"), size - dacl_offset); if (sacl_offset != 0 && sacl_offset <= size) - print_acl((const ACL*)((const u8*)descr + sacl_offset), + print_acl((const wimlib_ACL*)((const u8*)descr + sacl_offset), T("System"), size - sacl_offset); } @@ -440,7 +318,7 @@ print_wim_security_data(const struct wim_security_data *sd) for (u32 i = 0; i < sd->num_entries; i++) { tprintf(T("[SECURITY_DESCRIPTOR_RELATIVE %"PRIu32", length = %"PRIu64"]\n"), i, sd->sizes[i]); - print_security_descriptor((const SECURITY_DESCRIPTOR_RELATIVE*)sd->descriptors[i], + print_security_descriptor((const wimlib_SECURITY_DESCRIPTOR_RELATIVE*)sd->descriptors[i], sd->sizes[i]); tputchar(T('\n')); } diff --git a/tests/wims/cyclic.wim b/tests/wims/cyclic.wim new file mode 100644 index 00000000..b6f5420e Binary files /dev/null and b/tests/wims/cyclic.wim differ diff --git a/tests/wims/empty_dacl.wim b/tests/wims/empty_dacl.wim new file mode 100644 index 00000000..89911ec9 Binary files /dev/null and b/tests/wims/empty_dacl.wim differ diff --git a/tests/wims/longpaths.wim b/tests/wims/longpaths.wim new file mode 100644 index 00000000..491c9106 Binary files /dev/null and b/tests/wims/longpaths.wim differ