Filenames containing some shell metacharacters are not handled correctly in some Tasks or Console commands

[prl]

In particular, deletion of some files and recordings in the Movie Player and in the File Commander fails if the file names contain '$', '"' or '\'.

The failure in the Movie Player of "move to Trash" is due to the construction of the commandline in Tools.CopyFiles.MoveFileJob.init(). The command is constructed as

'mv -f "%s" "%s"' % (srcfile,destfile)

when a large (>100MB) file is moved to Trash.

So if a filename contains a '$' character, when the shell executes the command, it will try to do variable substitution, and wiill most likely change the filename from the actual filename to be operated on, and it will fail.

Similarly, deletion in the File Commander builds the shell command

cmdlist=["rm \"" + sourceDir + filename + "\""]

that is directly executed by Console. If the constructed path contains any of the sensitive shell metacharacters, it will also fail.

This problem is likely to affect any commands executed by passing a string to a subshell if the string can contain any shell metacharacters that have not been properly escaped.

Reproduction steps

In a T3 that uses the EIT EPG, not the IceTV EPG, create a recording of "The House That ₤100K Built" It will be shown in the EPG as "The House That $100K Built".

When the recording has completed, go to the Movie Player (MEDIA from live TV), select the recording of "The House That $100K Built", and press RED Delete. The Movie Player will show the recording as having been deleted. But if you then EXIT the Movie Player and re-enter it, the recording will still be visible in the Movie Player.

If the debug log is enabled, the log will show both the 'mv' command as padded to the shell, and the error message from 'mv' when it fails to delete the name that has been altered by unwanted variable substitution.

If you then go to the File Commander (e.g. MENU>Sources>T3), navigate to the folder containing the recording, the recording's .ts file will still be there (but because its metadata files are normally handled by a different mechanism, they will have been moved to .Trash).

If you select the recording's .ts file and press RED Delete, the deletion will also fail, but in this case an error screen can be seen, and it will show the filename as modified by the unwanted variable substitution.

[IanSav]

This could quickly and easily be fixed by using the appropriate single quote to stop the shell substitutions. Remember to protect any single quotes in the string.

[prl]

If only it were so simple. An ASCII single quote (or one used as an apostrophe) in a filename would break that. And a filename might contain both a $ and a single quote.

There should be design patterns (or even code) around that will quote a string properly. There certainly are for Perl. An alternative would be to use Python code that ultimately uses execvp(2) rather than system(3).

This is an issue I've visited before. It really needs to be done exactly right.

And applied everywhere it's needed.

[prl]

"An alternative would be to use Python code that ultimately uses execvp(2) rather than system(3)."

The enigma2 Task mechanism allows for that (but the CopyFileJob and MoveFileJob classes don't use it). The enigma2 Console class always uses system().

[IanSav]

Seems simple to me. Just prior to passing the string to a shell convert any single quotes in the sequence to single quote, double quote, single quote, double quote, single quote and then enclose the entire resultant string within single quotes. The single quotes will be protected and shell variables will not be expanded by the shell.

That is, a quoted string like "The House That's $100K Built" (modified from above to include a single quote) could be rewritten as 'The House That'"'"'s $100K Built'.

[prl]

' -> '\'' is more compact. But I still haven't satisfied myself that it's sufficient. I've read hints that it might not be, but they weren't explicit enough for me to decide whether there is a potential problem. Using execvp() rather than system() is still my preference.

[prl]

Getting away from the topic of how this might be fixed to why a fix is needed fairly urgently: with the code as it is, a suitably malevolently named recording or media file could cause any desired T3 commandline command to be executed if you try to delete it through the Movie Player or FileCommander, or do any of the other operations on it that are run via a system() library call. I can PM details if needed.

[prl]

I've gone back to the web post that said that the ' -> '\'' quoting wasn't sufficient, and on a closer reading, it only says that it's not sufficient if it's preceded by an un-quoted string from an uncontrolled source. But that's something to be avoided anyway, because such a string is unsafe in its own right.

I.e. os.system("constant string" + user_str1 + shell_quoted(user_str2)) is unsafe, because if user_str1 contains an unbalanced quote (' or ") or ends with \, it can cause problems with the quoting of user_str2. But user_str1 can be problematic anyway, independent of whether it interferes with the quoting of user_str2 or not.

[Peter Urbanec]

I think this was fixed at some stage in the past, right?

[prl]

I think that it's fixed in most places, but I didn't get right through everything.