search

git-for-windows / git

A fork of Git containing Windows-specific patches.
http://gitforwindows.org/
Other
8.36k stars 2.54k forks source link

Windows User Groups not counted for new repository ownership rules #3798

Closed churchs19 closed 1 year ago

churchs19 commented 2 years ago

Setup

$ git --version --build-options

git version 2.35.2.windows.1
cpu: x86_64
built from commit: 518ccba2352ce721cabbbf2933869c3c3313d1c3
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
$ cmd.exe /c ver

Microsoft Windows [Version 10.0.19044.1466]
# One of the following:
> type "C:\Program Files\Git\etc\install-options.txt"
> type "C:\Program Files (x86)\Git\etc\install-options.txt"
> type "%USERPROFILE%\AppData\Local\Programs\Git\etc\install-options.txt"
$ cat /etc/install-options.txt

Editor Option: Notepad++
Custom Editor Path:
Default Branch Option:
Path Option: Cmd
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: WinSSL
CRLF Option: CRLFAlways
Bash Terminal Option: MinTTY
Git Pull Behavior Option: Merge
Use Credential Manager: Enabled
Performance Tweaks FSCache: Enabled
Enable Symlinks: Disabled
Enable Pseudo Console Support: Disabled
Enable FSMonitor: Disabled

If the repository was cloned in an admin prompt, Windows sets the Owner to Administrators (<machine name>\Administrators).

Details

PowerShell though the issue occurs system-wide.

If the repository was cloned in an admin prompt, Windows sets the Owner to Administrators (<machine name>\Administrators). Any user-space applications then treat the same repository as not trusted resulting in the 'git log' failed with code 128: fatal: unsafe repository message.

I would expect that git should acknowledge that my user account is a member of the group that is the owner of the folder and treat the repository as safe as a result. The git 2.35.2 update appears to only treat the repository folder as safe if the owner of the folder exactly matches the account name, not if the account is a member of the group that owns the folder.

See above.

All repositories where the Windows folder owner is a group, not a specific user account.

rimrul commented 2 years ago

This is probably by design. A group is not your user. Your user might be a member of that group, but even then, there could be zero or more other members in that group.

A case could be made that we should consider a group as valid ownership if the current user is the only user in that group.

derrickstolee commented 2 years ago

@rimrul is correct: when the owner is a group with multiple users, it is vulnerable to attack from any member of that group. Thus, we want to be extra careful unless the repository is marked with safe.directory.

The "only user in this group" idea is interesting, but might be difficult to implement for low value. Is there a good reason to make the group the owner if there is only one user in the group?

churchs19 commented 2 years ago

@rimrul, and @derrickstolee, is this implemented the same way in other OS implementations? I guess I fail to see why a folder owned by a group shouldn't be allowed to be used by any member of the group? Isn't that how roles based authentication is supposed to work? Otherwise we either have to go and change the Windows ownership of every folder to make sure they're all owned by the one specific user.

In my case, we have some applications that require Visual Studio to run as administrator because they can only run/debug through IIS. In this case, given what has been stated, the initial git clone would have to be done as administrator and every process that runs through it would also have to be done as administrator because of this. As an end-user, I'd have to be conscious of whether I happened to be in an administrator level prompt or a regular user prompt every time I did a git clone because that would have ramifications for every client application on that cloned repository folder.

I think I understand the potential security issue, but it's beginning to seem to me that the "fix" for said issue wasn't completely thought through.

rimrul commented 2 years ago

@rimrul, and @derrickstolee, is this implemented the same way in other OS implementations?

That isn't how permissions on unix systems work. A file (or folder) is owned by a certain user not a group.

I guess I fail to see why a folder owned by a group shouldn't be allowed to be used

None of the users in the group are trustworthy. All of the users in the group have full access to a folder owned by the group. Therefore a folder owned by a group is not trustworthy.

atompkins commented 2 years ago

I think this stems from a fundamental misunderstanding of how security is implemented on Windows. If a user was not trustworthy they would not be a member of the group. It is best practice on Windows that files and folder are owned by groups NOT individual users. An untrusted user would not have the permissions required to create an arbitrary .git folder. It seems as if this "fix" has not been thought through very well. Given the amount of things it breaks I suspect most people will just turn off the check. The "vulnerability" itself is only applicable to misconfigured systems.

rimrul commented 2 years ago

Sure, in an ideal world you wouldn't have untrustworthy users.

PhilipOakley commented 2 years ago

@rimrul I think we have to remember that we are all unreliable in equal measure.

It is unfortunate that the Unix and Windows approaches to group usage and teams doesn't align, along with the different uses of the same terminology, such as "trustworthy", which is very context dependant.

At the moment the solution (for the Windows group users) is simply to forget about the CVE and assume everything '*' is safe, as we have no mechanism on Windows of setting safety-by-group, rather than the Unix safety-by-user when that is the core policy of the particular software team/company. Maybe a double asterisk '**' ?

It's going to be an interesting time...

derrickstolee commented 2 years ago

At the moment the solution (for the Windows group users) is simply to forget about the CVE and assume everything '*' is safe, as we have no mechanism on Windows of setting safety-by-group, rather than the Unix safety-by-user when that is the core policy of the particular software team/company. Maybe a double asterisk '**' ?

I think adding a new config key would be appropriate here, such as safe.directoryOwnerMatch = group or something like that, to avoid overloading safe.directory too much.

It's going to be an interesting time...

Now that we are out of embargo, we can take our time getting feedback from the community about how they use Git in these shared contexts. From that feedback, we can carefully design something that works for the largest number of users.

mfriedrich74 commented 2 years ago

I can confirm group ownership is best practices on file shares. It avoids one major problem. New files created by one user will still be able to be modified or deleted by another of the same group. Without sacrificing security. Would you use user ownership, you would have to give the users full admin rights to access each other files, if one user or app decided to mess with file's permissions. And yes, by administrator choice that created the group, or by definition, all members of the group are trustworthy. I still need to read the CVE though.

On Mon, Apr 18, 2022, 9:12 AM Derrick Stolee @.***> wrote:

At the moment the solution (for the Windows group users) is simply to forget about the CVE and assume everything '' is safe, as we have no mechanism on Windows* of setting safety-by-group, rather than the Unix safety-by-user when that is the core policy of the particular software team/company. Maybe a double asterisk '**' ?

I think adding a new config key would be appropriate here, such as safe.directoryOwnerMatch = group or something like that, to avoid overloading safe.directory too much.

It's going to be an interesting time...

Now that we are out of embargo, we can take our time getting feedback from the community about how they use Git in these shared contexts. From that feedback, we can carefully design something that works for the largest number of users.

— Reply to this email directly, view it on GitHub https://github.com/git-for-windows/git/issues/3798#issuecomment-1101397664, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZH5SBBHMXYJ2YQZZTY4G3VFVNT3ANCNFSM5TOHIIWA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

rimrul commented 2 years ago

And yes, by administrator choice that created the group, or by definition, all members of the group are trustworthy.

No. Just because some admin decided that some user is trustworthy enough to create or modify some files does absolutely not mean every other user that's also allowed to access these files should blindly trust them.

uniphiny commented 2 years ago

PLEASE FIND THE ATTACHED DOCUMENTS

On Tue, Apr 19, 2022 at 1:01 PM Matthias Aßhauer @.***> wrote:

And yes, by administrator choice that created the group, or by definition, all members of the group are trustworthy.

No. Just because some admin decided that some user is trustworthy enough to create or modify some files does absolutely not mean every other user that's also allowed to access these files should blindly trust them.

— Reply to this email directly, view it on GitHub https://github.com/git-for-windows/git/issues/3798#issuecomment-1102554431, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMJAXY7WN3NLH2NWUX7S3SDVF2OB7ANCNFSM5TOHIIWA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mfriedrich74 commented 2 years ago

No. Just because some admin decided that some user is trustworthy enough to create or modify some files does absolutely not mean every other user that's also allowed to access these files should blindly trust them.

Absolutely yes. That's how teams work. The admin decided to trust them. His decision. And approved by managers on which he acts on.

On Tue, Apr 19, 2022, 8:03 AM uniphiny @.***> wrote:

PLEASE FIND THE ATTACHED DOCUMENTS

On Tue, Apr 19, 2022 at 1:01 PM Matthias Aßhauer @.***> wrote:

And yes, by administrator choice that created the group, or by definition, all members of the group are trustworthy.

No. Just because some admin decided that some user is trustworthy enough to create or modify some files does absolutely not mean every other user that's also allowed to access these files should blindly trust them.

— Reply to this email directly, view it on GitHub < https://github.com/git-for-windows/git/issues/3798#issuecomment-1102554431 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AMJAXY7WN3NLH2NWUX7S3SDVF2OB7ANCNFSM5TOHIIWA

. You are receiving this because you are subscribed to this thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/git-for-windows/git/issues/3798#issuecomment-1102556207, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZH5SDGX7GXG53SVTUGZF3VF2OH3ANCNFSM5TOHIIWA . You are receiving this because you commented.Message ID: @.***>

rimrul commented 2 years ago

Not every multi user environment is a company. And even in company environments we can't always assume best practices environments where no involved parties are malicious or incompetent.

dscho commented 2 years ago

I do see a couple of misunderstandings that I need everybody to understand who wants to participate in this discussion:

Is this safe.directory change disruptive? Yes, it is. It is not only disruptive for regular, benign users, but also for attackers.

Would there have been an alternative to the safe.directory change that would still prevent the attack vector, yet be less disruptive for regular users? No, I don't believe so, at least not after @derrickstolee introduced support for safe.directory=*.

As to the idea put forth in this ticket, namely to add special rules for Windows groups in certain forms and shapes? It looks very brittle to me. The thing I cannot allow is to re-introduce the attack vector, now that attackers have been made aware by the CVE's publication. And this Windows groups special treatment? That has all the signs of doing precisely that: re-introduce the same vulnerability that we just plugged.

dscho commented 2 years ago

The admin decided to trust them. His decision. And approved by managers on which he acts on.

Please show some respect for the person who bore you by not excluding all people who do not share your gender.

rimrul commented 2 years ago

On Windows, you can create a folder and then assign ownership to somebody else. That is not possibly on Unix

That's not entirely true. chown exists on many unix systems. Though it is usually limited to administrative users.

mfriedrich74 commented 2 years ago

The admin decided to trust them. His decision. And approved by managers on which he acts on.

Please show some respect for the person who bore you by not excluding all people who do not share your gender.

I can assure you, it has nothing to do with disrespect. I try better next time.

mfriedrich74 commented 2 years ago

So, I generally disagree with treating any group owner as untrusted. I personally think it would be sufficient to just check for a changed object ID as owner and to not check or care whether the object ID is of type user or group. The rejection of all groups as owners (if my understanding is correct) goes a bit far in my opinion, but I agree that is more secure. On the other hand, I also disagree with using an elevated prompt or Administrator account for git (even though there might be valid use cases).

I can say that group ownerships in companies (like where i work) are not rare. I had it seen being used on team file shares and some users (typically not software engineers) use them to store repository full clones. Those users will be surprised if git gets upgraded.

I checked creating a folder on C:\ and can confirm it is being owned by my own account and not a group. I would think group ownership on the systems drive is not a default windows setup. Not sure where this would be occurring on a local system.

All that said, I cannot think of a better solution that is still as secure as it is right now. I believe though that this breaking change needs to be announced (on the download page?)/documented if it isn't yet (i have not checked). I will certainly bring this topic up in my company. Also, I think the error message could be improved to contain a hint towards being a security measure and group ownerships being used.

churchs19 commented 2 years ago

I believe though that this breaking change needs to be announced (on the download page?)/documented if it isn't yet (i have not checked). I will certainly bring this topic up in my company. Also, I think the error message could be improved to contain a hint towards being a security measure and group ownerships being used.

This has been a really fascinating discussion and this might just be the best suggestion to help in the short term. It's one thing to say we changed behavior for a security issue, but neither the error message nor the documentation currently help a user stuck with group ownership on a folder.

It's easy to say that a user shouldn't be using Administrator for git, but there are a number of perfectly legitimate scenarios for a Windows developer that will cause git processes to run as Administrator. For example, if you have a classic ASP.NET application that has to be hosted in IIS, Visual Studio insists that it has to run as Administrator or it won't open the project. When this is the case, the underlying integrated git operations also run as Administrator, triggering this issue where some changes are owned by the Adminstrators group instead of an individual account.

mfriedrich74 commented 2 years ago

By the way, regarding security and the CVE, what stops an attacker to set the new safe.directory option? That option is ignored if set in .git, correct?

dscho commented 2 years ago

I think the error message could be improved to contain a hint towards being a security measure and group ownerships being used.

This has been a really fascinating discussion and this might just be the best suggestion to help in the short term. It's one thing to say we changed behavior for a security issue, but neither the error message nor the documentation currently help a user stuck with group ownership on a folder.

Valid points. Please do work on this, I am looking forward to seeing a Pull Request.

what stops an attacker to set the new safe.directory option? That option is ignored if set in .git, correct?

Indeed, safe.directory is only respected in the system config and in the global (i.e. user's) config.

Caiptain1 commented 2 years ago

I have been testing this a bit and there's one thing that doesn't make sense which would contradict the expectations of @churchs19 outlined above.

I was curious about the different scenarios for the repositories. If you clone the repository as a user and try accessing it as an admin, it still lets you do that. I don't see why would that be the case. I took the code snippet that is supposed to check ownership and it does, in fact, say that admin group is not the owner of the path but Git still allows to do all operations :? Am I missing something?

dscho commented 2 years ago

If you clone the repository as a user and try accessing it as an admin, it still lets you do that.

You mean, if you run the clone in a regular session, then start an elevated session as the same user and use git? Keep in mind that this is still the same user running, just in elevated mode. I.e. you're not really running under an "administrator account" now, it's just that the same user now runs with elevated permissions (which is possible for users who are in the administrators group).

CrossVR commented 2 years ago

If there's a concern about malicious group members then the ownership check also isn't reliable, since it's very common on Windows for folders outside the user directory to be shared with all users even if owned by the current user.

The real solution here is to allow the user to whitelist the parent folder they use to store their repos and not trust any folder outside of the user directory even if owned by the current user unless it's in a whitelisted parent folder.

This would both take care of the inconvenience of having to specify every single repo and would actually add security rather than reduce it. Of course whitelisting folders that are shared with other users is still ill-advisable, but it's at least better than git assuming it's safe just because the user owns the directory or users resorting to safe.directory = *.

mfriedrich74 commented 2 years ago

Hi,

I think you are mixing up NTFS permissions versus share permissions. The owner check against the NTFS permissions works even if share permissions are broader. The reason is that actual permissions is the intersection of both sets of permissions.

Best regards, Mike

On Thu, Jun 9, 2022, 9:59 AM Jules Blok @.***> wrote:

If there's a concern about malicious group members then the ownership check also isn't reliable, since it's very common on Windows for folders outside the user directory to be shared with all users even if owned by the current user.

The real solution here is for git to allow the user to whitelist the parent folder they use to store their repos and not trust any folder outside of the user folder even if owned by the current user.

This would both take care of the inconvenience of having to specify every single repo and would actually add security rather than reduce it.

— Reply to this email directly, view it on GitHub https://github.com/git-for-windows/git/issues/3798#issuecomment-1151157776, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZH5SANIKMIK4BNHVRTAV3VOH2E3ANCNFSM5TOHIIWA . You are receiving this because you commented.Message ID: @.***>

CrossVR commented 2 years ago

@mfriedrich74 I don't think I'm mixing them, if a malicious group member has write permissions to a parent folder or to the repo folder itself they can put malicious things in a .git folder. The NTFS permissions can allow group members to write to the .git folder even if the user owns it and that's very common on Windows outside of the user folder. That's much less common on Linux where users rarely share write permissions, which is why the ownership check protects against it there.

I think the current implementation of the ownership check on Windows does very little to actually protect against the vulnerability. The mitigation tactic used on Linux simply doesn't provide the same protections on Windows, because group permissions are treated very differently between these two systems.

mfriedrich74 commented 2 years ago

Hi,

If i understood right, the check that is done here is whether the patent folder of .git folder (and all files underneath?) is owned by the user executing git. If the current user owns it, the person running git can be assumed responsible for permissions and files.

Now with the fix, while a malicious group member could create a .git folder if permissions allow, this person would not be able to tell git to manipulate the parent and the sibling folders if those are owned by someone else. No matter who uses git (owner or attacker).

If you have a group share on Windows and have automatic group ownership enabled, git will refuse to clone/use repositories on it. If you have personal ownership enabled, git will stay within the boundary of the owned users folders. So, a malicious group member could write .git folder that downloads malicious code the next time git is used by any member of the group. But git will refuse.

I think, it's not about protecting from any malicious writing. It's about preventing to use git to have an user download malicious files from the internet without the user knowing.

Best regards, Mike

On Sat, Jun 11, 2022, 8:10 AM Jules Blok @.***> wrote:

@mfriedrich74 https://github.com/mfriedrich74 I don't think I'm mixing them, if malicious group members have write permissions to the directory they can put malicious things in a .git folder. The NTFS permissions can allow group members to write to the .git folder even if the user owns it and that's very common on Windows outside of the user folder. That's much less common on Linux, which is why the ownership check works there.

— Reply to this email directly, view it on GitHub https://github.com/git-for-windows/git/issues/3798#issuecomment-1152912880, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZH5SDUIBAYBIC5HPBFQE3VOR65DANCNFSM5TOHIIWA . You are receiving this because you were mentioned.Message ID: @.***>

mapwiz commented 2 years ago

Mike, if an organization is compromised enough to have a domain admin adding malicious users to trusted groups, they have bigger problems than that. They can manipulate things to have users download malicious code anyway.

The only thing accomplished by refusing to consider group ownership is to drive people to completely bypass the security measure with safe.directory=*

It's probably a good idea to exclude groups built into Windows, but this can be accomplished by checking the RID of a group's SID. I wrote a proof of concept program to check folder group ownership but I'm hesitating to make the pull request @dscho mentioned.

mapwiz commented 2 years ago

@dscho Would a branch named "safegroups" or some such be a way to get this going?

dscho commented 2 years ago

@mapwiz Sure. I hope that that branch will be able to convince me that the logic is safe enough not to risk introducing easy attack vectors.

alexyz commented 2 years ago

I am getting the same issue. My repository is "owned" by the Administrators group, but is reported as unsafe by git. On a basic level, this doesn't make sense, as Administrators is a special group on Windows whose members are by definition trusted. You could hard code this exception.

However I think the problem goes a bit deeper than that. Windows uses ACLs for file permissions, and many groups and users can be assigned different levels of access.

If you create anything outside of your user directory, it will probably inherit the Authenticated Users/Modify permission. It's debatable whether this is a security risk, as generally you trust Authenticated Users (otherwise they wouldn't have an account), and if you really don't want that group to modify something, it's the administrators job to remove that inherited permission and the OSes job to enforce it. In any case, it's not really the applications job to check permissions.

dscho commented 2 years ago

My repository is "owned" by the Administrators group, but is reported as unsafe by git. On a basic level, this doesn't make sense, as Administrators is a special group on Windows whose members are by definition trusted.

The directory might be owned by the Administrators group, but the .git directory might not be. So no, that does not make it safe.

In v2.37.1, we introduced code that looks both at .git as well as the directory containing .git, which makes your argument somewhat valid, but not completely. The repository is owned by someone else. That in and of itself is dubious, you have to tell Git that you trust this nevertheless, and that's exactly what safe.directory is about.

It is in many ways, this security measure has the same goal as VS Code asking whether you trust the authors in a given workspace. The goal is the same: to make sure that no foreign code is run inadvertently.

In other words, I am still convinced that the current strategy should not be changed in the way that this here ticket asks for.

alexyz commented 2 years ago

The directory might be owned by the Administrators group, but the .git directory might not be. So no, that does not make it safe.

The .git directory is within the repository directory, and will inherit its permissions. In fact my repository itself has entirely inherited permissions. This is normal and the OS can do its job enforcing those permissions.

The repository is owned by someone else. That in and of itself is dubious, you have to tell Git that you trust this nevertheless, and that's exactly what safe.directory is about.

It's not dubious on Windows. As a previous commenter pointed out, it's quite common for groups to own directories on Windows. And Administrators really is a special group defined by Microsoft. If you have malicious administrators, you have much bigger problems that git can't solve.

And anyway, it's actually the whole access control list that matters, not the owner. If you want to warn users about risk, it should really be about the default inherited permission from the volume root (Authenticated Users/Modify), or any non administrator user or group in the ACL that isn't the current user. But Administrators or Local System is OK.

dscho commented 2 years ago

The .git directory is within the repository directory, and will inherit its permissions.

No Try running git init as a regular user in C:\Windows\Temp.

alexyz commented 2 years ago

No Try running git init as a regular user in C:\Windows\Temp.

I did this, and the .git folder inherits the permissions from C:\Windows\Temp. I'm not sure what your point is. Basically any time you create a file or directory in Windows, it will inherit the parent object's permissions.

To summarise:

Git does not error against the threat "malicious non-Administrator user with default inherited write access to repository". Maybe it should - however I think this bug report would get 100x more comments from surprised users.

Git does error about the threat "malicious Administrator". Which does not make sense. It's basically a contradiction.

dscho commented 2 years ago

My bad. I thought that you were talking about ownership when you mentioned permissions because it did not make any sense to counter my concerns about ownership information with talk about permissions.

mfriedrich74 commented 2 years ago

Should those mentioned use cases not easily avoidable by simply creating a dedicated folder for the repo by yourself?

So, don't do git init in C:/windows/temp.

But do instead: mkdir myrepo cd myrepo git init

And usually, the ownership is not inherited (at least I have not found inheriting ownerships in a local standard windows install). This way you own the repo folder as well as the .git folder. If you can own the .git folder, there should be no reason to not also create and own the parent.

Best regards, Mike

On Thu, Jul 14, 2022, 8:37 AM Johannes Schindelin @.***> wrote:

My bad. I thought that you were talking about ownership when you mentioned permissions because it did not make any sense to counter my concerns about ownership information with talk about permissions.

— Reply to this email directly, view it on GitHub https://github.com/git-for-windows/git/issues/3798#issuecomment-1184397054, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZH5SBEHNY4T26PJQH7A4DVUACY5ANCNFSM5TOHIIWA . You are receiving this because you were mentioned.Message ID: @.***>

dscho commented 2 years ago

So, don't do git init in C:/windows/temp.

That's not the problem. The problem is if someone else does it for you, and you don't notice, and your Git prompt runs git status and launches an FSMonitor that incidentally was also configured by that someone else, running a program with your credentials that you never intended to run with your credentials.

The problem is much larger than you pretend it to be.

mfriedrich74 commented 2 years ago

I wasn't intending to give a solution to the security problem. I was giving a solution to the annoyance of having to set excludes when the original patch is present.

On Tue, Jul 26, 2022, 10:16 AM Johannes Schindelin @.***> wrote:

So, don't do git init in C:/windows/temp.

That's not the problem. The problem is if someone else does it for you, and you don't notice, and your Git prompt runs git status and launches an FSMonitor that incidentally was also configured by that someone else, running a program with your credentials that you never intended to run with your credentials.

The problem is much larger than you pretend it to be.

— Reply to this email directly, view it on GitHub https://github.com/git-for-windows/git/issues/3798#issuecomment-1195542812, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABZH5SADW572NXCAK5D5AMTVV7XKBANCNFSM5TOHIIWA . You are receiving this because you were mentioned.Message ID: @.***>

wolfbeast commented 1 year ago

Landed here from #4177 where I reported the same issue.

I think there may be some misunderstanding here what a normal situation is for ownership of directories/folders on Windows. First of, ownership and permissions are not the same thing. Secondly, it is extremely common, especially in shared/commercial environments working on network locations to have NTFS ownership of directories be set to groups (often dev/department teams) and being determined at the SSO/network level. Not having a mechanism to allow group ownership in git and looking at user ownership only completely defies the point of trying to make it more secure, and it's undoable to add every repo directory to the global git config for every user every time a new repo is created, leaving the only workaround blanket-marking all directories everywhere as safe. I don't think that was the intention of introducing safe.directory :P

There is nothing dubious about e.g. a user "john" working in a repository that is owned by "product-devs"; john didn't create the repo, and doesn't solely own it (the group product-devs does). Git should check what groups the user belongs to and check if any of the groups the user is a member of matches the ownership information of the directory, and if so, not consider it dubious, because it simply isn't.

GabenGar commented 1 year ago

How do you change the ownership of a folder recursively on windows? I had a repo which I moved from linux machine through a local superuser share into an unelevated user, so it ended up with superuser ownership. chown --changes --recursive --no-dereference <user> <path> (ran as a superuser), which I assume comes from this package, reports that it does change ownership, but it isn't reflected in the related windows menus and VSCode doesn't see it as safe either. So either chown doesn't work as expected (being a unix-like abstraction over windows permission management) or I am misunderstanding how it should work. The menu option which says something about "inheritance" only applies to the files in the folder, not the subfolders, so it can get pretty arduous to change this way on the whole repo. The other way is to run a sketchy PowerShell script I found on StackOverflow, which I'd rather not do.

dscho commented 1 year ago

There is nothing dubious about e.g. a user "john" working in a repository that is owned by "product-devs"

I fear that this statement shows a lack of understanding of the threat scenario that safe.directory wants to protect against.

The concrete threat scenario is that many an inadvertent Git operation (for example, a Git prompt that helpfully shows the status of a Git worktree) would pick up a Git repository where john does not even suspect one to be, a repository that was maliciously created by somebody else in the product-devs group, and that activates certain features that potentially run specifically-crafted code to act on behalf of unsuspecting john.

For example, john could be calling a Git Bash in a network drive owned by that product-devs group and some attacker could have created a .git directory at the root of said drive, and john would certainly not suspect a Git worktree to be there. And merely directing their Git Bash there would run code on their behalf that they did not expect to run, and never had a chance to inspect, let alone to prevent from running.

If john does understand that there is a Git worktree there, or really in any location that is not owned by them but by a group they are member of, they have to indicate that understanding by adding a safe.directory entry.

Any idea how to make the safe.directory protection more convenient to use is welcome, as long as it does not reintroduce the original threat.

dscho commented 1 year ago

either chown doesn't work as expected (being a unix-like abstraction over windows permission management)

@GabenGar It is my understanding that this is exactly your problem: chown expects a simplistic Unix permission model whereas the file system on which it has to work uses Access Control Lists, a rather sophisticated model compared to the Unix model.

You most likely need to use icacls instead, I suspect with /setowner <username> /t /l /q.

wolfbeast commented 1 year ago

I fear that this statement shows a lack of understanding of the threat scenario that safe.directory wants to protect against.

Well, indeed you are absolutely right. But that scenario you explained is actually totally irrelevant since it would not be the task of git to prevent against it, but rather of the IT auditors to make sure no member of the group is acting in a malicious manner. Also, of note, group ownership does not automatically provide all individual group members with full access (as I already said ownership != permissions) and it would be the responsibility of network administration to make sure ACLs are configured properly.

On the contrary, the scenario you sketched seems to indicate your own lack of understanding of organisational IT management and how network administration is performed in the normal case within larger organisations, where groups are more often than not organisational departments and where individual user permissions are of almost no importance and where access and rights are almost if not entirely exclusively managed through group memberships and group policies. It is impossible to assign ownership to individual users in such scenarios because the effective owner must be the group with policy access. As said it is normal, and not in any way dubious, that directory ownership is a group and not an individual user. Any new repo created would, in the current case, require each and every individual group member to manually add the repo to the safe.directory list to be able to work with it. That is not a feasible way of handling this and must be addressed one way or another for git to continue to be useful and fulfilling its role properly.

Of course if you demand the perceived threat that groups are, in fact, not trusted to manage access and rights be maintained, then there is no solution here as the two concepts are mutually exclusive and cannot be reconciled, but be aware that it effectively makes using git on a file system where rights and permissions are determined by policy, not individual assignment, impossible, without completely disabling your safe.directory system.

dscho commented 1 year ago

@wolfbeast try to create a C:\.git directory.

TomaszSzt commented 1 year ago

For me this new security future is disruptive and at least confusing.

I have all rights to read and write a folder, right? I have rights to read and write some folders above. I was GIVEN the rights to c:\ but can be refused, right? I have given those right to my coworkers so they could do something.

I even log-in as a different user from time to time and still have access to files, since both users do have correct permissions and I can do my work.

The result is that git config --system safe.directory=* is a must for me.

I do understand the idea behind the "attack", but hell, this is a built-in "feature" of GIT. The number of times I did manipulate an incorrect repository due to that is countless ( I use repo-in-repo folder structure from many reasons ).

Correct me if I am wrong, but the entire idea behind the "attack" is that if I run:

  cd x:\a\b\c
  git something

expecting x:\a\b\c.git and that folder is not there, but there is x:\a.git then that second folder will be used without me knowing that?

Isn't it how the GIT was designed?

Due to the way it is done now users will either add safe.directory=* or the "white list" of folders will grow and grow continuously. I don't think You will be able to find a user who will do a cleanup from time to time, so this list will only grow and sooner or later will cover most of Your working space.

If this function is designed to prevent attacks without disrupting the work-flow it should be done in an another way.

First, if users can create .git somewhere then it is simple - the are allowed to do it and allowed to suffer all the effects. If users are allowed to read it, then they are allowed. This is up to IT management to set-up correct permissions to not allow it. The well designed permission system is strongest line of defense. That is, if its necessary at all.

Going the path proposed by safe.directory I would rather like to be able to restrict GIT, per user or per system, from where it can search for .git folders, depending on where the command is started. I have, for an example three separate code bases, one of C and one for JAVA and a last one for mechanical design files. Like:

  x:\sources\java
  x:\sources\C
  x:\designs

If I could set that if starting folder for git command is x:\sources\java* then it can't got up past x:\sources\java* and alike for other projects, the I think it would cover 99% of cases without messing up permissions, company wide trust management and etc. with the ownership of certain folders.

My humble request is: make this security option "opt-in" rather than "opt-out". This may be as simple as asking a question during install and automatically setting up the safe.directory=* if user will say: "I don't need such a security management".

Best regards, Tomasz Sztejka.

dscho commented 1 year ago

My humble request is: make this security option "opt-in" rather than "opt-out".

No. Just because it works for you does not mean that you get to demand to make millions of users vulnerable. Git for Windows should be secure by default. It's the same as with Visual Studio Code's recent change where you have to specify manually that you trust a given folder.

I sincerely hope that you understand why it would be dangerous, say, for Git Bash's prompt to pick up a Git repository at C:\.git (including its hooks), a location where very few Git users expect a Git repository, let alone code that is executed on their behalf without their consent.

wolfbeast commented 1 year ago

You still seem to refuse to understand that filesystem access control is handled at the organisation level in many places, not at the individual user level, especially in larger companies and orgs. Nobody would have permission to create anything at the root of a drive (except the sysadmin, and maybe not even them without a lot of effort). Trying to have git take over that access role is completely wrong for those environments and will cause a ton more work for every individual user who now has to add safe dirs for every repo they get to work in. How does that not bother you? Does that still not compute?

Just because it works for you does not mean that you get to demand to make millions of users vulnerable.

Instead, you get to demand that millions of users grow an ever-complex and unwieldy list of directories in their individual git configs, and that is OK in your book? Where group ownership is used, you can be pretty sure that the vulnerability you're trying to cover here will already have been covered by established access control policies, so all it adds is hurdles to work with code.

And thus, everyone with considerations bigger than individual end-user systems will make a blanket disable. Congrats, your narrow view has effectively made the feature into a nuisance.

PhilipOakley commented 1 year ago

Having worked in big corporates, I'm well aware that there are many things done for administrative convenience and stratification of projects that look at the 'wrong' security model when considering the threats that this restriction addresses.

The threat here is that there can be very fast, worm speed, lateral traversal of organisations that use these group methods once there is a single 'infection' at some distant user. If you are in more than one group, then you are an infection vector between the two groups. You don't need to be anything other than a careful user to still be a threat, as are all the other careful normal users that are on the Venn diagram of belonging to 2 groups.

If your organisation is so sure of itself then it will have no problem considering the original CVE and applying it's learning to the organisation's own scenario and compiling it's own carefully thought out solution that can be dog-fooded, and fed back to the wider Git project in the best traditions of Open Source.

It maybe that you need to use individual Git publish repos to achieve the proper separation of concerns. Git is distributed.

The administration will need to re-think how it works, based on the modern 'zero cost of production' model for digital data. Administrations using (conceptually) retro paper-based methods are slowly falling away. Git tore up the old SCM model.

Security is hard. {About 1,190,000,000 results}

CrossVR commented 1 year ago

I still think that allowing users to specify a parent directory with a wildcard is a good compromise. It still (partly) addresses the security concern as it doesn't allow .git repos to be processed any higher than that parent directory, but it also accomodates existing workflows that don't use the user folder to store repositories.