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

Fix the permissions on files and directories on Windows #6356

Closed
bmw opened this issue Sep 7, 2018 · 20 comments
Closed

Fix the permissions on files and directories on Windows #6356

bmw opened this issue Sep 7, 2018 · 20 comments

Comments

@bmw
Copy link
Member

bmw commented Sep 7, 2018

See the discussion at #6296 (comment).

@bmw bmw changed the title Fix the permissions on directories on Windows Fix the permissions on files and directories on Windows Oct 19, 2018
@bmw
Copy link
Member Author

bmw commented Oct 19, 2018

Another relevant discussion is at #6374 (comment).

@adferrand
Copy link
Collaborator

So lets start with the specifications I have so far.

@adferrand
Copy link
Collaborator

File permissions are totally different between Linux and Windows. As we all know, on Linux it is working as a series of bits that describe permissions for the file user owner, user group and everyone, on actions that are read, write and executable/walkable. Good start to know what we need, and so what will be translated to Windows.

Assuming that Certbot is executed on a given user (generally root, but not mandatory), we have mainly this 5 situations:

  1. files that are readable only by the user (umask 0400)
  2. files that are readable/writeable only by the user (umask 0600)
  3. directories that are readable/writeable/walkable only by the user (umask 0700)
  4. files that are readable/writable by the user, readable by everybody (umask 644)
  5. directories that are readable/writable/walkable by the user, readable/walkable by everybody (umask 755)

And of course, root can do everything he want.

@adferrand
Copy link
Collaborator

On Windows, everything is completely different. It is working using ACL on files, that describes for entities what they can do. Actions are full control, modify, read & execute, list folder content, read, write, special permissions. Entities are users, groups, built-in system accounts, meta-groups (like "everyone"). Given an action required by a user, and given the user itself, and the groups he is part on, authorization on the action will be given if any of the entities related to the user has the permission.

So obviously, making a bijective relation between Linux and Windows security models is impossible. We need to extract a subset of the capabilities on the two systems that make sense for Certbot usage, and construct the bijective relation on it. I will do that by making a security model description on Windows for the 5 situations I described on Linux.

@adferrand
Copy link
Collaborator

Note that a file has a owner on Windows, and this owner has every right on it.

@adferrand
Copy link
Collaborator

More in-depth about Windows file permissions, using: https://docs.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2003/cc783530(v=ws.10)

I think I am repeating myself about Windows, but it is a mess, again :)

It is extremely complicated, but one convenient way to see things is that: on every ACL can be applied a generic security descriptor. Theses generic descriptors are GENERIC_ALL, GENERIC_WRITE, GENERIC_READ and GENERIC_EXECUTE. They are translated into the relevant effective rights for folders and for files.

Another thing to consider it that the concept of owner is independent from ACL. Owner has the rights to modify ACLs, but in theory, he can have no ACL that concerns him: in this case, until a relevant ACL is added (by himself, because he has the right to do it), owner cannot do anything with the file (read, modify, delete, move, execute and so on.).

But all of this means that we can translate a particular POSIX mode (for user, group or all) into generic descriptors. For the most common modes:

  • 7 (rwx) = GENERIC_ALL (or GENERIC_WRITE + GENERIC_READ + GENERIC_EXECUTE)
  • 6 (rw) = GENERIC_WRITE + GENERIC_READ
  • 5 (rx) = GENERIC_READ + GENERIC_EXECUTE
  • 4 (r) = GENERIC_READ

@adferrand
Copy link
Collaborator

adferrand commented Nov 6, 2018

Now, we need to define what corresponds to user, group and all, and how we can express that root can do anything.

First about root. There is no root on Windows, but something is really close to: it is NT AUTHORITY\SYSTEM. This special account represents the machine itself, with the highest privileges on local, and nothing on remote (Active Directory). This will typically on this account that all Windows services that require administrative privileges will run on. So it is our root: any permission apply on a file will give authoritatively GENERIC_ALL to NT AUTHORITY\SYSTEM.

Second about the user. Obviously, it will be the account on which Certbot is currently running. Any file generated by Certbot will have its owner set to it, and effective DACL will be calculated using the POSIX mode applying to the user, with the conversion table described before.

Third about all. There is a special account on Windows, that represents anyone: it is everybody. So similarly to user, we calculate a DACL using the mode applying to all.

Finally, the group. There is not relevant group for a given user on Windows. In particular, Authenticated users is way to large, as it includes anyone that can connect to the machine except Guest. So it should be ignore. Anyway, group as effectively the same value than all in the Certbot files.

@adferrand
Copy link
Collaborator

adferrand commented Nov 6, 2018

Finally, a script example, that gives an equivalent of 744 to a file given as input.

import sys

import ntsecuritycon as con
import win32security
import win32api

def main(filename):
    user = win32security.LookupAccountName("", win32api.GetUserName())[0]
    system = win32security.LookupAccountName("", "System")[0]
    everyone = win32security.LookupAccountName("", "Everyone")[0]

    security = win32security.GetFileSecurity(filename, win32security.DACL_SECURITY_INFORMATION)

    dacl = win32security.ACL()
    dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con.GENERIC_ALL, user)
    dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con.GENERIC_ALL, system)
    dacl.AddAccessAllowedAce(win32security.ACL_REVISION, con.GENERIC_READ, everyone)

    security.SetSecurityDescriptorDacl(1, dacl, 0)
    win32security.SetFileSecurity(filename, win32security.DACL_SECURITY_INFORMATION, security)

if __name__ == "__main__":
    main(sys.argv[1])

This will require pywin32 to be installed.

@adferrand
Copy link
Collaborator

Something also to take into account, and it is in the script I posted above. Permissions on Windows can be inherited. Meaning that when you create a file, every permission from the folder, or every most restrictive permission you can find in the hierarchy (if set to be propagated) will be applied to the file.

On a script, you can break inheritance by creating a new DACL, like win32security.ACL() and set it to the target file. So any file permission script will need to be applied at each file creation, and ideally at each Certbot execution (at least for critical files).

@bmw
Copy link
Member Author

bmw commented Nov 6, 2018

Thanks for all of the research here @adferrand. This looks great.

But all of this means that we can translate a particular POSIX mode (for user, group or all) into generic descriptors. For the most common modes:

7 (rwx) = GENERIC_ALL (or GENERIC_WRITE + GENERIC_READ + GENERIC_EXECUTE)
6 (rw) = GENERIC_WRITE + GENERIC_READ
5 (rx) = GENERIC_READ + GENERIC_EXECUTE
4 (r) = GENERIC_READ

This maps things back to UNIX remarkably well. If GENRIC_READ, GENERIC_WRITE, and GENERIC_EXECUTE are the same read, write, and execute used as column headings in the table under "Special Permissions" in this section of the doc you linked earlier, it seems we can use these three values to set most permissions we care about.

The special permissions that are missing are "Delete Subfolders and Files", "Delete", "Change Permissions", and "Take Ownership". I don't think we'll ever want to set the last two. The owner can change permissions and I see no reason we should let other users claim ownership of the file.

I'm interested in the two delete permissions though. Maybe we don't need to worry about this and don't want to give users those permissions. I suppose my two biggest concerns are what are the conventions here (if they exist) and will the lack of a delete permission cause problems for Certbot when it tries to delete/replace its files?

So it is our root: any permission apply on a file will give authoritatively GENERIC_ALL to NT AUTHORITY\SYSTEM.

Does the local system user get these permissions automatically and implicitly or do we have to set them? I was reading a bit about this user here, but it's not very clear.

Third about all. There is a special account on Windows, that represents anyone: it is everybody. So similarly to user, we calculate a DACL using the mode applying to all.

This should work well. The document you linked says anonymous users aren't part of the everybody group, however, I'm not sure we need to worry about giving anonymous access to Certbot's files.

Related to how to handle permissions for users other than the owner, the document you linked suggests rather than explicitly denying users access, you just don't give them an entry in the ACL. So if we wanted a file that was readable/writable only by the owner, we shouldn't set an entry for everyone. I think this is a good idea and just wanted to mention it here for us to remember in the future.

Finally, the group. There is not relevant group for a given user on Windows. In particular, Authenticated users is way to large, as it includes anyone that can connect to the machine except Guest. So it should be ignore. Anyway, group as effectively the same value than all in the Certbot files.

This makes sense. The one related piece I think we should look into is what permissions would servers on Windows need to read these files. We should look at popular web servers like IIS and Apache as well as any other popular software such as mail servers we can think of that would need certificates.

This is tricky and probably not something we can design a catch all solution for, but it might be worth thinking a bit about up front.

This will require pywin32 to be installed.

This is probably fine. Possible concerns I (may naively) have are:

  1. Do we need this dependency? Certbot has been criticized (somewhat rightly in my opinion) for having too many dependencies (see Too many dependencies #1301). There's no need for us to reinvent the wheel, but what does this get us over ctypes for example?
  2. Is this library the best option? Is it the most well maintained, tested, etc.? I'm not familiar with the landscape here.
  3. We'll probably want to make this a conditional dependency so we don't (try to) install it on UNIX machines. There's some gotchas there which are discussed a bit at Don't install argparse on python >=3.2 #4379, but nothing we can't manage if it's worth the cost.

On a script, you can break inheritance by creating a new DACL, like win32security.ACL() and set it to the target file.

Did you test or find documentation about this? I couldn't quickly find anything.

Also, can we set this during file creation or do we have to create a file and then modify its ACL?

@adferrand
Copy link
Collaborator

adferrand commented Nov 6, 2018

This maps things back to UNIX remarkably well. If GENRIC_READ, GENERIC_WRITE, and GENERIC_EXECUTE are the same read, write, and execute used as column headings in the table under "Special Permissions" in this section of the doc you linked earlier, it seems we can use these three values to set most permissions we care about.

The special permissions that are missing are "Delete Subfolders and Files", "Delete", "Change Permissions", and "Take Ownership". I don't think we'll ever want to set the last two. The owner can change permissions and I see no reason we should let other users claim ownership of the file.

I'm interested in the two delete permissions though. Maybe we don't need to worry about this and don't want to give users those permissions. I suppose my two biggest concerns are what are the conventions here (if they exist) and will the lack of a delete permission cause problems for Certbot when it tries to delete/replace its files?

In fact it is even better than what you think. After the tests I did, GENERIC_ALL effectively gives Full control (and so includes all actions), and GENERIC_WRITE effectively gives Delete and Delete Subfolders. For what I see, the GENERIC_* gives an almost perfect mapping to what you have with POSIX.

As if after seeing what they have done on the NTFS permissions system, engineers in Microsoft looked the paperboard, then each other, and one of them said: 'Well guys, I think we screw up, let's do this POSIX thing ...'

Does the local system user get these permissions automatically and implicitly or do we have to set them? I was reading a bit about this user here, but it's not very clear.

When file or directory is created, every permission aggregated to the most restrictive for each account will be inherited. On the top level, C:, local system has Full control. So if not set explictly otherwise in the hierarchy, any file create will also have that permission. This authorization is fine. But NOT the other one, that says Everyone has read access.

So we do not care about the inheritance, we break it and set exactly what we want.

This should work well. The document you linked says anonymous users aren't part of the everybody group, however, I'm not sure we need to worry about giving anonymous access to Certbot's files.

Related to how to handle permissions for users other than the owner, the document you linked suggests rather than explicitly denying users access, you just don't give them an entry in the ACL. So if we wanted a file that was readable/writable only by the owner, we shouldn't set an entry for everyone. I think this is a good idea and just wanted to mention it here for us to remember in the future.

You are right, since Windows XP, anonymous users are in an separated group. And yes, I think we do not care about giving access to anonymous users.

For the rest, yes, it is exactly what we want. Composed with the systematic reinitialisation of the DACL for a file, we just give the permissions we want. My example was about 744, but remove the line about Everybody, and you have 700.

This makes sense. The one related piece I think we should look into is what permissions would servers on Windows need to read these files. We should look at popular web servers like IIS and Apache as well as any other popular software such as mail servers we can think of that would need certificates.

This is tricky and probably not something we can design a catch all solution for, but it might be worth thinking a bit about up front.

For what I saw, any serious service (like Nginx or Apache) will require administrative access to be installed, and will register itself under the LocalSystem, or any account with administrative access. As any local Administrator will be able to impersonate LocalSystem anyway, maybe we could set also GENERIC_ALL to the local administrator group. It will not reduce the security.

Any other service without administrative privileges will not be able to read the privkey. But it is quite similar to the situation on Linux: Certbot is run as root, privkey can only be read by root. So no service that run outside root is able to use Certbot. To my mind, the constraint is the same.

This is probably fine. Possible concerns I (may naively) have are:

1. Do we need this dependency? Certbot has been criticized (somewhat rightly in my opinion) for having too many dependencies (see #1301). There's no need for us to reinvent the wheel, but what does this get us over ctypes for example?
2. Is this library the best option? Is it the most well maintained, tested, etc.? I'm not familiar with the landscape here.
3. We'll probably want to make this a conditional dependency so we don't (try to) install it on UNIX machines. There's some gotchas there which are discussed a bit at #4379, but nothing we can't manage if it's worth the cost.

I will try to use ctypes. I will certainly be on the edge of the madness while navigating into win32 API. If I stay on the bright side, we will use native calls. Otherwise, sorry, we will need another dependency, but conditionally for Windows systems. Appart that, yes pywin32 is a pretty solid library of ~1000 stars on GitHub, with active development.

Did you test or find documentation about this? I couldn't quickly find anything.

Also, can we set this during file creation or do we have to create a file and then modify its ACL?

In fact, you can deduce it from the documentation, and I tested it. The DACL is a set of ACL associated to a given file. The DACL is constructed automatically when the file is created, and is able to link to existing DACL to maintain inheritance. But nothing prevent you from creating a entirely new DACL, and associate it to the file. If this new DACL as no link to another DACL, there is no inheritance.

And final word for your final sentence.

We need to destroy the default DACL created with the file and make our own. Otherwise, there is no way to ensure that private keys will not get a general read access to everyone by inheritance of any folder in the hierarchy.

If there is anything we need to respect in the specification to have a decent protection about sensible material in Certbot, this is this.

@adferrand
Copy link
Collaborator

adferrand commented Nov 7, 2018

OK @bmw, I give up about using ctypes directly to talk to the Windows kernel. It is just too hard, too low level, without enough examples and documentation. I will just make bad code here.

I prefer to rely on pywin32. You can specify dependencies that are platform specific. It is such a common pattern for pywin32 that it is used as an example in the official documentation: http://setuptools.readthedocs.io/en/latest/setuptools.html#declaring-platform-specific-dependencies

Is it ok for you?

@bmw
Copy link
Member Author

bmw commented Nov 8, 2018

I'll respond to the rest of your response here soon but conditionally depending on pywin32 is totally fine. I just wanted to ask the questions about it I did in my previous post to try and be a little diligent about us taking on more dependencies, but if there's much additional development overhead (and it sounds like there's a significant amount), we should just use pywin32.

I'm sure you've got this, but keep in mind the package might not be available at runtime so you probably can't just import its modules like usual.

@bmw
Copy link
Member Author

bmw commented Nov 8, 2018

Everything you wrote sounds good to me! My only comment is:

As any local Administrator will be able to impersonate LocalSystem anyway, maybe we could set also GENERIC_ALL to the local administrator group. It will not reduce the security.

How are local administrators able impersonate LocalSystem?

Maybe giving all local administrators access to these files makes sense regardless, but these are the kind of decisions that should be made carefully.

@adferrand
Copy link
Collaborator

adferrand commented Nov 8, 2018

Well, you cannot impersonate an account in a non-destructive way, like su on Linux, because you need the account password. But as an administrator, you can change the password of any account.

Any account that is in the administrators local group can invoke his administrative rights, and does everything he want. The only limitation is it cannot demote itself to a normal account if there is no other administrator account on the system.

So yes, in effect, to give an explicit acl to the administrators local group does not weaken the security model. It is like having root access on Linux.

@adferrand
Copy link
Collaborator

About pywin32, I tried to write a little the logic using only ctypes. With 50 lines, I gain the ability to obtain the accounts of LocalSystem and Everyone, and that corresponds to 2 lines with pywin32.

And when I check the code in pywin32 about these operations, I see that a dozen more potential flaws are taken care of. At this low level towards the kernel, and with administrative rights, the risk to break things is way to high.

Definitely, a prefer to rely on guys that know what they are doing, and accumulated experience on a lot of bad things that could happen when manipulating directly the kernel API.

@bmw
Copy link
Member Author

bmw commented Nov 8, 2018

So yes, in effect, to give an explicit acl to the administrators local group does not weaken the security model. It is like having root access on Linux.

Nice. This makes sense and seems to me like something we should do.

About pywin32, I tried to write a little the logic using only ctypes. With 50 lines, I gain the ability to obtain the accounts of LocalSystem and Everyone, and that corresponds to 2 lines with pywin32.

And when I check the code in pywin32 about these operations, I see that a dozen more potential flaws are taken care of. At this low level towards the kernel, and with administrative rights, the risk to break things is way to high.

Definitely, a prefer to rely on guys that know what they are doing, and accumulated experience on a lot of bad things that could happen when manipulating directly the kernel API.

We should definitely use pywin32 then. I didn't realize that it was at that much of a higher level than just making the system calls yourself in ctypes.

@adferrand
Copy link
Collaborator

All right! So I will start to make an implementation.

@bmw bmw added this to the 0.32.0 milestone Feb 20, 2019
@bmw bmw modified the milestones: 0.32.0, 0.33.0 Mar 7, 2019
@bmw bmw modified the milestones: 0.33.0, 0.34.0 Apr 3, 2019
@bmw bmw modified the milestones: 0.34.0, 0.35.0 Apr 29, 2019
@bmw bmw modified the milestones: 0.35.0, 0.36.0 Jun 3, 2019
@bmw bmw modified the milestones: 0.36.0, 0.37.0 Jul 12, 2019
@bmw
Copy link
Member Author

bmw commented Jul 29, 2019

I believe this is done.

@ams-tschoening
Copy link

For what I saw, any serious service (like Nginx or Apache) will require administrative access to be installed, and will register itself under the LocalSystem, or any account with administrative access.[...]

The interesting question is: Why did you think you need to care at all?

I'm not running web servers using admin accounts by default, my files and directories have the proper permissions and most software simply works, except certbot, which breaks ACLs because of its wrong assumptions. Certificates are renewed with one account and the web server runs in another one not being member of the admins group. That would easily work with permissions based on inheritance if certbot simply would keeps things as they are. The result of your implementation instead is that things don't work anymore and the web server needs to be part of the admins group for almost no reason. How can that be argued with increased security?

It's not, your implementation can't be made secure in any way: Either certbot and web server need to share the same account, having unnecessary write permissions on each other directories. Or both accounts need to be members of the admin group, which is unnecessary and insecure for most web servers. Don't deal with file system permissions unless for very good reasons. Like this example shows, most software doesn't have those reasons and simply breaks things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants