Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#10677 closed Patch - Bug Fix (Won't Fix)

Safely escape shell arguments

Reported by: github@… Owned by:
Priority: minor Milestone: unknown
Component: MythTV - General Version: Master Head
Severity: medium Keywords:
Cc: Ticket locked: no

Description

The existing ShellEscape? method(s) are not consistent and not thorough. This implementation will handle spaces, single quotes, double quotes, newlines, and all other shell metacharacters.

An attempt was made to fix this in changeset f4dbb71f4e974bc388d4c6f110e6936de9cf490b, (issue #9913), but it didn't escape most characters. For example, it didn't address newlines or pipes. It also didn't work with arguments that contained both a space and a quote.

The patch is in GitHub?: https://github.com/MythTV/mythtv/pull/18

Change History (6)

comment:1 Changed 7 years ago by beirdo

Why would anyone ever need newlines or pipes in command line arguments?

comment:2 Changed 7 years ago by github@…

This patch obviously doesn't require anyone to use newlines or pipes in their filenames. :) It also doesn't contain special code for those characters. I was just making the point that it handles all metacharacters.

The simplicity of this implementation should make it safer, because it is easier to audit. The existing ShellEscape function is 13 lines instead of the 1 I submitted. The existing function handles a file named "Takin'" but not one named "Sweet Talkin'". The inconsistency of the existing way just looked unattractive to me.

comment:3 Changed 7 years ago by Raymond Wagner

Milestone: unknown
Resolution: Won't Fix
Status: newclosed

I'm going to close this one as better shell escaping implies running commands through a shell. Better to do internal expansion of the commands and leave external shells completely out of the equation. The MythSystem? class supports this type of operation when the user supplies the command as a QStringList, but does not yet have a lexical parser if the user supplies it as a QString. It's something on my list of TODOs, but is going to require a bit of additional work if it is going to support the setting of environmental variables, IO piping, and other command line syntax needed to be handled if it is to be a drop-in replacement for cases where the user provides a full command string that is run unmodified.

comment:4 Changed 7 years ago by github@…

IIUC, you are suggesting that clients should not worry about shell metacharacters because MythSystem is responsible to do the escaping. So that means that if this issue is not going to be fixed, then issue #10680 ought to be addressed. The current situation is insecure, because command-line arguments are not getting escaped correctly.

comment:5 Changed 7 years ago by Raymond Wagner

As explained already, "shell escaping" is exactly as it sounds, escaping terms that would otherwise be handled improperly by a shell interpreter. You get rid of the shell interpreter, pass the arguments directly into the application yourself, and there is nothing left to do those "bad things" you are suggesting. In cases where the MythSystem?() user supplies the arguments with a QStringList, and the kMSNoRunShell flag, this is precisely what happens. The MythSystem? class bypasses the shell interpreter, and calls the application directly with an execv() system call.

What I am suggesting is that instead of bothering with escaping anything, just perform our own internal argument splitting to handle all the remaining cases, and bypass the issue entirely.

comment:6 Changed 7 years ago by github@…

The missing piece here seems to be the kMSNoRunShell flag. That would fix the issue so that clients can use MythSystem safely.

That flag is not documented, (see https://www.google.com/search?q=MythSystem+kMSNoRunShell). Of 63 uses of MythSystem in the code, that flag is not used anywhere except for in MetadataDownload.

I could be wrong, but I got the feeling that you are confident that the system's current behavior is correct. So, I'm just curious, if this issue and issue #10680 are going to be closed, then which ticket tracks the adoption of kMSNoRunShell in the 26 other files that use it? On the other hand, if you decide that the shell escaping bugs ought to be addressed, then let's reopen this ticket and start making patches.

Note: See TracTickets for help on using tickets.