Skip to content

(PUP-4684) Ensure Windows mode of 7 is FullControl#6859

Merged
RandomNoun7 merged 1 commit intopuppetlabs:5.5.xfrom
Iristyle:PUP-4684-change-ACL-group-synthesis
Jun 4, 2018
Merged

(PUP-4684) Ensure Windows mode of 7 is FullControl#6859
RandomNoun7 merged 1 commit intopuppetlabs:5.5.xfrom
Iristyle:PUP-4684-change-ACL-group-synthesis

Conversation

@Iristyle
Copy link
Copy Markdown
Contributor

@Iristyle Iristyle commented May 29, 2018

  • Previously, the group and other (Everyone) permissions are setup
    so that the ability to modify the DACL is restricted. A member of
    a group cannot take ownership of a file / directory, cannot write
    to the DACL or delete the DACL.

    This is by design to prevent a scenario where a group is given
    read-only access to a file, so that a user in that group may not
    rewrite a DACL, or take ownership, in a way that escalates
    privilege to this file.

    Maintain the same previous behavior, except when the mode is
    7, which should be equivalent to (F) / FullControl within the DACL,
    allowing the same behavior that is presently exhibited for a file
    owner.

  • Note that the previous behavior of removing FILE_EXECUTE for files
    written to a directory (86f2876)
    is maintained for the CREATOR OWNER:(OI) and CREATOR GROUP:(OI) ACEs,
    but that perm calculation must additionally ensure that WDAC
    (Write DACL) and WO (Write Owner) permissions are stripped.

  • Note that the FILE_DELETE_CHILD bit is set on the relevant bitmasks
    even when the path is a file rather than a directory. Even though
    the bit is irrelevant to files, it prevents a file from having a
    status of (F) / FullControl in tools like icacls and Get-Acl,
    instead appearing as (M,WDAC,WO) which is much more difficult to
    understand, despite being nearly the same thing.

  • Given a manifest like the following:

   user { 'bob':
     ensure => present,
     managehome => false,
     password => 'bob1234!',
     groups => ['Users'],
   }
   user { 'foo':
     ensure => present,
     managehome => false,
     password => 'foo1234!',
     groups => ['Users'],
   }

   file { 'C:\foo':
     ensure => directory,
     owner => 'foo',
     group => 'Administrators',
     mode => '0770',
   }
   file { 'C:\foo\foo.bat':
     ensure => file,
     owner => 'bob',
     mode => '740',
     content => 'echo "hello"'
   }

Puppet would produce the following permissions:

   C:\>icacls \foo

   \foo VAGRANT-2008R2\foo:(F)
        BUILTIN\Administrators:(RX,W,DC)
        Everyone:(Rc,S,RA)
        NT AUTHORITY\SYSTEM:(OI)(CI)(F)
        CREATOR OWNER:(CI)(IO)(F)
        CREATOR GROUP:(CI)(IO)(RX,W,DC)
        CREATOR OWNER:(OI)(IO)(R,W,D,WDAC,WO,DC)
        CREATOR GROUP:(OI)(IO)(R,W,DC)

   Successfully processed 1 files; Failed processing 0 files

   C:\>icacls \foo\foo.bat

   \foo\foo.bat VAGRANT-2008R2\bob:(M,WDAC,WO)
                VAGRANT-2008R2\None:(R)
                Everyone:(Rc,S,RA)
                NT AUTHORITY\SYSTEM:(F)

   Successfully processed 1 files; Failed processing 0 files

Note specifically on the directory \foo that the permissions for
Administrators are (RX,W,DC) despite the fact that a 7 was
specified for the mode of the group, which should mean (F).
Similarly CREATOR GROUP:(CI) suffers from the same problem.

Also note \foo\foo.bat has specified the mode for owner 'bob' as 7
which should be (F), yet it is written as (M,WDAC,WO).

Lastly, note that CREATOR OWNER:(OI)(IO) permissions for files have
(WO) and (WDAC) removed, meaning that an owner cannot change the
owner of the file they created (typically unnecessary) and cannot
rewrite the DACL. CREATOR GROUP:(OI)(IO) has a (D) added, allowing
for the group of a file to delete it. Puppet typically sets the
group to NONE when it is not being managed.

With the fix in place, the observed permissions are set as desired.

   C:\>icacls \foo
   \foo VAGRANT-2008R2\foo:(F)
        BUILTIN\Administrators:(F)
        Everyone:(Rc,S,RA)
        NT AUTHORITY\SYSTEM:(OI)(CI)(F)
        CREATOR OWNER:(CI)(IO)(F)
        CREATOR GROUP:(CI)(IO)(F)
        CREATOR OWNER:(OI)(IO)(R,W,D,DC)
        CREATOR GROUP:(OI)(IO)(R,W,D,DC)

   Successfully processed 1 files; Failed processing 0 files

   C:\>icacls \foo\foo.bat
   \foo\foo.bat VAGRANT-2008R2\bob:(F)
                VAGRANT-2008R2\None:(R)
                Everyone:(Rc,S,RA)
                NT AUTHORITY\SYSTEM:(F)

   Successfully processed 1 files; Failed processing 0 files
  • Update tests to only set 777 mode on files for the sake of cleanup
    when possible. d78afda added this
    to help ensure temporary test directories can be cleaned up after
    tests have run. With this change, there are now some cases where
    the current user cannot write the DACL anymore, so ignore such
    errors. It turns out that in those failing cases, the file system
    can still be cleaned up OK, whether running tests as SYSTEM or
    as a local user in development.

Paired-with: Michael Lombardi michael.lombardi@puppet.com
Paired-with: Bill Hurt bill.hurt@puppet.com

@Iristyle Iristyle force-pushed the PUP-4684-change-ACL-group-synthesis branch from 2e273ab to 1ede1fd Compare May 30, 2018 00:23
@puppetcla
Copy link
Copy Markdown

CLA signed by all contributors.

group_ace = winsec.get_aces_for_path_by_sid(path, sids[:power_users])
# there should only be a single ace for the given group
expect(group_ace.count).to eq(1)
expect(group_ace[0].mask).to eq(klass::FILE_ALL_ACCESS)
Copy link
Copy Markdown
Contributor Author

@Iristyle Iristyle May 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails due to a missing FILE_DELETE_CHILD

@Iristyle Iristyle force-pushed the PUP-4684-change-ACL-group-synthesis branch 3 times, most recently from ab5836c to 205b9cb Compare May 30, 2018 23:22
@Iristyle
Copy link
Copy Markdown
Contributor Author

jenkins please test this

@Iristyle
Copy link
Copy Markdown
Contributor Author

jenkins please test this on windows2012r2-64a,windows10ent-32a,windows2012r2_ja-64a%7Blocale=ja%7D,windows2016-64a,windows10ent-64a

@Iristyle Iristyle force-pushed the PUP-4684-change-ACL-group-synthesis branch 2 times, most recently from a719521 to a517b39 Compare May 31, 2018 18:20
@Iristyle
Copy link
Copy Markdown
Contributor Author

jenkins please test this on windows2012r2-64a,windows10ent-32a,windows2012r2_ja-64a%7Blocale=ja%7D,windows2016-64a,windows10ent-64a

@Iristyle Iristyle force-pushed the PUP-4684-change-ACL-group-synthesis branch 2 times, most recently from fe5bcd9 to 71a74cd Compare May 31, 2018 21:44
@Iristyle
Copy link
Copy Markdown
Contributor Author

jenkins please test this on windows2012r2-64a,windows10ent-32a,windows2012r2_ja-64a%7Blocale=ja%7D,windows2016-64a,windows10ent-64a

@Iristyle
Copy link
Copy Markdown
Contributor Author

Iristyle commented Jun 1, 2018

@Iristyle
Copy link
Copy Markdown
Contributor Author

Iristyle commented Jun 4, 2018

With the latest PR / builds against 71a74cd, the diff of perms is https://gist.github.com/Iristyle/9252f4dc5b13ee0513a6339968d00cc4

@Iristyle Iristyle force-pushed the PUP-4684-change-ACL-group-synthesis branch from 71a74cd to 67dbab5 Compare June 4, 2018 21:40
 - Previously, the group and other (Everyone) permissions are setup
   so that the ability to modify the DACL is restricted. A member of
   a group cannot take ownership of a file / directory, cannot write
   to the DACL or delete the DACL.

   This is by design to prevent a scenario where a group is given
   read-only access to a file, so that a user in that group may not
   rewrite a DACL, or take ownership, in a way that escalates
   privilege to this file.

   Maintain the same previous behavior, *except* when the mode is
   7, which should be equivalent to (F) / FullControl within the DACL,
   allowing the same behavior that is presently exhibited for a file
   owner.

 - Note that the previous behavior of removing FILE_EXECUTE for files
   written to a directory (86f2876)
   is maintained for the CREATOR OWNER:(OI) and CREATOR GROUP:(OI) ACEs,
   but that perm calculation must additionally ensure that WDAC
   (Write DACL) and WO (Write Owner) permissions are stripped.

 - Note that the FILE_DELETE_CHILD bit is set on the relevant bitmasks
   even when the path is a file rather than a directory. Even though
   the bit is irrelevant to files, it prevents a file from having a
   status of (F) / FullControl in tools like icacls and Get-Acl,
   instead appearing as (M,WDAC,WO) which is much more difficult to
   understand, despite being nearly the same thing.

 - Given a manifest like the following:

   user { 'bob':
     ensure => present,
     managehome => false,
     password => 'bob1234!',
     groups => ['Users'],
   }
   user { 'foo':
     ensure => present,
     managehome => false,
     password => 'foo1234!',
     groups => ['Users'],
   }

   file { 'C:\foo':
     ensure => directory,
     owner => 'foo',
     group => 'Administrators',
     mode => '0770',
   }
   file { 'C:\foo\foo.bat':
     ensure => file,
     owner => 'bob',
     mode => '740',
     content => 'echo "hello"'
   }

   Puppet would produce the following permissions:

   C:\>icacls \foo

   \foo VAGRANT-2008R2\foo:(F)
        BUILTIN\Administrators:(RX,W,DC)
        Everyone:(Rc,S,RA)
        NT AUTHORITY\SYSTEM:(OI)(CI)(F)
        CREATOR OWNER:(CI)(IO)(F)
        CREATOR GROUP:(CI)(IO)(RX,W,DC)
        CREATOR OWNER:(OI)(IO)(R,W,D,WDAC,WO,DC)
        CREATOR GROUP:(OI)(IO)(R,W,DC)

   Successfully processed 1 files; Failed processing 0 files

   C:\>icacls \foo\foo.bat

   \foo\foo.bat VAGRANT-2008R2\bob:(M,WDAC,WO)
                VAGRANT-2008R2\None:(R)
                Everyone:(Rc,S,RA)
                NT AUTHORITY\SYSTEM:(F)

   Successfully processed 1 files; Failed processing 0 files

   Note specifically on the directory \foo that the permissions for
   Administrators are (RX,W,DC) despite the fact that a 7 was
   specified for the mode of the group, which should mean (F).
   Similarly CREATOR GROUP:(CI) suffers from the same problem.

   Also note \foo\foo.bat has specified the mode for owner 'bob' as 7
   which should be (F), yet it is written as (M,WDAC,WO).

   Lastly, note that CREATOR OWNER:(OI)(IO) permissions for files have
   (WO) and (WDAC) removed, meaning that an owner cannot change the
   owner of the file they created (typically unnecessary) and cannot
   rewrite the DACL. CREATOR GROUP:(OI)(IO) has a (D) added, allowing
   for the group of a file to delete it. Puppet typically sets the
   group to NONE when it is not being managed.

   With the fix in place, the observed permissions are set as desired.

   C:\>icacls \foo
   \foo VAGRANT-2008R2\foo:(F)
        BUILTIN\Administrators:(F)
        Everyone:(Rc,S,RA)
        NT AUTHORITY\SYSTEM:(OI)(CI)(F)
        CREATOR OWNER:(CI)(IO)(F)
        CREATOR GROUP:(CI)(IO)(F)
        CREATOR OWNER:(OI)(IO)(R,W,D,DC)
        CREATOR GROUP:(OI)(IO)(R,W,D,DC)

   Successfully processed 1 files; Failed processing 0 files

   C:\>icacls \foo\foo.bat
   \foo\foo.bat VAGRANT-2008R2\bob:(F)
                VAGRANT-2008R2\None:(R)
                Everyone:(Rc,S,RA)
                NT AUTHORITY\SYSTEM:(F)

   Successfully processed 1 files; Failed processing 0 files

 - Update tests to only set 777 mode on files for the sake of cleanup
   when possible. d78afda added this
   to help ensure temporary test directories can be cleaned up after
   tests have run. With this change, there are now some cases where
   the current user cannot write the DACL anymore, so ignore such
   errors. It turns out that in those failing cases, the file system
   can still be cleaned up OK, whether running tests as SYSTEM *or*
   as a local user in development.

Paired-with: Michael Lombardi <michael.lombardi@puppet.com>
Paired-with: Bill Hurt <bill.hurt@puppet.com>
@Iristyle Iristyle force-pushed the PUP-4684-change-ACL-group-synthesis branch from 67dbab5 to 14f429a Compare June 4, 2018 23:06
@RandomNoun7 RandomNoun7 merged commit 67a809a into puppetlabs:5.5.x Jun 4, 2018
@Iristyle Iristyle deleted the PUP-4684-change-ACL-group-synthesis branch June 4, 2018 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants