Mailing List Archive: MythTV: Commits

Ticket #10677: Safely escape shell arguments

Index | Next | Previous | View Threaded

noreply at mythtv

Apr 30, 2012, 10:12 PM

Post #1 of 7 (98 views)
Permalink

Ticket #10677: Safely escape shell arguments

#10677: Safely escape shell arguments
-------------------------+----------------------------------
Reporter: github@… | Type: Patch - Bug Fix
Status: new | Priority: minor
Milestone: | Component: MythTV - General
Version: Master Head | Severity: medium
Keywords: | Ticket locked: 0
-------------------------+----------------------------------
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

--
Ticket URL: <http://code.mythtv.org/trac/ticket/10677>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center
_______________________________________________
mythtv-commits mailing list
mythtv-commits [at] mythtv
http://www.mythtv.org/mailman/listinfo/mythtv-commits

noreply at mythtv

Apr 30, 2012, 10:31 PM

Post #2 of 7 (92 views)
Permalink

Re: Ticket #10677: Safely escape shell arguments [In reply to]

noreply at mythtv

Apr 30, 2012, 11:07 PM

Post #3 of 7 (94 views)
Permalink

Re: Ticket #10677: Safely escape shell arguments [In reply to]

#10677: Safely escape shell arguments
------------------------------+-----------------------------
Reporter: github@… | Owner:
Type: Patch - Bug Fix | Status: new
Priority: minor | Milestone:
Component: MythTV - General | Version: Master Head
Severity: medium | Resolution:
Keywords: | Ticket locked: 0
------------------------------+-----------------------------

Comment (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.

--
Ticket URL: <http://code.mythtv.org/trac/ticket/10677#comment:2>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center
_______________________________________________
mythtv-commits mailing list
mythtv-commits [at] mythtv
http://www.mythtv.org/mailman/listinfo/mythtv-commits

noreply at mythtv

May 1, 2012, 11:11 AM

Post #4 of 7 (85 views)
Permalink

Re: Ticket #10677: Safely escape shell arguments [In reply to]

#10677: Safely escape shell arguments
------------------------------+-----------------------------
Reporter: github@… | Owner:
Type: Patch - Bug Fix | Status: closed
Priority: minor | Milestone: unknown
Component: MythTV - General | Version: Master Head
Severity: medium | Resolution: Won't Fix
Keywords: | Ticket locked: 0
------------------------------+-----------------------------
Changes (by wagnerrp):

* status: new => closed
* resolution: => Won't Fix
* milestone: => unknown

Comment:

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.

--
Ticket URL: <http://code.mythtv.org/trac/ticket/10677#comment:3>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center
_______________________________________________
mythtv-commits mailing list
mythtv-commits [at] mythtv
http://www.mythtv.org/mailman/listinfo/mythtv-commits

noreply at mythtv

May 1, 2012, 9:59 PM

Post #5 of 7 (81 views)
Permalink

Re: Ticket #10677: Safely escape shell arguments [In reply to]

noreply at mythtv

May 1, 2012, 10:14 PM

Post #6 of 7 (82 views)
Permalink

Re: Ticket #10677: Safely escape shell arguments [In reply to]

#10677: Safely escape shell arguments
------------------------------+-----------------------------
Reporter: github@… | Owner:
Type: Patch - Bug Fix | Status: closed
Priority: minor | Milestone: unknown
Component: MythTV - General | Version: Master Head
Severity: medium | Resolution: Won't Fix
Keywords: | Ticket locked: 0
------------------------------+-----------------------------

Comment (by wagnerrp):

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.

--
Ticket URL: <http://code.mythtv.org/trac/ticket/10677#comment:5>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center
_______________________________________________
mythtv-commits mailing list
mythtv-commits [at] mythtv
http://www.mythtv.org/mailman/listinfo/mythtv-commits

noreply at mythtv

May 1, 2012, 10:29 PM

Post #7 of 7 (79 views)
Permalink

Re: Ticket #10677: Safely escape shell arguments [In reply to]

#10677: Safely escape shell arguments
------------------------------+-----------------------------
Reporter: github@… | Owner:
Type: Patch - Bug Fix | Status: closed
Priority: minor | Milestone: unknown
Component: MythTV - General | Version: Master Head
Severity: medium | Resolution: Won't Fix
Keywords: | Ticket locked: 0
------------------------------+-----------------------------

Comment (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.

--
Ticket URL: <http://code.mythtv.org/trac/ticket/10677#comment:6>
MythTV <http://code.mythtv.org/trac>
MythTV Media Center
_______________________________________________
mythtv-commits mailing list
mythtv-commits [at] mythtv
http://www.mythtv.org/mailman/listinfo/mythtv-commits

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads