Menu ▾ ▴

#8 Make shell safe all filenames used in system() calls

PACPL
closed
Philip Lyons
shell system quote (1)
5
2018-07-22
2018-06-05
Paulo Marcel Coelho Aragão
No

I'veI'veI'veI've just come across a filename with an embedded backtick (`), and the decoding failed. Investigating the code, I discovered that convert encloses in double quotes all filenames which will be used in system calls and only escapes characters \ and $, so the system calls will fail if the filenames have embedded ` and ". Investigating a little further, there were other system calls which used filenames without any quoting at all.

You can reproduce this very easily: get any audio file, let's say a FLAC file, rename it to foo`"bar.flac and try to convert it. The conversion will fail. I'm not so sure how to provoke the failure in the other system calls, but surely they will fail with filenames which have not only embedded ` and ", but also \ and $, because they're not being quoted at all.

After reading this enlightening article, it seemed to me safer and cleaner to use method shell_quote from module String::ShellQuote, which in Ubuntu comes in package libstring-shellquote-perl. All system calls were carefully revised, and the double-quoted filenames used were all replaced by the result of shell_quote filename.

I've attached a patch.

1 Attachments
shell-escape-all-filenames.patch

Discussion

  • Philip Lyons

    Philip Lyons - 2018-06-09
    • Description has changed:

    Diff:

    --- old
+++ new
@@ -1,4 +1,4 @@
-I've just come across a filename with an embedded backtick (**\`**), and the decoding failed. Investigating the code, I discovered that `convert` encloses in double quotes all filenames which will be used in `system` calls  and only escapes characters **\\** and **$**, so the `system` calls will fail if the filenames have embedded **\`** and **"**. Investigating a little further, there were other `system` calls which used filenames without any quoting at all.
+I'veI'veI'veI've just come across a filename with an embedded backtick (**\`**), and the decoding failed. Investigating the code, I discovered that `convert` encloses in double quotes all filenames which will be used in `system` calls  and only escapes characters **\\** and **$**, so the `system` calls will fail if the filenames have embedded **\`** and **"**. Investigating a little further, there were other `system` calls which used filenames without any quoting at all.

 You can reproduce this very easily: get any audio file, let's say a FLAC file, rename it to ***foo\`"bar.flac*** and try to convert it. The conversion will fail. I'm not so sure how to provoke the failure in the other system calls, but surely they will fail with filenames which have not only embedded **\`** and **"**, but also **\\** and **$**, because they're not being quoted at all.
    • status: open --> pending
    • assigned_to: Philip Lyons
     

  • Philip Lyons

    Philip Lyons - 2018-06-09

    I'm currently away from the keyboard but I'll get them done as soon as I can thanks for the inputimimim

     

  • Philip Lyons

    Philip Lyons - 2018-06-17
    • status: pending --> accepted
     

  • Philip Lyons

    Philip Lyons - 2018-07-22
    • status: accepted --> closed
     

Log in to post a comment.