Skip to content

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

Closed
wants to merge 2 commits into from
Closed

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 4, 2024

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/

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.

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.
@cmb69
Copy link
Member Author

cmb69 commented Dec 4, 2024

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 is_executable(); the source code of the latter is in a Gist; need to verify if this is the actual source code). Anyhow, I think it's a bad idea to ship these Windows binaries; instead only the sources should be committed to the repo, and everything should be build on the fly (particularly relevant if we ever support ARM64 on Windows). We would need to figure out a clean way to do so; maybe that stuff should be moved to ext/zend_test. Suggestions are welcome!

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/>
@cmb69
Copy link
Member Author

cmb69 commented Dec 4, 2024

Without the patch, the cmd.exe in the current working directory is used, but no longer with the patch applied.

@cmb69 cmb69 marked this pull request as ready for review December 4, 2024 16:44
@cmb69 cmb69 requested a review from bukka as a code owner December 4, 2024 16:44
Copy link
Member

@bukka bukka left a 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.

@cmb69 cmb69 closed this in 5cbdd5f Dec 8, 2024
@cmb69
Copy link
Member Author

cmb69 commented Dec 8, 2024

Added the comment right away when applying.

@cmb69 cmb69 deleted the cmb/cmd.exe-hijacking branch December 8, 2024 18:12
charmitro pushed a commit to wasix-org/php that referenced this pull request Mar 13, 2025
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants