From: pabs <pabs@1f10aa63-cdf2-0310-b900-c93c546f37ac>
Date: Fri, 7 Dec 2007 04:29:57 +0000 (+0000)
Subject: Minor security fix: the unused mod_ffmpeg importer used popen to run convert, change... 
X-Git-Url: https://git.pterodactylus.net/?a=commitdiff_plain;h=83224f8cf1f9b7b15918a130759717a5a375be91;p=synfig.git

Minor security fix: the unused mod_ffmpeg importer used popen to run convert, change it to use pipe, fork and exec. Malicious sif files could previously have executed arbitrary shell commands. Also disable the unused part of the mod_imagemagick importer.


git-svn-id: http://svn.voria.com/code@1186 1f10aa63-cdf2-0310-b900-c93c546f37ac
---

diff --git a/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.cpp b/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.cpp
index bc4cc7b..f05b2d4 100644
--- a/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.cpp
+++ b/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.cpp
@@ -34,6 +34,9 @@
 #include <ETL/stringf>
 #include "mptr_ffmpeg.h"
 #include <stdio.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
 #include <iostream>
 #include <algorithm>
 #include <functional>
@@ -63,14 +66,47 @@ ffmpeg_mptr::seek_to(int frame)
 	{
 		if(file)
 		{
-			pclose(file);
+			fclose(file);
+			int status;
+			waitpid(pid,&status,0);
 		}
 
-		string command;
-
-		command=strprintf("ffmpeg -i \"%s\" -an -f image2pipe -vcodec ppm -\n",filename.c_str());
-
-		file=popen(command.c_str(),POPEN_BINARY_READ_TYPE);
+		int p[2];
+	  
+		if (pipe(p)) {
+			cerr<<"Unable to open pipe to ffmpeg"<<endl;
+			return false;
+		};
+	  
+		pid = fork();
+	  
+		if (pid == -1) {
+			cerr<<"Unable to open pipe to ffmpeg"<<endl;
+			return false;
+		}
+	  
+		if (pid == 0){
+			// Child process
+			// Close pipein, not needed
+			close(p[0]);
+			// Dup pipein to stdout
+			if( dup2( p[1], STDOUT_FILENO ) == -1 ){
+				cerr<<"Unable to open pipe to ffmpeg"<<endl;
+				return false;
+			}
+			// Close the unneeded pipein
+			close(p[1]);
+			execlp("ffmpeg", "ffmpeg", "-i", filename.c_str(), "-an", "-f", "image2pipe", "-vcodec", "ppm", "-", (const char *)NULL);
+			// We should never reach here unless the exec failed
+			cerr<<"Unable to open pipe to ffmpeg"<<endl;
+			return false;
+		} else {
+			// Parent process
+			// Close pipeout, not needed
+			close(p[1]);
+			// Save pipeout to file handle, will write to it later
+			file = fdopen(p[0], "wb");
+		}
 
 		if(!file)
 		{
@@ -148,6 +184,7 @@ ffmpeg_mptr::grab_frame(void)
 
 ffmpeg_mptr::ffmpeg_mptr(const char *f)
 {
+	pid=-1;
 #ifdef HAVE_TERMIOS_H
 	tcgetattr (0, &oldtty);
 #endif
@@ -160,7 +197,11 @@ ffmpeg_mptr::ffmpeg_mptr(const char *f)
 ffmpeg_mptr::~ffmpeg_mptr()
 {
 	if(file)
-		pclose(file);
+	{
+		fclose(file);
+		int status;
+		waitpid(pid,&status,0);
+	}
 #ifdef HAVE_TERMIOS_H
 	tcsetattr(0,TCSANOW,&oldtty);
 #endif
diff --git a/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.h b/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.h
index e77b513..9e421c9 100644
--- a/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.h
+++ b/synfig-core/trunk/src/modules/mod_ffmpeg/mptr_ffmpeg.h
@@ -30,6 +30,7 @@
 /* === H E A D E R S ======================================================= */
 
 #include <synfig/importer.h>
+#include <sys/types.h>
 #include <stdio.h>
 #include "string.h"
 #ifdef HAVE_TERMIOS_H
@@ -49,6 +50,7 @@ class ffmpeg_mptr : public synfig::Importer
 	SYNFIG_IMPORTER_MODULE_EXT
 public:
 private:
+	pid_t pid;
 	synfig::String filename;
 	FILE *file;
 	int cur_frame;
diff --git a/synfig-core/trunk/src/modules/mod_imagemagick/mptr_imagemagick.cpp b/synfig-core/trunk/src/modules/mod_imagemagick/mptr_imagemagick.cpp
index a077230..2481d06 100644
--- a/synfig-core/trunk/src/modules/mod_imagemagick/mptr_imagemagick.cpp
+++ b/synfig-core/trunk/src/modules/mod_imagemagick/mptr_imagemagick.cpp
@@ -159,6 +159,9 @@ imagemagick_mptr::get_frame(synfig::Surface &surface,Time /*time*/, synfig::Prog
 	return true;
 
 #else
+	
+#error This code contains tempfile and arbitrary shell command execution vulnerabilities
+	
 	if(file)
 		pclose(file);