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

Windows: Restore ACLs from original file to backup #52987

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@dagwieers
Copy link
Member

dagwieers commented Feb 26, 2019

SUMMARY

This code comes from win_blockinfile and seems worthwhile to retain in our shared Backup-File functionality.

4102047

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

windows backup

Windows: Restore ACLs from original file to backup
This code comes from win_blockinfile and seems worthwhile to retain in
our shared Backup-File functionality.
@dagwieers

This comment has been minimized.

Copy link
Member Author

dagwieers commented Feb 26, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 26, 2019

@ansibot ansibot added core_review and removed needs_revision labels Feb 26, 2019

@samdoran samdoran added P3 and removed needs_triage labels Feb 26, 2019

@ansibot ansibot added the stale_ci label Mar 6, 2019

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Mar 10, 2019

I've been playing around with this one a bit more and have a few questions/points. I thought it best to briefly give some background into the Windows security descriptors for objects.

A security descriptor (SD) is comprised of 4 parts, a very basic description of each part are;

  • Owner - The owner of the file, an owner has Full Control over the object even if they don't have an explicit DACL entry saying so
  • Group - Largely irrelevant and unused for Windows
  • DACL - A list of ACEs that govern the access control for the object, this is what the win_acl module controls and what people typically see when it comes to access control in Windows
  • SACL - A list of ACEs that govern the audit rules of an object. Defines what users and access types should be audited in the event logs

The Get-Acl cmdlet will automatically retrieve the owner, group, and dacl which means we are only preserving those parts of the SD of the backup file. The SACL is a special one because it requires the SeSecurityPrivilege to be able to read and write most parts of the SACL of an object. This privilege should only be assigned to administrators which means that typically only admins can manipulate the SACL. This leads to the first question, should we be trying to preserve the SACL. My gut is no but thought it best to ask that question anyway.

The second problem is around the Owner part of the SD. In Windows you can only set the owner of an object to either the current access token's user SID, or any groups in that token that has the SE_GROUP_OWNER attribute applied. This means that for standard user's it can only be set to the current user, or the Administrators group if the user is an admin. Windows does provide a way to bypass this with the SeRestorePrivilege which allows you to set the owner to any SID. Unfortunately this is a powerful privilege that only administrators should have. Effectively this means that if the module is run as a standard user account and the user is not already the owner of the source file then Backup-File will fail.

While running a module as a non-admin is probably not the mot common scenario today, we probably don't want to be hamstrung too much by this fact. I'm wondering whether the failure to set the the SD should be an error or whether we should be creating a warning. If it's the latter we should be breaking apart each group of the SD when we go to copy it so that a failure on one would not necessarily stop the other aspects from being copied.

The last point is unrelated to the SD, when we get a failure in the Get-Acl/Set-Acl call, we currently just throw an exception, we should probably remove the backup file that was created on a failure. If we were to treat the preserving of the SD as a warning and not a failure then disregard this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.