Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid lookups for rights of 'LocalSystem' in windows service #7083

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

btm
Copy link
Contributor

@btm btm commented Mar 28, 2018

LocalSystem is a special account for the service subsystem, and the security
subsystem doesn't know about it. It inherits rights from BUILTIN\Administrators
so we don't need to check it for SeServiceLogonRight. Even if we look up System
it wouldn't show up as it gets that right from hidden membership in
BUILTIN\Administrators.

@btm btm requested a review from a team March 28, 2018 22:18
@btm
Copy link
Contributor Author

btm commented Mar 29, 2018

@stuartpreston NetworkService and LocalService aren't a problem:

chef (14.0.174)> Chef::ReservedNames::Win32::Security.get_account_right('LocalService')
 => ["SeAuditPrivilege", "SeChangeNotifyPrivilege", "SeImpersonatePrivilege", "SeCreateGlobalPrivilege", "SeIncreaseQuotaPrivilege", "SeAssignPrimaryTokenPrivilege", "SeSystemtimePrivilege", "SeTimeZonePrivilege"]
chef (14.0.174)> Chef::ReservedNames::Win32::Security.get_account_right('NetworkService')
 => ["SeAuditPrivilege", "SeChangeNotifyPrivilege", "SeImpersonatePrivilege", "SeCreateGlobalPrivilege", "SeIncreaseQuotaPrivilege", "SeAssignPrimaryTokenPrivilege", "SeServiceLogonRight"]

versus:

chef (14.0.174)> Chef::ReservedNames::Win32::Security.get_account_right('LocalSystem')
Traceback (most recent call last):
       11: from C:/opscode/chef/bin/chef-shell:59:in `<main>'
       10: from C:/opscode/chef/bin/chef-shell:59:in `load'
        9: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/bin/chef-shell:36:in `<top (required)>'
        8: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/lib/chef/shell.rb:80:in `start'
        7: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/lib/chef/shell.rb:80:in `catch'
        6: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/lib/chef/shell.rb:81:in `block in start'
        5: from (irb):3
        4: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/lib/chef/win32/security.rb:186:in `get_account_right'
        3: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/lib/chef/win32/security.rb:602:in `with_lsa_policy'
        2: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/lib/chef/win32/security.rb:395:in `lookup_account_name'
        1: from C:/opscode/chef/embedded/lib/ruby/gems/2.5.0/gems/chef-14.0.174-universal-mingw32/lib/chef/win32/error.rb:81:in `raise!'
Chef::Exceptions::Win32APIError (No mapping between account names and security IDs was done.
---- Begin Win32 API output ----
System Error Code: 1332
System Error Message: No mapping between account names and security IDs was done.
---- End Win32 API output ----
)

@stuartpreston
Copy link

LocalService doesn’t have the right though so presumably would blow up further down where try and add the right to the account? It would be good if there was a class for these type of accounts so we could treat them all the same way consistently (i.e don’t attempt to add an NT right to these accounts)

@btm
Copy link
Contributor Author

btm commented Mar 29, 2018

The right adds fine with LocalService. My concern is with making too many assumptions about what someone might be doing. Like we if made get_account_right and add_account_right themselves skip some accounts, we might break someone else. As far as I cant tell, LocalSystem is the real outlier here.

chef (14.0.174)> Chef::ReservedNames::Win32::Security.add_account_right('LocalService', "SeServiceLogonRight")
 => nil
chef (14.0.174)> Chef::ReservedNames::Win32::Security.get_account_right('LocalService')
 => ["SeAuditPrivilege", "SeChangeNotifyPrivilege", "SeImpersonatePrivilege", "SeCreateGlobalPrivilege", "SeIncreaseQuotaPrivilege", "SeAssignPrimaryTokenPrivilege", "SeSystemtimePrivilege", "SeTimeZonePrivilege", "SeServiceLogonRight"]

@stuartpreston
Copy link

So TIL, I had assumed that all three of these special accounts would have the same behavior when assigning tokens but clearly not. I agree with you about not making too many assumptions. Thanks @btm

LocalSystem is a special account for the service subsystem, and the security
subsystem doesn't know about it. It inherits rights from BUILTIN\Administrators
so we don't need to check it for SeServiceLogonRight. Even if we look up System
it wouldn't show up as it gets that right from hidden membership in
BUILTIN\Administrators.

Signed-off-by: Bryan McLellan <btm@loftninjas.org>
@stuartpreston
Copy link

+1 on this, fine for merge

@tas50 tas50 merged commit 5522c96 into master Mar 29, 2018
@tas50 tas50 deleted the btm/fix-windows-service branch March 29, 2018 21:02
@lock
Copy link

lock bot commented May 28, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants