(PUP-1255) Don't be sensitive to pseudo-inherited ACEs #2211
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, the test would fail on Windows 2012, as the access control
entries in the Temp directory's DACL are missing the INHERITED_ACE bit:
> setacl -on temp -ot file -actn list -lst "f:tab;w:d,s,o,g;i:y"
temp
Owner: WIN-61G7FSKJ7JI\josh
Group: WIN-61G7FSKJ7JI\None
DACL(not_protected):
NT AUTHORITY\SYSTEM full allow container_inherit+object_inherit
BUILTIN\Administrators full allow container_inherit+object_inherit
WIN-61G7FSKJ7JI\josh full allow container_inherit+object_inherit
Note how none of the entries granting 'full' access are "inherited".
The test creates two empty temp files (the source is generated in the
call to Puppet::Util.replace_file) each with permissions:
WIN-61G7FSKJ7JI\josh
WIN-61G7FSKJ7JI\None
NT AUTHORITY\SYSTEM:(I) 0x1f01ff
BUILTIN\Administrators:(I) 0x1f01ff
WIN-61G7FSKJ7JI\josh:(I) 0x1f01ff
Each ACE is inherited and grants full access as expected. But when the Win32
API `ReplaceFile` is called, it prepends non-inherited ACEs to the destination
file:
WIN-61G7FSKJ7JI\josh
WIN-61G7FSKJ7JI\None
NT AUTHORITY\SYSTEM: 0x1f01ff
BUILTIN\Administrators: 0x1f01ff
WIN-61G7FSKJ7JI\josh: 0x1f01ff
NT AUTHORITY\SYSTEM:(I) 0x1f01ff
BUILTIN\Administrators:(I) 0x1f01ff
WIN-61G7FSKJ7JI\josh:(I) 0x1f01ff
While the merging behavior is unexpected, it doesn't alter the effective
security of the file.
This commit marks the directory as protected, which breaks inheritance
at that point. As a result, ReplaceFile behaves as expected.
Also note that if the Temp ACLs are reset recursively:
icacls Temp /reset /t
Then the merging behavior does not occur and the test passes (without
this commit).
See also pseudo-inherited ACEs:
http://helgeklein.com/setacl/faq/#what-are-pseudo-protected-acls-and-pseudo-inherited-aces-setacl-2-1
|
FYI the travis failure on 1.8.7 is the known issue with rubygems. I thought they had a fix forthcoming so didn't merge the workaround from puppet/master to puppet/stable. |
|
Evaluating this one. |
|
@kylog do you think we need to go ahead with backporting that fix? |
|
The travis fix is backported to stable. |
|
@kylog you sir are a scholar. |
|
CLA signed by all contributors. |
| @@ -58,6 +58,10 @@ | |||
| :if => Puppet.features.microsoft_windows? do | |||
|
|
|||
| dir = tmpdir('DACL_playground') | |||
| protected_sd = Puppet::Util::Windows::Security.get_security_descriptor(dir) | |||
| protected_sd.protect = true | |||
There was a problem hiding this comment.
Hah, so protect was the solution here... interesting.
There was a problem hiding this comment.
Yeah, it forces Windows to evaluate the missing "pseudo" inheritance items and then make them explicit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the test would fail on Windows 2012, as the access control
entries in the Temp directory's DACL are missing the INHERITED_ACE bit:
Note how none of the entries granting 'full' access are "inherited".
The test creates two empty temp files (the source is generated in the
call to Puppet::Util.replace_file) each with permissions:
Each ACE is inherited and grants full access as expected. But when the Win32
API
ReplaceFileis called, it prepends non-inherited ACEs to the destinationfile:
While the merging behavior is unexpected, it doesn't alter the effective
security of the file.
This commit marks the directory as protected, which breaks inheritance
at that point. As a result, ReplaceFile behaves as expected.
Also note that if the Temp ACLs are reset recursively:
Then the merging behavior does not occur and the test passes (without
this commit).
See also pseudo-inherited ACEs:
http://helgeklein.com/setacl/faq/#what-are-pseudo-protected-acls-and-pseudo-inherited-aces-setacl-2-1