Implemented a workaround for mandatory ASLR #513
Conversation
|
Isn't this a near-duplicate of #512? What is the response to #512 (comment), then? Or is there no response at all? |
|
@dscho Sorry yeah it is a duplicate the original pull request got a bit messy in terms of force pushes and silly mistakes etc. so I decided to not bother with it but then changed my mind. The response is here: #512 (comment) |
There was a problem hiding this comment.
I see where you're coming from, yet I also respect the concerns raised that ASLR should not be disabled quietly, it must be a consciously made choice on the users' part.
I want to end up with a solution that addresses that concern and still gives you what you so rightfully want: an easy way to disable the ASLR protection for the MSYS executables (if, and only if, Mandatory ASLR is enabled, that is).
The good news is that I think that this is possible, and I left some feedback I believe to help toward that end.
installer/install.iss
Outdated
| @@ -3450,7 +3450,7 @@ begin | |||
| WizardForm.StatusLabel.Caption:='Running post-install script'; | |||
| Cmd:=AppDir+'\post-install.bat'; | |||
| Log('Line {#__LINE__}: Executing '+Cmd); | |||
| if (not Exec(Cmd,ExpandConstant('>"{tmp}\post-install.log"'),AppDir,SW_HIDE,ewWaitUntilTerminated,i) or (i<>0)) and FileExists(Cmd) then begin | |||
| if (not Exec(Cmd,ExpandConstant('/SILENT ' + IntToStr(Integer(WizardSilent())) + ' >"{tmp}\post-install.log"'),AppDir,SW_HIDE,ewWaitUntilTerminated,i) or (i<>0)) and FileExists(Cmd) then begin | |||
There was a problem hiding this comment.
So we're never showing the dialog in the installer now, right? That's kind of at odds with the intention to let the user still be in control.
There was a problem hiding this comment.
It isn't a classic /SILENT flag where the presence of the flag changes anything, but the value of the following argument does.
See the parsing code:
@SET silent=0
@IF /I "%1" == "/SILENT" (
@IF "%2" == "0" (
@SET silent=%2
) ELSE (
@IF "%2" == "1" (
@SET silent=%2
)
)
)
@IF "%silent%" == "0" (
@SETLOCAL EnableDelayedExpansionThere was a problem hiding this comment.
Ah. That's confusing, /SILENT does not usually take a value anywhere else.
post-install.bat
Outdated
| @SET powershell_script=Add-Type -AssemblyName PresentationFramework,System.Windows.Forms;^ | ||
| function Main(^)^ | ||
| {^ | ||
| $title = 'Git Installer';^ | ||
| $icon = [System.Windows.Forms.MessageBoxIcon]::Question;^ | ||
| $body = \"Mandatory ASLR security exceptions are necessary for the executables in the `\"usr/bin`\" directory and Git Bash to function correctly on Windows systems with mandatory ASLR enabled which is a Windows security feature that is disabled by default.`n`nWARNING: If you have installed Git to a path modifiable by a unprivileged user or an external drive this will add mandatory ASLR security exceptions for those paths on which could introduce a security vulnerability^!`n`nWARNING: Doing this significantly slows down the load time of the program settings list in the exploit protection section of the Windows security application but it will load eventually^!`n`nWould you like to attempt privilege escalation if necessary and add these exceptions?\";^ | ||
| $buttons = 'YesNo';^ | ||
| $response = [System.Windows.MessageBox]::Show($body, $title, $buttons, $icon^);^ | ||
| if ($response -eq 'No'^)^ | ||
| {^ | ||
| exit;^ | ||
| }^ | ||
| $process = Start-Process powershell.exe -Verb RunAs -WindowStyle Hidden -PassThru -Wait -ArgumentList '-Command \"cd ''%cd%''; Get-ChildItem -Path usr/bin -Filter *.exe -File | ForEach-Object { Set-ProcessMitigation -Name $_.FullName -Disable ForceRelocateImages }\"';^ | ||
| if ($process.ExitCode -ne 0^) {^ | ||
| Main;^ | ||
| }^ | ||
| }^ | ||
| Main | ||
|
|
||
| powershell.exe -Command !powershell_script! |
There was a problem hiding this comment.
That's a rather lengthy piece of PowerShell code embedded in Batch code (with the necessary quoting, rendering it less readable than I would desire).
But even more immediate is my concern that this message box would annoy users who do not even have Mandatory ASLR enabled, and therefore need not be bothered disabling it for the MSYS executables. Can you think of any way to detect, programmatically, that Mandatory ASLR is enabled? Preferably implemented in InnoSetup's variant of Pascal, this check should be used to avoid even mentioning this new installer option unless the user needs it (see more on the UI implications of this patch below).
It would probably make more sense to turn this into a stand-alone PowerShell script, with proper options, say, to:
- disable ASLR on the executables
- undo the ASLR disabling on the executables
- specify a different set of executables
This script could then be run from the installer, when (and if!) the user checked a dedicated box to force-disable ASLR on the MSYS executables.
That would also solve a rather big problem with the current patch: the user's decision is not recorded. And cannot be pre-populated via /LOADINF.
And that issue, that this patch implements a separate UI from the installer just to ask the user whether they want to disable ASLR for the MSYS executables, that issue is not only relevant to recording previous choices, it also is relevant because it causes unnecessary inconsistencies regarding when the user is asked for their choices (the current paradigm is: answer a gazillion questions, and then the installer runs and won't bother you again until everything is done, but with this here patch, there are more questions lurking after the lengthy extraction process) as well as the Look & Feel (the message box popped up via PowerShell most likely won't follow the same visual language as InnoSetup, making it all look a bit amateur-ish).
Finally, separating this out into a PowerShell script that lives, say, in C:\Program Files\Git\cmd, would allow users to re-think their decision whether or not to disable ASLR after installing Git for Windows, without forcing them to reinstall it.
There was a problem hiding this comment.
Okay I'll get on it.
Correct me if I'm wrong or missed anything anywhere below but the installer should:
- Check if mandatory ASLR is enabled preferably in InnoSetup's variant of Pascal
- If and only if it is display a option natively in the installer to add the exceptions
- Record the users decision
And the PoweShell script should:
- Live in the C:\Program Files\Git\cmd directory.
- Support both enabling and disabling the ASLR exceptions
- Support specifying a different set of executables
There was a problem hiding this comment.
@AndrewBorah38 yep, that would be ideal!
There was a problem hiding this comment.
@dscho Updated, let me know if I missed anything :)
There was a problem hiding this comment.
@AndrewBorah38 Nice!
Since it is the policy in Git for Windows (just like in the Git project) to liberally rewrite commits before they get merged, so as to have an easier story to follow when reading the commit history, I allowed myself to squash the changes into a single commit.
While at it, I also marked up the WARNING labels in red, fixed grammar a little, rewrote the combined commit message a little, and added support for running ./installer/release.sh -d SecurityOptions even on systems where mandatory ASLR isn't enabled. I also had to change the CI quite a bit (and rebase because of conflicts) because the installation had failed (due to RdbSecurityOptions[SO_MandatoryASLR] being only initialized when mandatory ASLR was enabled, even if the installer always tried to access its Checked attribute later on), and I had to add a way to figure out why.
Here is the diff: /~https://github.com/git-for-windows/build-extra/compare/0b4f248693873337462d8df839eb4e22418487f6..00fe18b4965f9f53f11f37d56298f30fbe545800
Could I ask you to verify that the installer shows the page for you (assuming that your system has mandatory ASLR enabled)?
Also: I have further questions:
- https://learn.microsoft.com/en-us/windows/security/threat-protection/overview-of-threat-mitigations-in-windows-10#converting-an-emet-xml-settings-file-into-windows-10-mitigation-policies seems to suggest using
-Disable MandatoryASLRinstead of-Disable ForceRelocateImages. Do you have any authoritative source which flag to use? - when I tried to find any documentation of that registry setting, it would appear that there is none (but there are a lot of non-authoritative sources, some suggesting other registry keys, but there is something authoritative about the registry setting but it talks about fonts!). Could you find an authoritative source that we can link to in this PR, and in the commit message?
There was a problem hiding this comment.
@AndrewBorah38 did you find any answers for those questions? I really would like to move this PR along, ideally it would just need a few more adjustments to the commit message(s).
There was a problem hiding this comment.
@dscho Sorry I've been on holiday and couldn't login to reply, you can expect a response regarding the questions you asked within the next few days.
There was a problem hiding this comment.
@dscho Yeah it still shows the prompt.
As for using "-Disable MandatoryASLR" instead, this command doesn't actually exist, here's a link to the docs for the PowerShell command: https://learn.microsoft.com/en-us/powershell/module/processmitigations/set-processmitigation?view=windowsserver2022-ps#example-1
As for the mitigation options key I found references to it here if that is sufficient: https://learn.microsoft.com/en-us/microsoft-365/security/defender-endpoint/troubleshoot-exploit-protection-mitigations?view=o365-worldwide
There was a problem hiding this comment.
@AndrewBorah38 I reworded the commit message: 53000e5. Could you have a final look over it and give a 👍 or 👎?
There was a problem hiding this comment.
@dscho Yep looks good 👍
dscho
force-pushed
the
mandatory-aslr-workaround
branch
4 times, most recently
from
1da1358 to
29eb7e0
Compare
This file is missing when the `make-file-list.sh` script is called from `please.sh create-sdk-artifact build-installer`, for example. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The `$ARCH_BITNESS` environment variable seems to _happen_ have existed, but it is not the value we want to use here: the installer that we just built is what should determine which bitness we are using. This squashes a linter warning. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Instead of using an environment variable to communicate between two steps, which cannot easily be validated by the GitHub workflow linter, use the `result` output (which is the intended use case for that feature). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Under certain circumstances, the installer would be built both in the `build-artifacts` matrix as well as in the `sdk-artifacts` one. That is a waste of time and resources, so let's just build the installer in the latter matrix (which validates both 32-bit and 64-bit build, and is therefore a bit more comprehensive than the former matrix in that instance). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The idea is to force the `sdk-artifacts` job to be run when more than 200 lines are in the numstat of `please.sh`. However, when the numstat of `please.sh` is empty, the check failed because the empty string is not 0. Abuse a quirk in Bash where `$(())` (i.e. the arithmetic evaluation of the empty string) yields 0. This avoids symptoms in the CI runs' logs of the form: /home/runner/work/_temp/17a8b050-ac43-4536-9818-63b75d561be8.sh: line 23: test: 200: unary operator expected Incidentally, this bug ensured that the `sdk-artifacts` job was run always, not only when necessary. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We do log the output into a file so that it can be inspected, but currently the output is not shown unless the installation succeeded. That's bad. We need the output _particularly_ when the installation failed. Let's just print it in a separate step, making sure that it is printed whenever the installation fails. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We have this awkward situation where the installer can be built as part of the `sdk-artifacts` matrix as well as of the `build-artifacts` one. In the former case, we validated the installer, in the latter we did not. Now that we no longer build the installer in both, we must copy/edit the validation steps into the latter matrix. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
mandatory-aslr-workaround
branch
from
29eb7e0 to
b1a9bc7
Compare
dscho
force-pushed
the
mandatory-aslr-workaround
branch
from
b1a9bc7 to
00fe18b
Compare
ASLR (Address Space Layout Randomization) is an important tool to secure software, by avoiding predictable addresses from which malicious shell code could be executed in attacks exploiting vulnerabilities. Unfortunately, the way the MSYS2 runtime works is incompatible with ASLR and would not allow processes to be spawned via the emulated `fork()` syscall. The MSYS2 runtime, though, is the work horse behind Git Bash and all the other executables in `/usr/bin/`. With this change, the installer detects when mandatory ASLR is enabled, and in that case offers the user to add security exceptions so that the executables in the "usr/bin" directory and Git Bash function correctly. The main logic is implemented in a PowerShell script that can be run separately from the installer, too. Seeing as it is a security concern to disable mandatory ASLR in such a big sweep, let's warn the users loudly (and with red "WARNING" labels) about it. Note: The documentation at https://learn.microsoft.com/en-us/windows/security/threat-protection/overview-of-threat-mitigations-in-windows-10#converting-an-emet-xml-settings-file-into-windows-10-mitigation-policies suggests to use the invocation Set-ProcessMitigation [...] -Disable MandatoryASLR However, as the documentation at https://learn.microsoft.com/en-us/powershell/module/processmitigations/set-processmitigation clearly documents (and, `Get-Help Set-ProcessMitigation`, too), the `MandatoryASLR` value is not applicable, but the `ForceRelocate` one is. The example even mentions `ForceRelocateImages`, which is what we use, then. Also note: There is apparently no official documentation indicating how to detect when mandatory ASLR is enabled. The best documentation found in the process of developing this here patch is at https://learn.microsoft.com/en-us/microsoft-365/security/defender-endpoint/troubleshoot-exploit-protection-mitigations and suggests looking at `HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Image File Execution Options`, which is what this patch does. Signed-off-by: Andrew Riseborough <AndyBorah38@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This lets the user run `installer/release.sh -d SecurityOptions` to see what the page looks like, even if they do not have Mandatory ASLR enabled. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
mandatory-aslr-workaround
branch
from
00fe18b to
de60614
Compare
|
/add relnote feature When installing into a Windows setup with Mandatory Address Space Layout Randomization (ASLR) enabled, which is incompatible with the MSYS2 runtime powering Git Bash, SSH and some other programs distributed with Git for Windows, the Git for Windows installer now offers to add exceptions that will allow those programs to work as expected. The workflow run was started |
When installing into a Windows setup with Mandatory Address Space Layout Randomization (ASLR) enabled, which is incompatible with the MSYS2 runtime powering Git Bash, SSH and some other programs distributed with Git for Windows, [the Git for Windows installer now offers to add exceptions](#513) that will allow those programs to work as expected. Signed-off-by: gitforwindowshelper[bot] <gitforwindowshelper-bot@users.noreply.github.com>
|
Thank you @AndrewBorah38! |
This comment has been minimized.
This comment has been minimized.
Implemented a workaround for mandatory ASLR in the installer by asking the user if they wish to add security exceptions so that the executables in the "usr/bin" directory and Git Bash function correctly on Windows systems with mandatory ASLR enabled which is a Windows security feature that is disabled by default.
Fixes:
The aforementioned issues are present in all versions of Windows I have tested with the current Git v2.41.0.windows.3 installer which are:
To be specific you can replicate this issue by enabling mandatory ASLR on a Windows system, rebooting and then running the Git installer and attempting to load Git Bash after at which point you will be greeted with a screen that says the following:
Synopsis:
The post installation script would ask the user if they wish to add security exceptions so that the executables in the "usr/bin" directory and Git Bash function correctly on Windows systems with mandatory ASLR enabled and attempt privilege escalation if necessary. (It is necessary to attempt privilege escalation if this script was called by the portable installer which runs as the local user by default.)
If the silent flag is provided the installer would not attempt to add mandatory ASLR security exceptions to ensure that users are informed of this update and don't add them accidentally when installing silently via the command line or 3rd party Windows package managers.
If the user has installed Git to a path modifiable by a unprivileged user or an external drive the installer would add exceptions for those paths which could introduce a security vulnerability and doing this would significantly slow down the load time of the program settings list in the exploit protection section of the Windows security application but it would load eventually. (However the user would be informed of these two constraints via the message box)
I try'd to mimic the installers current methodology as closely as possible by implementing this in the post installation Batch file where DLL rebasing occurs however had to use PowerShell for the core of the workaround because Batch doesn't have native message box or exploit protection management functionality.