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

File name extension requirement broke Git hooks #16480

Open
5 tasks done
anttiah opened this issue Nov 17, 2021 · 37 comments
Open
5 tasks done

File name extension requirement broke Git hooks #16480

anttiah opened this issue Nov 17, 2021 · 37 comments
Assignees
Labels
KeepOpen The bot will ignore these and not auto-close Needs-Investigation The behavior reported in the issue is unexpected and needs further investigation. WG-Engine core PowerShell engine, interpreter, and runtime WG-NeedsReview Needs a review by the labeled Working Group

Comments

@anttiah
Copy link

anttiah commented Nov 17, 2021

Prerequisites

Description

Now that script files must have .ps1 file name extension in Windows, because of #15859, it's no longer possible to have Git hooks written in PowerShell. This is a breaking change!

From Git documentation at https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks (emphasis by me)

To enable a hook script, put a file in the hooks subdirectory of your .git directory that is named appropriately (without any extension) and is executable. From that point forward, it should be called. We’ll cover most of the major hook filenames here.

Git hooks in written in PowerShell have worked flawlessly since the file name exetension requirement was removed in PowerShell 6.x. After the 7.2 release, trying execute a git command that has hooks associated with it results in an error message and commit fails.

Steps to reproduce

Make sure Git is installed with full support for all commands. Meaning that paths

  • C:\Program Files\Git\cmd
  • C:\Program Files\Git\mingw64\bin
  • C:\Program Files\Git\usr\bin

are present in user or system PATH environment variable. Git for Windows installer will do this for you.

Then create a file named pre-commit with content:

#!/usr/bin/env pwsh
Write-Verbose 'Hello from pre-commit hook' -Verbose
exit 0

Note that shebang line is required here because of how Git was ported to Windows.

Make sure the file name does not have any extension and place the file into .git\hooks directory of a local git repository.

Make some changes in the repo and try to commit them. For example: git commit -m "Testing" or use VS Code's version control tab.

Expected behavior

pre-commit hook gets executed:

VERBOSE: Hello from pre-commit hook
[testing ca1faea] Testing
 1 file changed, 1 insertion(+)
 create mode 100644 test.txt

Actual behavior

Commit fails with error message:

Git: Processing -File '.git/hooks/pre-commit' failed because the file does not have a '.ps1' extension. Specify a valid PowerShell script file name, and then try again.

Error details

No response

Environment data

Name                           Value
----                           -----
PSVersion                      7.2.0
PSEdition                      Core
GitCommitId                    7.2.0
OS                             Microsoft Windows 10.0.19043
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Visuals

No response

@anttiah anttiah added the Needs-Triage The issue is new and needs to be triaged by a work group. label Nov 17, 2021
@iSazonov iSazonov added Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a WG-Security security related areas such as JEA labels Nov 17, 2021
@anttiah
Copy link
Author

anttiah commented Nov 17, 2021

I was so upset about this change that I forgot to add repro steps. Sorry for that. I have now added the missing details to the original message.

@iSazonov
Copy link
Collaborator

Make sure the file name does not have any extension

Why cannot you add .ps1 extension?

@anttiah
Copy link
Author

anttiah commented Nov 17, 2021

Why cannot you add .ps1 extension?

As stated in the description, it's required by Git:
https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks

To enable a hook script, put a file in the hooks subdirectory of your .git directory that is named appropriately (without any extension) and is executable. From that point forward, it should be called. We’ll cover most of the major hook filenames here.

@daxian-dbw daxian-dbw added Breaking-Change breaking change that may affect users and removed Issue-Question ideally support can be provided via other mechanisms, but sometimes folks do open an issue to get a labels Nov 17, 2021
@daxian-dbw
Copy link
Member

@SteveL-MSFT Was the change required due to security reason? Can you please take a look?

@jborean93
Copy link
Collaborator

That's weird at least on Linux it works for me

echo -e '#!/usr/bin/env pwsh\necho "test"' > /tmp/test_script
chmod +x /tmp/test_script

# All 3 below work on 7.2.0
/tmp/test_script
pwsh /tmp/test_script
pwsh -File /tmp/test_script

Maybe it's a problem specific for Windows where shebangs don't usually work and it relies on the file extension to have an associated executable. Still if tools like Mingw/Cygwin/WSL can work with the shebang and still pick up Windows executables then I don't think checking the extension is really ideal.

@anttiah
Copy link
Author

anttiah commented Nov 17, 2021

That's weird at least on Linux it works for me

This issue originated from #15859 which was for Windows only.

@jborean93
Copy link
Collaborator

jborean93 commented Nov 17, 2021

From the linked PR:

However, Windows doesn't support shebang, so we should be consistent with Windows PowerShell and only allow for scripts with .ps1 extension.

Personally I think pwsh should be consistent everywhere and we can ignore what WinPS did. Especially since WinPS was a bit more restrictive. I don't see a real reason why this would really be beneficial to security.

@SteveL-MSFT
Copy link
Member

pwsh -file foo would read the contents of foo and execute it as though you typed it into the console. This is needed on non-Windows for shebang to work correctly. However, on Windows, this could be a security concern as things like checking if a script is signed (and should be executed) only happens for .ps1 files.

I wonder on Windows if it might work to allow for pwsh -file foo, but instead of straight execution, we save it to a temporary .ps1 and then execute that script.

Would be good for Security WG to take a look at such a proposal.

@jborean93
Copy link
Collaborator

However, on Windows, this could be a security concern as things like checking if a script is signed (and should be executed) only happens for .ps1 files.

Why not check the signature all the time and not just with the extension? If PowerShell knows it is going to execute a file as a script then it should do the check regardless of whether an extension is present.

@anttiah
Copy link
Author

anttiah commented Nov 17, 2021

I wonder on Windows if it might work to allow for pwsh -file foo, but instead of straight execution, we save it to a temporary .ps1 and then execute that script.

Not sure about performance impact of that kind of workaround, but in reality it might actually fix this one as well: #9887. Only on Windows, though. I haven't tested that one recently but I'd guess that empty automatic variables is still an issue.

@PaulHigin
Copy link
Contributor

PaulHigin commented Nov 29, 2021

@WG-Security

Why not check the signature all the time and not just with the extension?

Windows signature checking is based on file extension, and this is something we cannot control.

The idea of copying a non-extension file to a file with an extension, has a number of problems. We may not have permissions to write to the current location. Writing the file to a different location may break the scripts behavior if it is intended to run in a specific location.

@PaulHigin PaulHigin added Needs-Investigation The behavior reported in the issue is unexpected and needs further investigation. and removed Breaking-Change breaking change that may affect users Needs-Triage The issue is new and needs to be triaged by a work group. labels Nov 29, 2021
@atruskie
Copy link

Upgrading PowerShell broke all our githooks. I'd reiterate that the change from #15859 is breaking and even though I've read the release notes I did not comprehend that this would be a side effect.

@atruskie
Copy link

I'll note too, we actually symlinked to our Powershell script:

New-Item -ItemType SymbolicLink -Force -Path ".git/hooks" -Name pre-commit -Value "../../bin/pre-commit.ps1" 

Which ironically does have a .ps1 extension and made the error message extra confusing (even if it was correct from PowerShell's perspective).

Our work around is to create a shim bash script which then invokes the PowerShell script:

$shim = @"
#!/usr/bin/env sh
pwsh -noprofile -F ./bin/pre-commit.ps1

"@
$shim -replace "`r`n", "`n" | Out-File ".git/hooks/pre-commit" -Encoding utf8NoBOM -NoNewline

@iSazonov
Copy link
Collaborator

Windows signature checking is based on file extension, and this is something we cannot control.

Hmm, sheband is Unix alternative to Windows file extension , so to speak. Interesting, could we port signature checking to Unix with supporting sheband?

@anttiah anttiah changed the title File name extesion requirement broke Git hooks File name extension requirement broke Git hooks Dec 22, 2021
@anttiah
Copy link
Author

anttiah commented Dec 23, 2021

Windows has many file types that can be, from the user's point-of-view, executed without extension. For example, .exe, .bat, .cmd and, as far as I know, every other file type listed in the PATHEXT environment variable. Feels counter-intuitive that it's perfectly ok to call exes, cmds, etc. without extension but not a PowerShell script even when the file is just a parameter of the main executable.

Preliminary tests show that Git seem to work fine with an exe as a hook file, so I might try to convert my scripts to executables. I'm by no means a prorgrammer and my first results with ps2exe module were buggy. Converting every hook is ugly and clumsy workaroud, but I'm running out of options. Our company IT keeps updating my PowerShell to the latest version automatically so downgrading won't help for long.

@gamagan
Copy link

gamagan commented Feb 23, 2022

Just ran into this problem ,too. On Windows 10, and just upgraded to PowerShell 7.2 -- this broke my pre-commit hooks. Not good.

@rkeithhill
Copy link
Collaborator

However, on Windows, this could be a security concern as things like checking if a script is signed (and should be executed) only happens for .ps1 files.

As a compromise, how about allowing non-extension files to execute when a signature check is not required?

I'm trying to think of ways a user could indicate that yeah, I get it's a security risk but I don't care. I want to allow extensionless scripts to execute. IMO this is kind of a big deal. You should be able to write Git commit hooks in PowerShell.

@iSazonov
Copy link
Collaborator

Why don't Git like file extensions?

@anttiah
Copy link
Author

anttiah commented Feb 24, 2022

Why don't Git like file extensions?

In Unix-like systems the piece of information that tells the operating system that file is executable is an attribute of the underlying file system – the execute permission. It has no relation what so ever with the actual file name. In case the file that should be executed happens to be script, like Bash, Python, PowerShell, Perl, etc., etc. , then a special notation starting with #! (called shebang) in the beginning of the file tells the OS how proceed with the file. So anything can be executable. This concept does not exist in Windows.

I have no actual knowledge about how Git was ported to Windows but just based on their web site and my experience, there is some kind of emulation layer that allows for Git developers to do things Unix-way, e.g without the need to care about extensions, and the emulation layer then does its best to implement those operations in the Windows world. This is clearly visible in the hooks as, even in Windows, they need to have that shebang notation but with pwsh as the executable[1].

Adding support for extensions would require curated and parametrized list of allowed extensions, and well defined search order for those in case there is multiple extensions with the same file name. It's a can of worms from the Unix perspective.

[1] Actually, the executable in the example is Git provided Windows port of env which will then launch pwsh with the original script file name as a parameter .

@jborean93
Copy link
Collaborator

That doesn't really help explain why the hook can't just have the .ps1 extension. It should still work through the shebang as all that has now changed is that the path passed into pwsh -File now has the extension allowing it to work.

While I agree that requiring the extension is wrong there's not much that can be done about that at this time without adding even more complexity and logic to the signature verification stage. If your git hooks can have the extension then that's definitely the easier of 2 options.

@anttiah
Copy link
Author

anttiah commented Feb 24, 2022

That doesn't really help explain why the hook can't just have the .ps1 extension. It should still work through the shebang as all that has now changed is that the path passed into pwsh -File now has the extension allowing it to work.

If I have files pre-commit.exe, pre-commit.ps1, pre-commit.ahk and pre-commit.js in my hooks directory, then which one Git will pick?

@jborean93
Copy link
Collaborator

Ah ok that's the part I was missing, the git hooks have to be under that name rather than it be configurable. Thanks for clarifying!

@gamagan
Copy link

gamagan commented Feb 24, 2022

Ah ok that's the part I was missing, the git hooks have to be under that name rather than it be configurable. Thanks for clarifying!

Git allows changing the hook's path using config core.hooksPath. Not sure why they don't allow also changing the actual hook names.

@anttiah
Copy link
Author

anttiah commented Feb 24, 2022

List of supported hooks is indeed hard-coded. It's defined here: https://git-scm.com/docs/githooks. Abstracting the steps in the work flow from the actual file names would definitely make things easier on the Windows side.

@iSazonov
Copy link
Collaborator

Why don't Git like file extensions?

In Unix-like systems the piece of information that tells the operating system that file is executable is an attribute of the underlying file system – the execute permission. It has no relation what so ever with the actual file name.

Hmm, so if the hook file name is qqqps1 the hook works but if we add dot in the name qqq.ps1 the hook stop working. Looks weird :-)

@gamagan
Copy link

gamagan commented Feb 24, 2022

Btw, @atruskie's workaround works well for me.

The code he posted looked a bit weird to me, at first, until I realized that was meant to be ran from the command line to create the shim. The resulting code that ends up in, say, the pre-commit hook would look like this:

#!/usr/bin/env sh
pwsh -noprofile -F ./path/to/pre-commit.ps1

@anttiah
Copy link
Author

anttiah commented Feb 24, 2022

Those kind of shims might work in limited set of cases where input is not needed, but some of the hooks get their input from stdin and are expected to return their output via stdout which are hard to relay from the shim to the actual PowerShell script. That's why this needs to be fixed in either Git (by parametrizing hook file names) or PowerShell (by allowing extensionless scripts).

I would like to see this getting fixed in PowerShell as nothing has changed in Git and hooks did work with pwsh versions 6.x - 7.1.5.

@jborean93
Copy link
Collaborator

jborean93 commented Aug 3, 2022

Windows signature checking is based on file extension, and this is something we cannot control.

@SteveL-MSFT @PaulHigin I've recently gone through this code and it seems like the .ps1 extension isn't strictly needed.

While yes pwsh uses an API which takes in the filename there is also an API that is already implemented where you can supply the contents of a file and specify the extension yourself. In the case where -File my_script_without_ext was used then PowerShell can read the file like normal and use that content with the API.

This is even possible through the Get-AuthenticodeSignature cmdlet today. You can change the path used to something that is already signed, this just uses Pester to demonstrate which is more likely to be present.

$psd1Content = Get-Module -Name Pester -ListAvailable |
    Sort-Object -Property Version -Descending |
    Select-Object -First 1 -Property @{N='PSPath'; E={$_.Path}} |
    Get-Content -Raw -AsByteStream

Get-AuthenticodeSignature -SourcePathOrExtension '.psd1' -Content $psd1Content

No need for temp file and pwsh has to read the contents anyway so it's not like it's doing extra work.

@PaulHigin PaulHigin added the Needs-Triage The issue is new and needs to be triaged by a work group. label Aug 3, 2022
@SteveL-MSFT
Copy link
Member

@jborean93 that's great news! So if we can ensure that on Windows the script is still validated, then we just need someone to submit a PR :)

@PaulHigin
Copy link
Contributor

@TravisEz13 I seem to remember that there is an issue with this. Maybe Travis can recall what it is (if any).

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 9, 2022

Perhaps it is how SRP/AppLocker and Defender APIs work.

@PaulHigin
Copy link
Contributor

@WG-Security
Even though we could implement this for normal script file execution, we don't have any control over AppLocker or WDAC file signature validation. So even if we enabled this for PowerShell execution policy, we cannot for AppLocker or WDAC policies, and we would have to continue blocking execution of script files without an extension for these policy scenarios.

@PaulHigin PaulHigin added WG-Engine core PowerShell engine, interpreter, and runtime and removed WG-Security security related areas such as JEA labels Oct 10, 2022
@PaulHigin
Copy link
Contributor

@WG-Engine from a security perspective, this can be done (enable for PowerShell execution policy, disable for other policies), but it seems wrong to have different behaviors.

@anttiah
Copy link
Author

anttiah commented Nov 6, 2022

If PowerShell script really needs to pass AppLocker and WDAC (understandable) and it works with extensions only, and Git is not going to change how hooks are called (which seems to be the case), then there is not too many options left. And the remaining ones are not pretty either.

One possible solution is to take advantage of the fact that in Windows an executable can be called without extension. So, my suggestion is to make pwsh.exe aware of its own file name and adjust its behaviour based on that name like this:
When pwsh.exe is renamed to something else and called without extension, it searches for and evaluates a .ps1 file with the same name as the executable and located in the same directory just as if pwsh.exe would have been called with that file name as a parameter.

For example, if a diretory C:\repo\.git\hooks contains files

  • prepare-commit-msg.ps1
  • prepare-commit-msg.exe (renamed pwsh.exe)

then a command C:\repo\.git\hooks\prepare-commit-msg (without extension) would make PowerShell to behave like the command would have been "C:\Program Files\PowerShell\7\pwsh.exe" C:\repo\.git\hooks\prepare-commit-msg.ps1, while passing rest of the parameters to the script and taking care of STDIN, STDOUT and STDERR as usual.

There is some pros and cons to this.

Pros

  • It should work correctly, even with AppLocker and WDAC applying to both EXEs and scripts.
  • Executable is the same, no changes needed to publisher or hash based policies, unless publisher policy require exact file name.
  • Hooks are not versioned content of the repository. They are local to the computer hosting the repo. Thus, hooks are not pushed or cloned with the rest of the files.

Cons

  • It's a hack.
  • Needs one copy of pwsh.exe for each Git hook.
  • PowerShell executable would be outside its default path, which might cause problems with finding DLLs and require changes to path based policies.
  • Parameters cannot be passed to pwsh.exe without some new TBD method (envs, maybe?)

@rkeithhill rkeithhill removed the Needs-Triage The issue is new and needs to be triaged by a work group. label Jul 24, 2023
@vexx32
Copy link
Collaborator

vexx32 commented Jul 25, 2023

WG-Engine reviewed this issue yesterday.

We agree with the premise of the issue: there doesn't appear to be a compelling reason to prevent PowerShell from executing files without a *.ps1 file extension.

We acknowledge that there are however security concerns here, especially with regard to how the initial implementation bypassed the safeguards initially present with usage of -File to invoke scripts. We believe a proper solution would be one that:

  1. Allows -File to be used to invoke scripts with unexpected file extensions, even files lacking an extension
  2. Does not bypass any file signature verification code paths, including Windows catalog signing

Our impression is that there shouldn't be a security issue with invoking extension-less scripts; if the script contains an embedded signature, it should be verified as per normal, and if it is catalog-signed (or using AppLocker etc), unless the signing/identification method somehow can indicate the script should be extensionless, the signature verification should fail as it would for any other unknown scripts.

It is likely there is work needed here to fully determine the scope of the underlying issue and more properly rework -File's script handling to allow this without bypassing signature verification code paths.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution-No Activity Issue has had no activity for 6 months or more label Jan 22, 2024
@anttiah
Copy link
Author

anttiah commented Jan 22, 2024

Bump. Any progress on this?

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Resolution-No Activity Issue has had no activity for 6 months or more label Jan 22, 2024
@hiyorijl
Copy link

Please make it extensionless, automation is needed 🥹🥹, any progress?

@SteveL-MSFT SteveL-MSFT added KeepOpen The bot will ignore these and not auto-close WG-NeedsReview Needs a review by the labeled Working Group labels Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
KeepOpen The bot will ignore these and not auto-close Needs-Investigation The behavior reported in the issue is unexpected and needs further investigation. WG-Engine core PowerShell engine, interpreter, and runtime WG-NeedsReview Needs a review by the labeled Working Group
Projects
None yet
Development

No branches or pull requests