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

prevent arbitrary command execution via URL when using --defaultURLHandler #1003

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

thomasgl-orange
Copy link
Contributor

I've just seen changes in 1.3.16, and I think #996 points to a security flaw (which was there already, just happened to see it because of this change). In this version, when using --defaultURLHandler favorite-browser, opening an URL will execute a shell process (child_process.exec(...)) to run the following command:

favorite-browser "THE_URL"

Although THE_URL has some URL-encoded characters, this still leaves plenty of potential for arbitrary command execution via a malicious URL in a Teams message. In particular, the following characters are not encoded in the path of an URL: $, (, ).

Here is a way to exploit that: https://google.com/$(sudo$IFS$(true)shutdown$IFS$(true)-h$IFS$(true)now)

  • $IFS is substituted with some space characters
  • $(true) is substituted with empty string, it's used here to separate $IFS and whatever follows
  • sudo shutdown -h now is the resulting command...
  • $(...) executes this command in a sub-shell, and is substituted with its standard output (which is empty here)
  • favorite-browser will simply open https://google.com/ (while the computer starts shutting down if sudo expects no password)

In this PR, I propose to use child_process.execFile(...) as a safer replacement for exec(...). No shell involved, just a command name (favorite-browser) and its unaltered string argument (the URL string, as-is).

The drawback is that maybe someone somewhere has been successfully using something like --defaultURLHandler "favorite-browser --some-browser-option", and this will not work anymore. IMHO, it's an acceptable trade-off. If you think it's not, then some alternative fixes could be:

  • with execFile(...): split defaultURLHandler, use first word as command and remaining ones as additional arguments before the URL
  • with exec(...): properly shell-escape the details.url string in the command (I think escaping $ as \$, plus the double-quotes which has already been added around the string, might be enough, thanks to the limited set of chars we get in URLs, but not 100% sure)

@jijojosephk
Copy link
Collaborator

Thanks @thomasgl-orange 👍 You're a star 🏅

@jijojosephk jijojosephk merged commit 6d08b83 into IsmaelMartinez:develop Oct 31, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants