Able to inject arbitrary commands when using "Run external program on torrent completion"

[micapucha]

Please provide the following information

qBittorrent version and Operating System

4.1.5 FreeBSD, but seeing the source code, probably all versions

What is the problem

The function Application::runExternalProgram() located in qBittorrent/src/app/application.cpp does not sanitize the name of the torrent and other parameters before passing them as a command line arguments. Right now it does a simple text substitution, which is vulnerable to command injection.

What is the expected behavior

The parameters are sanitized before being passed to the command line. This means something like PHP's escapeshellarg.

Steps to reproduce

1. Configure qBitTorrent to run an external script in Preferences - Downloads. Anything is valid. You can even quote the arguments. For example:
   /home/user/notify.sh "%N" "%L" %C %Z

2. Create a new torrent with the following command:
   mktorrent -n 'Name"; firefox ; "' -o exploit.torrent SourceDir
   To test it locally with already created content, without needing to upload your torrent to a public tracker, create a directory named 'Name"; firefox ; "' (without the first and last quotes) that has the same values as the previous command's SourceDir. This will re-hash the content and mark it as completed, executing the script. You can use midnight commander to do this.

3. When the torrent download finishes, Firefox is opened

Greetings

[micapucha]

This exploit can be triggered without user interaction if we're receiving a feed from RSS. The user won't notice the fishy name until it's too late.

[Chocobo1]

Ping @Piccirello
You might be interested in this.
Oops, maybe not, I thought it was related to WebAPI on the first sight.

[Piccirello]

Thanks for pinging me, this is really interesting. Especially with the RSS feed attack vector. We currently take precautions to prevent things like XSS from torrent names, so I would consider this issue as credible.

[Piccirello]

Some quick reading on escapeshellarg and other implementations indicates that wrapping each parameter in single quotes (and escaping any single quotes within the parameter) will prevent it from being interpreted by the shell. I think we should wrap each value in single quotes before making it available to the external script. Does anyone see a downside/disagree with this approach?

Also pinging @glassez @sledgehammer999 in case they're interested

[Chocobo1]

I think we should wrap each value in single quotes before making it available to the external script. Does anyone see a downside/disagree with this approach?

Seeing the available runExternalProgram parameters, we only need to sanitize the torrent name parameter and the current tracker parameter.

[micapucha]

Hi @Piccirello thanks for looking at this issue. Other way of injecting a command could involve using an escape character like \ at the end of the name, so a parameter like "name" would become "name\". This means the last quote is ignored.

Try the following example in bash:
echo "a\" ";firefox;\"

This will execute firefox.

Greetings

[micapucha]

Another method, which will do the trick on Unix systems (/bin/sh is called from QProcess::startDetached on line 339). I haven't tested it but it will probably work:

echo "Torrent name$(firefox)"

More information: https://portswigger.net/web-security/os-command-injection

Banned characters should at least be $, ", ', ` and newlines. This is independent of quoting the arguments, which should also be done.

Greetings

[Piccirello]

This means the last quote is ignored.

This is not the case when using single quotes, as I understand it. Executing echo 'a\" ";firefox;\' results in the text being echod.

Seeing the available runExternalProgram parameters, we only need to sanitize the torrent name parameter and the current tracker parameter.

I agree with this, with one caveat- I think we may want to quote the category name. It's specified by the user, but could have side effects if the user uses special characters. Categories must match the regex static const QRegularExpression re(R"(^([^\\\/]|[^\\\/]([^\\\/]|\/(?=[^\/]))*[^\\\/])$)"), but I'm having a difficult time interpreting that.

Banned characters should at least be $, ", ', ` and newlines

I'm really not sure we have to do any of this if we use single quotes.

[micapucha]

Ok, didn't really notice the comment about single quotes. :)
It doesn't execute Firefox now

[micapucha]

@Piccirello Try with this one, only using single quotes gets you command execution:
echo 'Name\' '\';firefox;\'
This requires controlling two parameters, could it be achieved with torrent name and tracker or file name?

EDIT: Optimized to just one parameter
echo 'Name\';firefox;\'

[Piccirello]

There's command execution because the third single quote isn't escaped. For your optimized example, the string should end with an unescaped single quote.

[Chocobo1]

I agree with this, with one caveat- I think we may want to quote the category name. It's specified by the user, but could have side effects if the user uses special characters.

Sounds reasonable.

[micapucha]

Ok imagine the following example: I (the attacker) create a torrent with the following name: Name\';firefox;\

Then you add single quotes around my string and escape the quotes that were inside. The parameter sent to the system is this one:
echo 'Name\\';firefox;\'

Firefox runs

The escape character is very tricky and shouldn't be in a legit torrent name, it could be removed or replaced with a space.

[Piccirello]

Good examples. Maybe this will be a little trickier than it first seemed.

The escape character is very tricky and shouldn't be in a legit torrent name

I'm not sure if we can make this generalization. It would make things easier but may not be true.

I wonder if we can get any security guarantees by passing the entire command, split on spaces, as an arguments list to QProcess::startDetached.

[micapucha]

Good afternoon, I contacted the CVE database so downstream Linux distributions acknowledge the vulnerability and backport and apply the patch. It's available at: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-13640

This is also their response, will come in handy to fix the quote escaping:

The following might be useful in fixing the vulnerability:

https://www.openwall.com/lists/oss-security/2014/02/04/3

The proper way (at least if your shell runs in a UTF-8 or ISO-8859
locale) to escape shell arguments is to wrap them in '', after replacing
embedded ' characters with the four character sequence '\''.

Greetings

[Chocobo1]

@micapucha
Are you able to compile qbittorrent? It would be great if you can help verify the fix:

Replace this line https://github.com/qbittorrent/qBittorrent/blob/master/src/app/application.cpp#L338
with QProcess::startDetached(program);.

[micapucha]

Hi @Chocobo1

I compiled qbittorrent with the patched line, and can no longer trigger the command execution. I have tested 10 rigged .torrents and swapping between ", ', and no quotes in the preferences and shellcode, and and also using $() in the shellcode.

In any case, I think the absence of code execution is because you're no longer calling /bin/sh and passing the command as argument. Because of this, the shellcode is not being interpreted. However, the user must put the parameters themselves between quotes to work correctly (eg. a torrent named "Don Quixote" will pass "Don" as the first parameter and "Quixote" as the second one). I don't fully grasp the security implications on this, and if this could achieve execution in other platforms (eg. Windows)

Tested on: Debian 10, Qbittorrent 4.1.5 from debian sources patched manually

EDIT: Find attached the test files, if anybody wants to continue testing it. TestSuite.zip

Greetings

[Piccirello]

@micapucha Thanks for taking care of the CVE portion. I always love me a good CVE 😈

I’ll take another look at this tonight if time allows, or tomorrow at the latest.

[micapucha]

@Piccirello Thank you for your time and developing this amazing app :)

Find attached the test suite, it's useful if anybody wants to continue further testing

TestSuite.zip

[Chocobo1]

I compiled qbittorrent with the patched line, and can no longer trigger the command execution.

Hmm... sounds like good news!

In any case, I think the absence of code execution is because you're no longer calling /bin/sh and passing the command as argument. Because of this, the shellcode is not being interpreted. ...

Yes, that is correct. I thought it was convenience to be able to invoke shell env/commands but didn't consider from security viewpoint. Also that patch isn't new, it is actually the old form (df95efe).

I don't fully grasp the security implications on this, and if this could achieve execution in other platforms (eg. Windows)

That code path is only for non-windows system.

[Chocobo1]

@micapucha
Thanks for reporting it! Will be fixed in next release v4.1.7/v4.2.0.

[micapucha]

Thank you for fixing the issue :)
Greetings