[1.7.6] [patch] Shell commands should be escaped properly

Locked
rasher
User
Posts: 3
Joined: Tue Dec 27, 2005 4:02 pm

[1.7.6] [patch] Shell commands should be escaped properly

Post by rasher » Tue Mar 21, 2006 12:48 am

In hash_data() in the non-windows case, the filename is passed in doublequotes without any additional escaping. This can confuse the shell for some filenames - for example names with dollar-signs in them, in which case the shell will attempt to expand that variable and the hashing will fail. Instead shellescapearg() and no double-quotes should be used (the function will encase the argument in single-quotes and escape any special characters).

This is where I ran into the bug, but I've tried finding other places where external programs are called and no escaping is done. I have not changed anything in the "Windows" cases, as I don't know if escapeshellarg() works, or is even available on Windows. I have not tested any of the other cases except the one in hash_data() though, since I don't have files to test with at hand.

Patch for this problem (if that doesn't work out, just tell me where to mail it):

Code: Select all

diff -ru getid3-1.7.6.orig/getid3/getid3.lib.php getid3-1.7.6/getid3/getid3.lib.php
--- getid3-1.7.6.orig/getid3/getid3.lib.php 2006-03-21 01:22:21.000000000 +0100
+++ getid3-1.7.6/getid3/getid3.lib.php  2006-03-21 01:30:31.000000000 +0100
@@ -566,7 +566,7 @@

        } else {

-           $commandline = 'sha1sum "'.$file.'"';
+           $commandline = 'sha1sum '.escapeshellarg($file);
            if (ereg("^([0-9a-f]{40})[ \t\n\r]", strtolower(`$commandline`), $r)) {
                return $r[1];
            }
@@ -623,7 +623,7 @@

            } else {

-               $commandline  = 'head -c '.$end.' "'.$file.'" | ';
+               $commandline  = 'head -c '.$end.' '.escapeshellarg($file).' | ';
                $commandline .= 'tail -c '.$size.' | ';
                $commandline .= $unix_call;

diff -ru getid3-1.7.6.orig/getid3/getid3.php getid3-1.7.6/getid3/getid3.php
--- getid3-1.7.6.orig/getid3/getid3.php 2006-03-21 01:42:19.000000000 +0100
+++ getid3-1.7.6/getid3/getid3.php  2006-03-21 01:36:48.000000000 +0100
@@ -1086,7 +1086,7 @@

                } else {

-                   $commandline = 'vorbiscomment -w -c "'.$empty.'" "'.$file.'" "'.$temp.'" 2>&1';
+                   $commandline = 'vorbiscomment -w -c "'.$empty.'" '.escapeshellarg($file).' '.escapeshellarg($temp).' 2>&1';
                    $VorbisCommentError = `$commandline`;

                }
diff -ru getid3-1.7.6.orig/getid3/module.audio.shorten.php getid3-1.7.6/getid3/module.audio.shorten.php
--- getid3-1.7.6.orig/getid3/module.audio.shorten.php   2006-03-21 01:43:01.000000000 +0100
+++ getid3-1.7.6/getid3/module.audio.shorten.php    2006-03-21 01:31:05.000000000 +0100
@@ -135,7 +135,7 @@
                 $ThisFileInfo['error'][] = 'shorten binary was not found in path or /usr/local/bin';
                 return false;
             }
-            $commandline = (file_exists('/usr/local/bin/shorten') ? '/usr/local/bin/' : '' ) . 'shorten -x "'.$ThisFileInfo['filenamepath'].'" - | head -c 44';
+            $commandline = (file_exists('/usr/local/bin/shorten') ? '/usr/local/bin/' : '' ) . 'shorten -x '.escapeshellarg($ThisFileInfo['filenamepath']).' - | head -c 44';

        }

diff -ru getid3-1.7.6.orig/getid3/write.metaflac.php getid3-1.7.6/getid3/write.metaflac.php
--- getid3-1.7.6.orig/getid3/write.metaflac.php 2006-03-21 01:43:12.000000000 +0100
+++ getid3-1.7.6/getid3/write.metaflac.php  2006-03-21 01:34:16.000000000 +0100
@@ -80,7 +80,7 @@
            } else {

                // It's simpler on *nix
-               $commandline = 'metaflac --no-utf8-convert --remove-vc-all --import-vc-from='.$tempcommentsfilename.' "'.$this->filename.'" 2>&1';
+               $commandline = 'metaflac --no-utf8-convert --remove-vc-all --import-vc-from='.escapeshellarg($tempcommentsfilename).' '.escapeshellarg($this->filename).' 2>&1';
                $metaflacError = `$commandline`;

            }
@@ -132,7 +132,7 @@
            } else {

                // It's simpler on *nix
-               $commandline = 'metaflac --remove-vc-all "'.$this->filename.'" 2>&1';
+               $commandline = 'metaflac --remove-vc-all '.escapeshellarg($this->filename).'" 2>&1';
                $metaflacError = `$commandline`;

            }
diff -ru getid3-1.7.6.orig/getid3/write.vorbiscomment.php getid3-1.7.6/getid3/write.vorbiscomment.php
--- getid3-1.7.6.orig/getid3/write.vorbiscomment.php    2006-03-21 01:42:35.000000000 +0100
+++ getid3-1.7.6/getid3/write.vorbiscomment.php 2006-03-21 01:33:48.000000000 +0100
@@ -79,7 +79,7 @@

            } else {

-               $commandline = 'vorbiscomment -w --raw -c "'.$tempcommentsfilename.'" "'.$this->filename.'" 2>&1';
+               $commandline = 'vorbiscomment -w --raw -c '.escapeshellarg($tempcommentsfilename).' '.escapeshellarg($this->filename).' 2>&1';
                $VorbiscommentError = `$commandline`;

            }

rasher
User
Posts: 3
Joined: Tue Dec 27, 2005 4:02 pm

Post by rasher » Tue Mar 21, 2006 1:01 am

Additionally (and in my attempt to make a clean patch, I forgot to redo this change), at least my version of 'tail' will think that the last argument is a filename, so there must not be a space between -c and the number of bytes. The following patch changes this.

Code: Select all

diff -ru getid3-1.7.6.orig/getid3/getid3.lib.php getid3-1.7.6/getid3/getid3.lib.php
--- getid3-1.7.6.orig/getid3/getid3.lib.php     2006-03-21 01:22:21.000000000 +0100
+++ getid3-1.7.6/getid3/getid3.lib.php  2006-03-21 01:57:40.000000000 +0100
@@ -624,7 +624,7 @@
                        } else {

                                $commandline  = 'head -c '.$end.' "'.$file.'" | ';
-                               $commandline .= 'tail -c '.$size.' | ';
+                               $commandline .= 'tail -c'.$size.' | ';
                                $commandline .= $unix_call;

                        }

Allan Hansen
getID3() v2 developer
Posts: 445
Joined: Sun May 04, 2003 2:22 pm
Location: Holmegaard, Denmark

Post by Allan Hansen » Thu Jun 08, 2006 9:15 am

Bug confirmed.

Non-writing bugs fixed in 2.0.0 and 1.7.7.

Same with head and tail.

Locked