-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Harden against cmd.exe hijacking #17043
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
Conversation
To be able to easily test this, we provide a minimalist C file which will be build as test_helper, and used by the new test case.
Building of bad_cmd.exe is pretty hackish as is. It seems so far we have no support for such test helper binaries at all. There are already ext\standard\tests\file\windows_acls\tiny.exe and ext\ffi\tests\callconv_x86.dll, which have been committed as binaries without source code (the source code of the former doesn't matter since it is only used to verify |
As is, whenever `proc_open()` needs to invoke the shell, cmd.exe is looked up in the usual executable search path. That implies that any cmd.exe which is placed in the current working directory (which is not necessarily what is reported by `getcwd()` for ZTS builds), will be used. This is a known attack vector, and Microsoft recommends to always use the fully qualified path to cmd.exe. To prevent any cmd.exe in the current working directory to be used, but to still allow users to use a drop in replacement for cmd.exe, we search only the `PATH` for cmd.exe (and pass the fully qualified path to `CreateProcessW`), instead of relying on automatic executable search by passing the base name only. [1] <https://msrc.microsoft.com/blog/2014/04/ms14-019-fixing-a-binary-hijacking-via-cmd-or-bat-file/>
Without the patch, the cmd.exe in the current working directory is used, but no longer with the patch applied. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. Maybe just that comment suggestion would be useful.
Added the comment right away when applying. |
As is, whenever `proc_open()` needs to invoke the shell, cmd.exe is looked up in the usual executable search path. That implies that any cmd.exe which is placed in the current working directory (which is not necessarily what is reported by `getcwd()` for ZTS builds), will be used. This is a known attack vector, and Microsoft recommends to always use the fully qualified path to cmd.exe. To prevent any cmd.exe in the current working directory to be used, but to still allow users to use a drop in replacement for cmd.exe, we search only the `PATH` for cmd.exe (and pass the fully qualified path to `CreateProcessW`), instead of relying on automatic executable search by passing the base name only. To be able to easily test this, we provide a minimalist C file which will be build as test_helper, and used by the new test case. [1] <https://msrc.microsoft.com/blog/2014/04/ms14-019-fixing-a-binary-hijacking-via-cmd-or-bat-file/> Closes phpGH-17043.
As is, whenever
proc_open()
needs to invoke the shell, cmd.exe islooked up in the usual executable search path. That implies that any
cmd.exe which is placed in the current working directory (which is not
necessarily what is reported by
getcwd()
for ZTS builds), will beused. This is a known attack vector, and Microsoft recommends to
always use the fully qualified path to cmd.exe.
To prevent any cmd.exe in the current working directory to be used, but
to still allow users to use a drop in replacement for cmd.exe, we
search only the
PATH
for cmd.exe (and pass the fully qualified pathto
CreateProcessW
), instead of relying on automatic executable searchby passing the base name only.
[1] https://msrc.microsoft.com/blog/2014/04/ms14-019-fixing-a-binary-hijacking-via-cmd-or-bat-file/
To be able to easily test this, we provide a minimalist C file which will be build as test_helper, and used by the new test case.