[PUP-9068] Windows admin? check should consider group membership Created: 2018/08/15  Updated: 2018/10/29  Resolved: 2018/10/03

Status: Closed
Project: Puppet
Component/s: Windows
Affects Version/s: PUP 5.5.3
Fix Version/s: PUP 5.5.z, PUP 6.0.3

Type: Improvement Priority: Normal
Reporter: Ethan Brown Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: ntfs, security, windows
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to PA-2019 Privilege escalation via %ProgramData... Closed
relates to PUP-9106 Windows file system ACLs should alway... Closed
relates to PUP-6729 NTFS permissions should be recalculat... Closed
relates to PUP-8939 Administrators are not able to run pu... Closed
Template:
Acceptance Criteria:
  • A user with elevated / admin tokens, that is not a member of the Administrators group, should write data to their home directory
Epic Link: Windows NTFS and Permissions Improvements
Team: Windows
Story Points: 1
Sprint: Windows 2018-10-3
QA Risk Assessment: Needs Assessment

 Description   

In PA-2019, the installer was changed to lay down permissions differently so that ProgramData generally has Administrators: (F) and SYSTEM: (F) set recursively.

It's possible to create an "administrative" user based on their token privileges, but without actually making them part of the Administrators group. The check inside Puppet at for elevated_security? at https://github.com/puppetlabs/puppet/blob/e7839794a1d7d393e6716927764c1276494123c2/lib/puppet/util/windows/process.rb#L183-L205 will then pass, despite the user not being in Administrators.

If such a user is assigned to the Puppet service, then pandemonium ensues, given how permissions are set on ProgramData\PuppetLabs.

The admin? check should be altered to ensure the user is part of Administrators or not. This determines where data can be written for that user.



 Comments   
Comment by Glenn Sarti [ 2018/08/21 ]

Ethan Brown Can you add the instructions on how to create an admin user without admin group. This seems very edgecase-y

Comment by Ethan Brown [ 2018/09/06 ]

There's a nice PowerShell module that does all the heavy lifting with privilege assignment at https://gallery.technet.microsoft.com/scriptcenter/Grant-Revoke-Query-user-26e259b0

Through the process of elimination, I was able to determine the single token privilege necessary to "trick" our code - namely SeImpersonatePrivilege

# create the user
net user testadmin Admin123 /add
# grant the impersonation privilege
Grant-UserRight -Account testadmin -Right SeImpersonatePrivilege
 
# verify user rights - should return only the SeImpersonatePrivilege
Get-UserRightsGrantedToAccount testadmin
 
# use psexec to launch a cmd process and navigate to a directory with Puppet installed, for instance C:\source\puppetlabs-scheduled_task>
# run ruby and show elevated is on
bundle exec ruby -e "require 'puppet'; puts Puppet::Util::Windows::Process.elevated_security?"
# true

For reference, the account should also display something similar to the following for whoami /all

USER INFORMATION
----------------
 
User Name                SID
======================== ============================================
vagrant-2008r2\testadmin S-1-5-21-271343509-1886877197-423808128-4919
 
 
GROUP INFORMATION
-----------------
 
Group Name                                           Type             SID                                          Attributes
==================================================== ================ ============================================ ==================================================
Everyone                                             Well-known group S-1-1-0                                      Mandatory group, Enabled by default, Enabled group
VAGRANT-2008R2\g45991a14-ee2d-48f6-925c-6ea809a5f994 Alias            S-1-5-21-271343509-1886877197-423808128-4735 Mandatory group, Enabled by default, Enabled group
VAGRANT-2008R2\TestGroup-PUP8231                     Alias            S-1-5-21-271343509-1886877197-423808128-4699 Mandatory group, Enabled by default, Enabled group
BUILTIN\Users                                        Alias            S-1-5-32-545                                 Mandatory group, Enabled by default, Enabled group
NT AUTHORITY\INTERACTIVE                             Well-known group S-1-5-4                                      Mandatory group, Enabled by default, Enabled group
CONSOLE LOGON                                        Well-known group S-1-2-1                                      Mandatory group, Enabled by default, Enabled group
NT AUTHORITY\Authenticated Users                     Well-known group S-1-5-11                                     Mandatory group, Enabled by default, Enabled group
NT AUTHORITY\This Organization                       Well-known group S-1-5-15                                     Mandatory group, Enabled by default, Enabled group
NT AUTHORITY\Local account                           Well-known group S-1-5-113                                    Mandatory group, Enabled by default, Enabled group
NT AUTHORITY\NTLM Authentication                     Well-known group S-1-5-64-10                                  Mandatory group, Enabled by default, Enabled group
Mandatory Label\High Mandatory Level                 Label            S-1-16-12288                                 Mandatory group, Enabled by default, Enabled group
 
 
PRIVILEGES INFORMATION
----------------------
 
Privilege Name                Description                               State
============================= ========================================= ========
SeChangeNotifyPrivilege       Bypass traverse checking                  Enabled
SeImpersonatePrivilege        Impersonate a client after authentication Enabled
SeIncreaseWorkingSetPrivilege Increase a process working set            Disabled
 
C:\Windows\system32>

For reference, with the privilege removed, the output of whoami /all is nearly identical except for the SeImpersonatePrivilege assignment above and the Medium Mandatory Level set instead of the High Mandatory Level above.

Mandatory Label\Medium Mandatory Level               Label            S-1-16-8192                                  Mandatory group, Enabled by default, Enabled group

There's a good tidbit about high integrity tokens not required to be in the Administrators group at https://stackoverflow.com/a/30970434
Another real-world example is at https://peter.hahndorf.eu/blog/elevate-nonadmin.html

The MSDN documentation for the integrity mechanism is at https://msdn.microsoft.com/en-us/library/bb625963.aspx

Comment by Kenn Hussey [ 2018/09/24 ]

Ethan Brown are you still planning to get this in for Puppet 6.0.1?

Comment by Keiran Haggerty [ 2018/09/25 ]

Erick Banks to pair with Glenn on this ticket.

Comment by Glenn Sarti [ 2018/10/01 ]

Ethan Brown So after pairing with Erick Banks this is probably not an issue.

The elevated_security? method you posted is only called by Puppet::Util::Windows::User.admin? which already does the elevated AND group membership checks.

This is then called by Puppet::Util::SUIDManager.root? to determine if a user is a root. I also verified the correct behaviour of Puppet::Util::Windows::User.admin? using the restricted token set you posted earlier i.e. it was always false.

Given this I don't think there's anything to do for this ticket. Closing as Won't Do.

EDIT - Whoops...the admin? method was somewhat correct. Keeping ticket open.

Comment by Glenn Sarti [ 2018/10/01 ]

Pull Request - Targetted against 5.x

https://github.com/puppetlabs/puppet/pull/7131

Comment by Michael Lombardi [ 2018/10/01 ]

Merged to 5.5.x at: https://github.com/puppetlabs/puppet/commit/abda86cec25e62e6a2fb80150294469e1031d3fa

Comment by Ethan Brown [ 2018/10/03 ]

https://github.com/puppetlabs/puppet/commits/abda86cec25e62e6a2fb80150294469e1031d3fa includes the merged commit

Last known good build of puppet-agent on 5.5.x:
Built at: 1 Oct 2018 22:09:08
PUPPET_AGENT_VERSION: 5.5.6.141.gdb6e53b
PUPPET_AGENT_COMMIT: db6e53b11c698e42a163be82f3f603ea6d122668
PUPPET_AGENT_SHORT_COMMIT: db6e53b11
FACTER_COMMIT: 8dd59ecdfbbf9a0c24f3257c960fb95feb241c9c
PUPPET_COMMIT: abda86cec25e62e6a2fb80150294469e1031d3fa
HIERA_COMMIT: 715ae4039e4cc7d248ccd9a1cf74c65d8b7f6226
PXPAGENT_COMMIT: b23ec4b6b114e766ce183fd311be047cd4dcf735

Generated at Wed Jun 26 23:33:44 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.