-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Workaround for Mandatory ASLR #1412
Comments
Interesting. Are you sure that you have to exclude all |
No. I only have to exclude those who have relocations and aren't marked ASLR-ready. Maybe such binaries are only found under I just didn't get to it, and it wasn't that important at the moment. I just needed Git to work so I did the minimum that worked for me at the moment. (I also wanted something that works on a clean machine, so I didn't have Python and pefile to use. I later found some PowerShell packages on GitHub that parse PE files, but didn't get to it yet. Of course, if you incorporate something like this into the GfW installer you won't have those limitations. Or you can know in advance which binaries require exemptions and which don't and generate a list of files to exempt as part of the GfW build process.) |
Indeed. I know that pretty precisely because the In fact, it is even safe to assume that the opposite is true, too: all This means that all of the In fact, Git's own executables ( And yes, I think the best place for doing essentially what your Powershell script does would be the installer, as opt-in. Interested? It should be a fun ride. Step 1: install Git for Windows' SDKhttps://git-for-windows.github.io/#download-sdk It will run for a while, download ~200MB of MSYS2's packages, and build a current Git. Step 2: build an installerThis should be as easy as calling Step 3: change the installerProbably the best place would be to add it as a "component". See e.g. how the "autoupdate" feature works: https://github.com/git-for-windows/build-extra/blob/af9cff50050b15520a8a3885ccfb6c9b4b65611b/installer/install.iss#L105 You will want to add the exceptions around the same place the autoupdater is installed: https://github.com/git-for-windows/build-extra/blob/af9cff50050b15520a8a3885ccfb6c9b4b65611b/installer/install.iss#L2101-L2106 And remove those exceptions around the same place the autoupdater is uninstalled: https://github.com/git-for-windows/build-extra/blob/af9cff50050b15520a8a3885ccfb6c9b4b65611b/installer/install.iss#L2347-L2352 You can set registry values by imitating how Git for Windows' installer registers the Explorer integration: https://github.com/git-for-windows/build-extra/blob/af9cff50050b15520a8a3885ccfb6c9b4b65611b/installer/install.iss#L2040-L2054 You can enumerate files in a directory using And you can extend this to a recursive search by handing Finally, the top-level directory with which to start is the Step 4: build and test a custom installerThis is really important, to verify that it worked: Step 5: open a Pull Request... and I'll merge it as soon as possible. And then I'll celebrate! |
That's good to know. I'll take a look at implementing this when I could spare a little bit more time. |
Hello, Please, and I must reiterate, do not automatically deactivate, disable, or otherwise circumvent security options users or administrators have set up. Also @conioh, please be aware, that the proper method to enable system-wide ASLR is to set the following registry options:
To properly enforce DEP, you would run If you have done that, using exclusion rules is not possible, and applications cannot opt-out of image relocation or execute disable bit protections. |
@NightmareJoker2 you would have a ton more credibility if you would not have claimed to be able to fix Perl with regards to ASLR "in less than 5 minutes", without ever backing up such a strong claim with anything substantial. I am not saying that your statements are incorrect. I am saying that I doubt them, based on how past claims held up in bright daylight. |
@dscho I did not claim "fixing it" would take me five minutes. I said in #1196 (comment), and I quote: "[I] could send in a pull request [of what I already have] in less than 5 minutes, if I wanted to". |
Yes, all of this makes me trust those claims even less. Now you are suggesting to switch to CMake. Seriously. |
@conioh let's get back on track and just ignore the distraction, okay? Please feel free to reach out if anything in my write-up is unclear (or does not work as advertised). And thank you for offering to contribute! |
You are not supposed to trust me, you are supposed to verify them for yourself. That is the whole point of the exercise. I want you to learn something rather than take what I say at face value.
What is the problem with that? Using CMake or a similar build system abstraction layer gets you binaries built with the native tools and is easy to integrate with a continuous integration system that tells you about build errors on any targeted platform for each commit and pull request, if you so choose. You can even automate creation and tear-down of the build VM or container. Of course you can also tackle the problem of fixing the MinGW compiler, but I'd wager that to be a challenging endeavor, if nobody considered it in over 7 years. Quite possibly, also, because switching the toolset is so much easier, especially thanks to standardization of the programming language. For the last time: |
And the worst part is: this here ticket is not in need of distractions like the one provided by you. It is just wasting time and good will. So let's stop here. |
You are pretending to know something you cannot possibly have any knowledge about. You know how the saying goes: "If you assume..." (I'll stop there)
It does have several problems. The people from the VideoLAN, Rust, or Bitcoin projects can sing you songs about it. If you feel what I say is incorrect, point it out, detail your issue and render proof of error and I will correct it for future use. You are more likely than not, reading something into my words, that is simply not there, or focusing on an irrelevant detail that has next to no relation to the problem at hand.
Well, do it then, because you currently aren't. Your makefile does not mention them, and the manner in which you are compiling right now causes MinGW to not emit or to emit an incorrect relocation table.
This is clearly not a binary that was compiled and linked with the flags I mentioned. It is probable, that the
Much rather, you have proven to me, how little you care, how you can't read the information given to you or choose not to, and how you appear to rather turn off security so developers can work with the tool, and go on their merry way of ignoring it in their own applications, too, because they don't notice it missing. This is not okay, Sir.
No, the worst part is, you don't even understand yourself what the actual issue is. You have started to propose, that someone create security exception rules for your application and install those by default for all users of your application, when that is precisely what the users or system administrators who manage the systems your application gets installed onto do not want, and you are able, with relative ease, to make most parts (short of those relying on Perl or external Cygwin bits) of the applications work by adjusting linker flags during compilation, which aren't set by default. So, call this a "distraction" all you want, but for as long as you are planning on compromising security or suggest to your users they do it for you, especially that of others, even though that is more work than not to, "distract you" I must. Anything else would be feigning ignorance and utterly irresponsible. |
I hope to take a look at it the coming week. The joker did have one valid point that got lost inside all the nonsense he wrote. I intended to raise it after I see that I manage to make reasonable changes to the InnoSetup scripts etc., but since he kind-of mentioned it: I think that adding a screen and a checkbox during the install to enable setting the exceptions might be better. Sure, I think adding the exceptions is the only reasonable approach if you have Mandatory ASLR enabled, but someone else might think otherwise. He might think that he prefers not to use GfW at all in that case. Maybe he prefers to use git on WSL or write a bunch of his own tools based on libgit2, or God know what else. I think this should be his choice. |
You haven't got the faintest idea what you're talking about. Please stop wasting everybody's time. The amount of nonsense you wrote is literally incredible.
This is doubly- (or even triply-) wrong. The flags you enabled here are (documented on here): 0x1000 in unrelated while 0x10 is... what exactly? That's strike one. Even if the nonsense I just quote wasn't wrong, it's simply irrelevant. The question isn't how to enable Mandatory ASLR. The question is how to make "Git Bash" along with all its MSYS utilities to work when Mandatory ASLR is enabled. In other words, the question is how to exempt the MSYS binaries from Mandatory ASLR. You don't even understand what the issue at hand is. That's strike two.
Again, this is not only completely false, but also utterly stupid. The Microsoft Docs page on customizing Windows Defender Exploit Guard protections explicitly states it is possible and gives the exact steps required to do so. You have to pick the leave the System settings section and go into the Program settings where you choose or add a binary and select Override system settings for whatever setting you're interested in. Including ASLR. If you weren't a complete moron - something dscho already knew but I wasn't sure of - and actually read something before blabbering on here you would know that. Do you even understand what "override system settings" means? That's strike three. You're out. But you're too stupid to know it so you continue with your nonsense and post another comment, as if you haven't embarrassed yourself enough by that point.
But the thing is that regardless of the MinGW GCC not adding relocation information without Again, if you had just read more and wrote less... If it were up to me you'd be banned from the Internet. Luckily for you and unfortunately for the rest of humanity it's not up to me. Don't mention me again unless it's to apologize for all the nonsense you just wrote. |
Undocumented, obviously. And since that's the case, I can't say at this time.
The solution to which would be fixing the binaries so they can run with the protections enabled.
Wrong.
The registry options are not set by the new Exploit Guard protection options, they used to be set by EMET, which is no longer supported. Setting the registry option to enforce the options, will cause the exclusion rules to be ignored. Please do try it.
You can relocate anything, but the finer details of that are too far removed from the topic at hand. Windows does not support |
That why did you specify it? Because you're an idiot? Because you're an idiot.
Fine. Then you go ahead and do it if you're so smart. The sane people will continue working with the approach that gave millions of Windows users the ability to use Git.
Wrong.
Not only wrong, but also incredibly dumb. Like the rest of your nonsense.
Windows Defender Exploit Guard: Reduce the attack surface against next-generation malware This is also said in Moving Beyond EMET II – Windows Defender Exploit Guard, etc. Second, I have tried it. I published my script after seeing it works and fixed my problem. It is you, dumbass, who should try things before saying they don't work. Why won't you take 5 minutes and spend them on trying and learning something instead of spreading BS all over the Internet? You have no idea what you're talking about and your trolling is incredible disruptive. Please stop it. @dscho, please make him stop. |
That's the wrong binary for starters. And then some. @conioh I would like to implore you to avoid getting emotional about this. And please, please do not get lured into name calling and insults. I really want this project to be a nice place. I actually do more than that: I require it to be a nice place, because I have to work in it. I sadly agree with the problematic nature of the claims and the really unfortunate turn the discussion took, and in order to prevent even more toxicity, I (reluctantly) blocked NightmareJoker2. So can we please set this unhappy distraction aside? I still think that this ticket is important enough that it deserves not to be derailed. And I still hope that you are up to this:
Thanks! |
Might be at least a good idea to use GetProcessMitigationPolicy during the use of the affected tools and emit a warning/error if force relocation of images is enabled. This'll at least be much less confusing to the user, and allow him to take appropriate action to fix the issue. |
@sylveon I do not know about this feature. How would we use it? |
PROCESS_MITIGATION_ASLR_POLICY aslr_policy;
// GetProcessMitigationPolicy returns non-zero on success
if (GetProcessMitigationPolicy(GetCurrentProcess(), ProcessASLRPolicy, &aslr_policy, sizeof(aslr_policy)))
{
if (aslr_policy.EnableForceRelocateImages)
{
// fatal: image force relocation is enabled.
}
}
else
{
// GetProcessMitigationPolicy failed, relevant info in GetLastError()
} Additionally, if this check is performed before the msys dll is loaded (unfortunately disabling force relocation won't magically move back the images to their preffered base address, that would be too easy), we can try to turn it off: aslr_policy.EnableForceRelocateImages = false;
if (!SetProcessMitigationPolicy(ProcessASLRPolicy, &aslr_policy, sizeof(aslr_policy)))
{
// fatal: image force relocation is enabled and couldn't be turned off.
} After a bit of looking around, it seems that PROC_THREAD_ATTRIBUTE_LIST *attribute_list;
InitializeProcThreadAttributeList(attribute_list, ???);
DWORD mitigation_policy = PROCESS_CREATION_MITIGATION_POLICY_FORCE_RELOCATE_IMAGES_ALWAYS_OFF;
UpdateProcThreadAttribute(attribute_list, NULL, PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY, &mitigation_policy, sizeof(mitigation_policy), NULL, NULL);
CreateProcess(...);
DeleteProcThreadAttributeList(attribute_list); This snippet is probably incorrect, as I've never used the ProcThreadAttribute functions before. This MSDN blog article might help: https://blogs.msdn.microsoft.com/oldnewthing/20130426-00/?p=4543/ |
Is there anybody who is still interested in working on this? |
Well I'd be interested (and I feel more confident about my Win32 programming abilities so it doesn't seems as bad as when I submitted my message) but this would probably require digging in the cygwin source (and maybe the ASLR-compatible I've also never had much luck compiling anything that is cygwin-based to begin with (they always fail with the most cryptic of errors) so I try to stay away. Additionally, I'm not sure if git for windows uses a custom cygwin, but if it doesn't it's additional work to get this merged upstream. Of course, work could be done instead to implement all git commands natively without using fork, but that will most likely be a ton of work that I don't have the time to do nor confidence to get right and cause issues when updating from upstream (unless that work is submitted upstream too) |
The As to spawning other processes: the The
The idea here would be to install the Git for Windows SDK and then run
We do have a fork at https://github.com/git-for-windows/msys2-runtime, and no, you do not need to get this merged upstream, we ship with a custom MSYS2 runtime.
Well, it definitely is a ton of work, but I am happy to report that most of the important commands are already converted. So I should actually say it was a ton of work. The only major commands that still need to be converted are Having said that, Git aliases will still be executed via the MSYS2 Bash, and I think also the editor and the hooks (unless they are |
This is extremely helpful, thank you. I'll try getting something working later this month. |
@sylveon that would be awesome! |
Alright this is a bit late, but when compiling msys2-runtime by following https://github.com/git-for-windows/git/wiki/Building-msys2-runtime, makepkg fails with the following
|
@sylveon hmm. I seem to not have that problem. Maybe |
Should have looked a bit before that output, this seems to be the error:
Looks like symlinks are not properly created. I enabled developer mode and gave my user permission to create symlinks in |
Set |
spoke too fast
|
@dscho any idea about the error above and/or other communication channels you prefer to get help about those issues? |
That rings a bell. I think it had something to do with Cygwin's source code all of a sudden wanting to create symbolic links (and MSYS2's Let me look. |
I resolved that by setting The issue is those |
I don't think that IOW what should be the symlinks are instead files (containing the symlink target in plain text), and obviously that cannot work. |
The |
The answer might lie here: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=fd3619828e94a24a92cddec42cbc0ab33352eeb4 |
Was able to build and dogfood the DLL successfully after downgrading binutils to |
For the record, I just built MSYS2 runtime v3.1.4, with the fixes I had hinted at earlier. |
It seems the repo linked in the initial post doesn't exist anymore. I found a copy here https://github.com/Wurzelmann/GfW_ASLR_Exceptions for anybody who needs it now. However now I'm getting this error:
This probably is not related, but I thought it would be wise to check this in. Edit: seems to be related to #2448. |
It's also available at: https://gitlab.com/conio.h/GfW_ASLR_Exceptions I edited the top post with the updated URL. |
In the end, it seems unlikely that we can have a sensible workaround for Mandatory ASLR. The MSYS2 runtime will simply not work with it. |
Would it be possible to detect it and/or update the errors to be more helpful though? Or maybe add a warning during installation. |
@kotx if you can figure out how to detect Mandatory ASLR, then implement that in Git for Windows' installer and open a PR, I will gladly work with you to get it merged! |
There is potential detection code in #1412 (comment). It would require dynamically importing |
I don't think that Git wrapper is the best place for this. Well, maybe it's a good fall-back when no installer was involved, but it is unclear what's the least annoying way to surface that warning/error. In my mind, step number one really is to implement this check in the installer. And the responsibility for doing the actual work lies with the people who are concerned about ASLR. It's really not okay to just talk and try to leave the hard work to others. |
Is that supposed to refer to me? |
If you truly care about trying to get Git for Windows to work with Mandatory ASLR, then yes, this includes you. I, for one, don't, so it does not include me. |
The Git for Windows binaries don't work with ASLR because of the way cygwin/Mingw/MSYS/whatever implements
fork
. This is unfortunate, but ain't gonna change anytime soon (as per #1196 etc.).However, running with Mandatory ASLR enabled may be useful or desired as part of computer hardening.
A (sad) workaround is to add exceptions to the system policy and exempt the Git binaries from ASLR.
Doing that through the GUI is crazy as there are 444 such EXEs. The PowerShell
Set-ProcessMitigation
cmdlet doesn't support using full paths; it would exempt any process with a given name. The two options left are preparing an XML of an undocumented but easily understandable schema and importing it usingSet-ProcessMitigation -PolicyFilePath
or editing the Registry manually.The following is a PowerShell script that directly manipulates the Registry and adds exceptions for all the EXE files inside the Git directory except the 3 found at the root: https://gitlab.com/conio.h/GfW_ASLR_Exceptions
It expects a path to the Git directory (e.g. "C:\Program Files\Git") and looks for the git install directory in the Registry if no path is provided. The path parameter can be used for "portable git" installs.
There's virtually no error checking there. I have no idea what happens if you gives a bad path, for example. What I can say is that "it works on my computer" (RS3 x64, if anybody cares).
The text was updated successfully, but these errors were encountered: