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

escapeshellarg() dangerous on windows (always assumes bash escape rules) #395

Closed
1 task
divinity76 opened this issue Aug 25, 2019 · 5 comments
Closed
1 task

Comments

@divinity76
Copy link
Contributor

divinity76 commented Aug 25, 2019

  • Have you checked the guidelines in our Contributing document?

Description

Windows/cmd has quite different escaping rules from bash, and the current escapeshellarg() function always assumes it's running on bash, even if it's running on windows. this could allow hackers to execute programs on command-line-arguments created from user input. for example:

user_input="test.txt & dir &;";
cmd="notepad "+escapeshellarg(user_input);
  • here the programmer have correctly used escapeshellarg() to create the cmd argument in a safe way, where the user decide what file to open with notepad, but nothing more. but because of the bug, even on MS Windows the command becomes:

notepad 'test.txt & dir &;'

which to microsoft's cmd.exe means roughly: open notepad with the argument test.txt and run the command dir simultaneously

.. escapeshellarg() needs to check if it's running on Windows or not, and then use OS-specific escape rules, at least that's what PHP's escapeshellarg() does.

@oskarlh
Copy link
Contributor

oskarlh commented Oct 23, 2019

I think this function should just be removed. I mean, what's the correct behaviour on the Web, which has no shell?

@oskarlh
Copy link
Contributor

oskarlh commented Oct 23, 2019

And what about other operating systems that aren't Windows or UNIX? Just throw an error?

@divinity76
Copy link
Contributor Author

well the php behavior is to check if it's running on Windows, if yes use cmd escape-rules, otherwise use POSIX escape rules..

not sure if we should follow suite or delete the function. anyhow, you can apparently check if you're running on Windows or not by running

if((typeof window === "undefined" ? process.platform : window.navigator.platform).toString().indexOf("Win") === 0 ){
// running on Windows
} else {
// Not windows
}

divinity76 added a commit to divinity76/locutus that referenced this issue Sep 22, 2020
kvz pushed a commit that referenced this issue Oct 2, 2020
Copy link

github-actions bot commented Apr 5, 2024

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@kvz
Copy link
Collaborator

kvz commented Apr 5, 2024

Thanks for reporting, I fixed this in the associated commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants