Re: [ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-10-29 Thread Eric Biggers 
On Sat, Oct 29, 2016 at 11:21:37AM +0200, Jean-Pierre André wrote:
> Hi again,
> 
> Eric Biggers wrote:
> > Hi Jean-Pierre,
> > 
> > Sorry for the late response.
> 
> No problem. I also did not do much about it.
> 
> The intent of ntfs_valid_descr() was to guard against
> processing security descriptors with invalid or unknown
> features, but your need is to check whether a descriptor
> is valid for Windows. The purpose of ntfs-3g is to map
> Unix concepts to an ntfs file system, which is somehow
> different from emulating the Windows behavior (a moving
> target, Windows 8 and Windows 10 brought significant
> changes).
> 
> The translations of Windows ACLs to Posix ones rely on
> heuristics which will be defeated if the ACEs are not
> as expected.
> 
> Maybe having two variants of ntfs_valid_descr() would
> be the way to go, as you do not need translations,
> inheritance, etc.
> 
> Jean-Pierre

Maybe.  I suppose that would mean callers would be updated to specify whether
they want the stricter validation or not, and ntfs_get_ntfs_acl() and
ntfs_set_ntfs_acl() wouldn't require the stricter validation?  Would the
stricter validation apply to the SACL as well as the DACL?  It seems that it
shouldn't, i.e. having entries in the SACL, such as system audit entries or
integrity labels or whatever, shouldn't prevent NTFS-3G from attempting to map
the DACL to UNIX permissions.

Eric

--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
ntfs-3g-devel mailing list
ntfs-3g-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel

Re: [ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-10-29 Thread Jean-Pierre André 
Hi again,

Eric Biggers wrote:
> Hi Jean-Pierre,
>
> Sorry for the late response.

No problem. I also did not do much about it.

The intent of ntfs_valid_descr() was to guard against
processing security descriptors with invalid or unknown
features, but your need is to check whether a descriptor
is valid for Windows. The purpose of ntfs-3g is to map
Unix concepts to an ntfs file system, which is somehow
different from emulating the Windows behavior (a moving
target, Windows 8 and Windows 10 brought significant
changes).

The translations of Windows ACLs to Posix ones rely on
heuristics which will be defeated if the ACEs are not
as expected.

Maybe having two variants of ntfs_valid_descr() would
be the way to go, as you do not need translations,
inheritance, etc.

Jean-Pierre

>
> On Mon, Sep 26, 2016 at 01:52:36PM +0200, Jean-Pierre André wrote:
>>> 1. "Object" ACEs are mentioned as only being used for Active Directory 
>>> objects
>>>  [source: Windows Internals 6th edition].  On Windows, trying to use
>>>  SetFileSecurity() to set an object ACE in the DACL of an NTFS 
>>> directory fails
>>>  with ERROR_INVALID_ACL.  This is different from how Windows treats 
>>> truly
>>>  unknown ACE types (see below).  But I think it would be fine for 
>>> NTFS-3G to
>>>  simplify things by treating object ACEs like any other unknown ACE 
>>> type.
>>
>> Just for me to understand : Unix stores special objects
>> (such as pipes, sockets, etc.) as file system objects with
>> access control attached. Does Windows also records special
>> objects as file system objects ? If so, how are these
>> objects distinguished from files and directories ?
>
> Active Directory objects are stored in a database, not directly on the
> filesystem.  They just happen to both use the security descriptor format.
>
>> The purpose of ntfs_valid_descr() is to reject descriptors
>> which ntfs-3g cannot process properly. I am not keen on
>> letting special ACEs leaking into translations to Linux.
>>
>> Inheritance is of particular concern, because this is rather
>> complex and there are undocumented special cases which have
>> to be found by trials and errors. Moreover the result is
>> generally not satisfactory for Linux users who have
>> expectations different from Windows ones (typical example :
>> inheritance of execute permissions).
>>
>> Also : if objects can inherit special ACEs from their parent
>> directory, what prevents them to be inherited to plain files
>> created in the same directory ?
>>
>> Cannot these specific needs be implemented within wimlib ?
>
> I am not sure what you mean regarding the purpose of ntfs_valid_descr(), but 
> the
> existing behavior is that it allows unknown ACEs in both the DACL and SACL
> except in certain cases.  And in those certain cases, such as a callback ACE
> that is not the last ACE in the list, it is broken for backup/restore
> applications like wimlib which expect to be able to set security descriptors
> that were created on Windows.
>
> Note that as a backup/restore application, wimlib does not want ACE 
> inheritance
> at all, nor does it want translation of Windows ACLs to POSIX or Linux
> permissions.  It simply wants to stamp a security descriptor on each file or
> directory.  It can be assumed that the security descriptors are valid in the
> sense that Windows would accept them.  As part of this, it can be assumed that
> the ACEs in each ACL have the common ACE_HEADER.  But it can *not* be assumed
> that every ACE is of a known type.  The expected behavior is that unknown ACEs
> are settable but are then skipped during access evalution.  This would match 
> the
> Windows behavior.  I think Windows users might actually find it quite annoying
> for Windows to do otherwise because then people could not, for example, 
> restore
> security descriptors intended for a later version of Windows from a live 
> system
> running an older version of Windows.  Essentially the same argument applies to
> NTFS-3G.
>
> With regards to inheritance, as I pointed out Windows simply performs the
> standard inheritance algorithm on unknown ACEs per the standard ACE_HEADER
> flags.  It's true that there could be special rules that it is missing, but it
> seems like the most logical behavior and probably the easiest to implement 
> too.
> Of course, this isn't currently relevant to wimlib which as I mentioned does 
> not
> care about inheritance.  I just thought I'd suggest it as an improvement 
> (though
> it's still not clear to me what the current behavior is "supposed" to be, and
> that's part of the problem).
>
> Eric
>



--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
n

Re: [ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-10-28 Thread Eric Biggers 
Hi Jean-Pierre,

Sorry for the late response.

On Mon, Sep 26, 2016 at 01:52:36PM +0200, Jean-Pierre André wrote:
> > 1. "Object" ACEs are mentioned as only being used for Active Directory 
> > objects
> > [source: Windows Internals 6th edition].  On Windows, trying to use
> > SetFileSecurity() to set an object ACE in the DACL of an NTFS directory 
> > fails
> > with ERROR_INVALID_ACL.  This is different from how Windows treats truly
> > unknown ACE types (see below).  But I think it would be fine for 
> > NTFS-3G to
> > simplify things by treating object ACEs like any other unknown ACE type.
> 
> Just for me to understand : Unix stores special objects
> (such as pipes, sockets, etc.) as file system objects with
> access control attached. Does Windows also records special
> objects as file system objects ? If so, how are these
> objects distinguished from files and directories ?

Active Directory objects are stored in a database, not directly on the
filesystem.  They just happen to both use the security descriptor format.

> The purpose of ntfs_valid_descr() is to reject descriptors
> which ntfs-3g cannot process properly. I am not keen on
> letting special ACEs leaking into translations to Linux.
> 
> Inheritance is of particular concern, because this is rather
> complex and there are undocumented special cases which have
> to be found by trials and errors. Moreover the result is
> generally not satisfactory for Linux users who have
> expectations different from Windows ones (typical example :
> inheritance of execute permissions).
> 
> Also : if objects can inherit special ACEs from their parent
> directory, what prevents them to be inherited to plain files
> created in the same directory ?
> 
> Cannot these specific needs be implemented within wimlib ?

I am not sure what you mean regarding the purpose of ntfs_valid_descr(), but the
existing behavior is that it allows unknown ACEs in both the DACL and SACL
except in certain cases.  And in those certain cases, such as a callback ACE
that is not the last ACE in the list, it is broken for backup/restore
applications like wimlib which expect to be able to set security descriptors
that were created on Windows.

Note that as a backup/restore application, wimlib does not want ACE inheritance
at all, nor does it want translation of Windows ACLs to POSIX or Linux
permissions.  It simply wants to stamp a security descriptor on each file or
directory.  It can be assumed that the security descriptors are valid in the
sense that Windows would accept them.  As part of this, it can be assumed that
the ACEs in each ACL have the common ACE_HEADER.  But it can *not* be assumed
that every ACE is of a known type.  The expected behavior is that unknown ACEs
are settable but are then skipped during access evalution.  This would match the
Windows behavior.  I think Windows users might actually find it quite annoying
for Windows to do otherwise because then people could not, for example, restore
security descriptors intended for a later version of Windows from a live system
running an older version of Windows.  Essentially the same argument applies to
NTFS-3G.

With regards to inheritance, as I pointed out Windows simply performs the
standard inheritance algorithm on unknown ACEs per the standard ACE_HEADER
flags.  It's true that there could be special rules that it is missing, but it
seems like the most logical behavior and probably the easiest to implement too.
Of course, this isn't currently relevant to wimlib which as I mentioned does not
care about inheritance.  I just thought I'd suggest it as an improvement (though
it's still not clear to me what the current behavior is "supposed" to be, and
that's part of the problem).

Eric

--
The Command Line: Reinvented for Modern Developers
Did the resurgence of CLI tooling catch you by surprise?
Reconnect with the command line and become more productive. 
Learn the new .NET and ASP.NET CLI. Get your free copy!
http://sdm.link/telerik
___
ntfs-3g-devel mailing list
ntfs-3g-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel

Re: [ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-09-26 Thread Jean-Pierre André 
Hi Eric,

Eric Biggers wrote:
> On Wed, Sep 21, 2016 at 12:01:23PM +0200, Jean-Pierre André wrote:
>>
>> I have never met an object ACE and they might be irrelevant
>> for a file system which only deals with files and directories.
>>
>> Is there a point in ntfs-3g accepting ACE types controlling
>> entities which are not emulated on Linux (callbacks, labels,
>> policies, etc.) ?
>
> Yes, because it should be --- and already is, except in certain cases ---
> possible to use NTFS-3G to restore security descriptors that were created 
> under
> Windows.  This can be done by using wimlib to extract a WIM image to a NTFS
> volume (for example).
>
> I think the emulation of ACEs under Linux is a separate concern which for some
> of the new ACE types isn't really possible or meaningful.
>
> I also did some research, and some experiments on Windows:
>
> 1. "Object" ACEs are mentioned as only being used for Active Directory objects
> [source: Windows Internals 6th edition].  On Windows, trying to use
> SetFileSecurity() to set an object ACE in the DACL of an NTFS directory 
> fails
> with ERROR_INVALID_ACL.  This is different from how Windows treats truly
> unknown ACE types (see below).  But I think it would be fine for NTFS-3G 
> to
> simplify things by treating object ACEs like any other unknown ACE type.

Just for me to understand : Unix stores special objects
(such as pipes, sockets, etc.) as file system objects with
access control attached. Does Windows also records special
objects as file system objects ? If so, how are these
objects distinguished from files and directories ?

> 2. "Callback" ACEs, also known as "conditional" ACEs, are mentioned as only
> existing for use by the AuthZ API, which is a userspace API for access
> control.  The Windows kernel does *not* evaluate such ACEs when performing
> access checks [source: Windows Internals 6th edition].  However, I *was* 
> able
> to set such an ACE in the DACL of an NTFS directory using 
> SetFileSecurity().
> In addition, on Windows such an ACE is inherited per the standard ACE 
> header
> flags, and the generic rights and SID mapping is performed.  Still, I 
> don't
> yet know exactly *why* recent Windows 10 builds have been observed to use
> such ACEs.
>
> 3. Truly unknown ACE types are accepted by SetFileSecurity().  They also are
> inherited per the standard ACE header flags.  However, they are not 
> evaluated
> during access checks.  In addition, SetFileSecurity() does no validation 
> of
> the ACCESS_MASK or SID fields of unknown ACEs --- which makes sense 
> because
> the format of such ACEs is actually unknown beyond the ACE_HEADER.  
> Instead,
> the ACE size field simply required to be at least sizeof(ACE_HEADER) and a
> multiple of 4.  No generic rights or SID mapping is performed during
> inheritance of unknown ACEs.
>
> So, given the requirements and these observations, I'd like to propose that
> NTFS-3G handle unknown ACE types as follows:
>
> * ntfs_valid_descr() accepts them and check the size only (like Windows)
> * ntfs_inherit_acl() performs inheritance on unknown ACE types per the ACE
>header flags but without the generic mapping (like Windows).  Optionally,
>generic rights and SID mapping can be done for callback ACEs.
> * NTFS-3G otherwise ignores unknown ACEs (like Windows)
>
> Any thoughts on this?

The purpose of ntfs_valid_descr() is to reject descriptors
which ntfs-3g cannot process properly. I am not keen on
letting special ACEs leaking into translations to Linux.

Inheritance is of particular concern, because this is rather
complex and there are undocumented special cases which have
to be found by trials and errors. Moreover the result is
generally not satisfactory for Linux users who have
expectations different from Windows ones (typical example :
inheritance of execute permissions).

Also : if objects can inherit special ACEs from their parent
directory, what prevents them to be inherited to plain files
created in the same directory ?

Cannot these specific needs be implemented within wimlib ?

Jean-Pierre



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

Re: [ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-09-25 Thread Eric Biggers 
On Wed, Sep 21, 2016 at 12:01:23PM +0200, Jean-Pierre André wrote:
> 
> I have never met an object ACE and they might be irrelevant
> for a file system which only deals with files and directories.
> 
> Is there a point in ntfs-3g accepting ACE types controlling
> entities which are not emulated on Linux (callbacks, labels,
> policies, etc.) ?

Yes, because it should be --- and already is, except in certain cases ---
possible to use NTFS-3G to restore security descriptors that were created under
Windows.  This can be done by using wimlib to extract a WIM image to a NTFS
volume (for example).

I think the emulation of ACEs under Linux is a separate concern which for some
of the new ACE types isn't really possible or meaningful.

I also did some research, and some experiments on Windows:

1. "Object" ACEs are mentioned as only being used for Active Directory objects
   [source: Windows Internals 6th edition].  On Windows, trying to use
   SetFileSecurity() to set an object ACE in the DACL of an NTFS directory fails
   with ERROR_INVALID_ACL.  This is different from how Windows treats truly
   unknown ACE types (see below).  But I think it would be fine for NTFS-3G to
   simplify things by treating object ACEs like any other unknown ACE type.

2. "Callback" ACEs, also known as "conditional" ACEs, are mentioned as only
   existing for use by the AuthZ API, which is a userspace API for access
   control.  The Windows kernel does *not* evaluate such ACEs when performing
   access checks [source: Windows Internals 6th edition].  However, I *was* able
   to set such an ACE in the DACL of an NTFS directory using SetFileSecurity().
   In addition, on Windows such an ACE is inherited per the standard ACE header
   flags, and the generic rights and SID mapping is performed.  Still, I don't
   yet know exactly *why* recent Windows 10 builds have been observed to use
   such ACEs.

3. Truly unknown ACE types are accepted by SetFileSecurity().  They also are
   inherited per the standard ACE header flags.  However, they are not evaluated
   during access checks.  In addition, SetFileSecurity() does no validation of
   the ACCESS_MASK or SID fields of unknown ACEs --- which makes sense because
   the format of such ACEs is actually unknown beyond the ACE_HEADER.  Instead,
   the ACE size field simply required to be at least sizeof(ACE_HEADER) and a
   multiple of 4.  No generic rights or SID mapping is performed during
   inheritance of unknown ACEs.

So, given the requirements and these observations, I'd like to propose that
NTFS-3G handle unknown ACE types as follows:

* ntfs_valid_descr() accepts them and check the size only (like Windows)
* ntfs_inherit_acl() performs inheritance on unknown ACE types per the ACE
  header flags but without the generic mapping (like Windows).  Optionally,
  generic rights and SID mapping can be done for callback ACEs.
* NTFS-3G otherwise ignores unknown ACEs (like Windows)

Any thoughts on this?

Eric

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

Re: [ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-09-21 Thread Jean-Pierre André 
Hi again,

Eric Biggers wrote:
> Hmm, I think I messed up the "object" ACEs slightly (assuming they ever 
> actually
> exist).  According to
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa374857(v=vs.85).aspx,
> the SID is actually at a variable offset.
>
> Another problem is that NTFS-3G interprets ACLs in other places, so if there
> really are ACEs with a new format, they would need to be handled there too...

I have never met an object ACE and they might be irrelevant
for a file system which only deals with files and directories.

Is there a point in ntfs-3g accepting ACE types controlling
entities which are not emulated on Linux (callbacks, labels,
policies, etc.) ?

Regards

Jean-Pierre

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

Re: [ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-09-21 Thread Eric Biggers 
Hmm, I think I messed up the "object" ACEs slightly (assuming they ever actually
exist).  According to
https://msdn.microsoft.com/en-us/library/windows/desktop/aa374857(v=vs.85).aspx,
the SID is actually at a variable offset.

Another problem is that NTFS-3G interprets ACLs in other places, so if there
really are ACEs with a new format, they would need to be handled there too...

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

[ntfs-3g-devel] [PATCH 2/2] ACE validation fixes

2016-09-21 Thread Eric Biggers 
- 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 
---
 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