msys2 / msys2-installer

The one-click installer for MSYS2
BSD 3-Clause "New" or "Revised" License
561 stars 87 forks source link

CVE-2022-37172 #51

Open lazka opened 2 years ago

lazka commented 2 years ago

https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-37172

So there is an issue to track this.

I couldn't find any other installer that handles this, so I'm not sure what the right thing to do is (except cygwin, which seems to set some hardcoded ACLs, but I couldn't find their code/logic)

The only thing I can think of is:

icacls.exe C:/msys64 //reset # just for debugging, this goes to the default we have now
icacls.exe C:/msys64 //grant "$USERDOMAIN\\$USERNAME:F" # full permissions for the installing user
icacls.exe C:/msys64 //inheritancelevel:r # disable inheritance, remove inherited permissions

Though not sure if we have the current user somehow in the installer, and if that breaks anything else. It might give more permissions as before when installing into a more strict directory (??)

On could argue that this is how Windows works.. you will get the permissions of the directory you install to, and if C is world writable, then MSYS2 is too. But I understand the the defaults of both Windows and our installer make this insecure.

Ideas / thoughts / and pointers to other projects welcome.

elieux commented 2 years ago

It's an interesting combination of circumstances:

The defaults:

> icacls C:\
C:\ BUILTIN\Administrators:(OI)(CI)(F)
    NT AUTHORITY\SYSTEM:(OI)(CI)(F)
    BUILTIN\Users:(OI)(CI)(RX)
    NT AUTHORITY\Authenticated Users:(OI)(CI)(IO)(M)
    NT AUTHORITY\Authenticated Users:(AD)
    Mandatory Label\High Mandatory Level:(OI)(NP)(IO)(NW)

First three (admins, system and users) are very reasonable, the two after (authenticated users) are the contentious ones, the last one (label) shouldn't play a role. I'm not quite knowledgeable enough to say whether read access for authenticated users is required for anything; in my experience, everything works without it.

Note that the creator/owner will have implicit full access.

Proposal

It depends on how complex we want this. The proposal above is essentially an installation only for that singular user and no one and nothing else. I'd add at least the system account.

We should definitely give a choice. I guess we could have three modes:

  1. keep inherited, as before
  2. per-user install (should not be admin): grant full access to system, maybe full access to self, remove inherited
  3. system-wide install (requires admin): grant full access to system, admins, read to users, remove inherited

We could preselect the mode according to the installation path and current user, but that's already a bit further than this proposal should probably go. :)

I'm not sure how to design the whole process around it though. I'd like to present the permissions (to see the consequences of choosing mode 1) to the user somehow (maybe just give a button that opens the Explorer properties of the directory with the Security tab active), but we'd need to create the directory first, which means get the path first and possibly elevate as well.

lazka commented 2 years ago

I'm afraid that giving users a choice will (a) just confuse them and (b) make them select the default anyway.

What about just doing (2) always and have a page in the documentation for alternative recommended setups?

I assume most users are on a single user machine, so they shouldn't see any difference compared to now with (2) right?

goyalyashpal commented 1 year ago

Link to new page: https://www.cve.org/CVERecord?id=CVE-2022-37172

Umh, while I agree that most users are on a single user machine... I thought to share my perspective and reasoning behind:

lhmouse commented 2 months ago

Suggestion:

  1. The installer can create the c:\msys64 directory before installing anything, then copy the ACL from c:\program files, so most installed files will be protected from non-priviledged processes.
  2. The /tmp directory should be writeable for everyone. On Linux it has the sticky bit set; I think the correct setup for Windows would be to allow Users to create files and subdirectories, and for CREATOR OWNER to have full control.
  3. Each user's home directory in /home should be owned by themself.