New option '-g'; fixed security hole CAN-2005-0071 when grabbing to file

7 files changed, 95 insertions, 15 deletions

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 44641efd..2e977d5e 100644
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -1370,6 +1370,8 @@ Darren Salt <linux@youmustbejoking.demon.co.uk>
  '--no-kbd'
  for adding '__attribute__' to functions that use printf() like parameters
  for suggesting to write grabbed images to the SVDRP connection encoded in base64
+ for suggesting to open the file handle in the SVDRP GRAB command in a way that
+ it won't follow symbolic links, and to canonicalize the file name
 
 Sean Carlos <seanc@libero.it>
  for translating OSD texts to the Italian language
@@ -1571,3 +1573,7 @@ Bob Withers <bwit@pobox.com>
  for publishing a Base64 encoder at http://www.ruffboy.com/download.htm
  (http://www.ruffboy.com/code/Base64.zip), part of which was used when writing
  the cBase64Encoder class
+
+Javier Fernández-Sanguino Peña <jfs@computer.org>
+ for reporting a security hole in the way the SVDRP command GRAB writes the
+ image file
diff --git a/HISTORY b/HISTORY
index 40304569..ab8bab01 100644
--- a/HISTORY
+++ b/HISTORY
@@ -3963,7 +3963,7 @@ Video Disk Recorder Revision History
   commands may now be executed at any time, and the message will be displayed
   (no more "pending message").
 
-2005-12-29: Version 1.3.38
+2005-12-30: Version 1.3.38
 
 - Fixed handling second audio and Dolby Digital PIDs for encrypted channels
   (was broken in version 1.3.37).
@@ -4023,3 +4023,17 @@ Video Disk Recorder Revision History
   (encoded in base64) if the given file name consists of only the file
   extension (".jpg", ".jpeg" or ".pnm"), or if only "-" is given as file
   name (based on a suggestion from Darren Salt).
+- The new command line option '-g' must be given if the SVDRP command GRAB
+  shall be allowed to write image files to disk. The parameter to this option
+  must be the full path name of an existing directory, without any "..", double
+  '/' or symlinks. By default, or if "-g- is given, grabbing to files is
+  not allowed any more because of potential security risks.
+- Modified the way the SVDRP command GRAB writes the grabbed image to a file
+  to avoid a security hole (CAN-2005-0071, reported by Javier Fernández-Sanguino
+  Peña):
+  + The file handle is now opened in a way that it won't follow symbolic links
+    (suggested by Darren Salt).
+  + The given file name is now canonicalized, so that it won't contain any
+    ".." or symlinks (suggested by Darren Salt).
+  + Grabbing to files is limited to the directory given in the the command
+    line option '-g'. By default grabbing to files is not allowed any more.
diff --git a/device.c b/device.c
index fb5360c8..cd8a1402 100644
--- a/device.c
+++ b/device.c
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: device.c 1.113 2005/12/29 14:51:41 kls Exp $
+ * $Id: device.c 1.114 2005/12/30 13:48:29 kls Exp $
  */
 
 #include "device.h"
@@ -330,12 +330,12 @@ uchar *cDevice::GrabImage(int &Size, bool Jpeg, int Quality, int SizeX, int Size
 bool cDevice::GrabImageFile(const char *FileName, bool Jpeg, int Quality, int SizeX, int SizeY)
 {
   int result = 0;
-  FILE *f = fopen(FileName, "wb");
-  if (f) {
+  int fd = open(FileName, O_WRONLY | O_CREAT | O_NOFOLLOW | O_TRUNC, DEFFILEMODE);
+  if (fd >= 0) {
      int ImageSize;
      uchar *Image = GrabImage(ImageSize, Jpeg, Quality, SizeX, SizeY);
      if (Image) {
-        if (fwrite(Image, ImageSize, 1, f) == 1)
+        if (safe_write(fd, Image, ImageSize) == ImageSize)
            isyslog("grabbed image to %s", FileName);
         else {
            LOG_ERROR_STR(FileName);
@@ -345,7 +345,7 @@ bool cDevice::GrabImageFile(const char *FileName, bool Jpeg, int Quality, int Si
         }
      else
         result |= 1;
-     fclose(f);
+     close(fd);
      }
   else {
      LOG_ERROR_STR(FileName);
diff --git a/svdrp.c b/svdrp.c
index 74b89758..7d84ed4d 100644
--- a/svdrp.c
+++ b/svdrp.c
@@ -10,7 +10,7 @@
  * and interact with the Video Disk Recorder - or write a full featured
  * graphical interface that sits on top of an SVDRP connection.
  *
- * $Id: svdrp.c 1.87 2005/12/30 10:27:23 kls Exp $
+ * $Id: svdrp.c 1.88 2005/12/30 15:11:16 kls Exp $
  */
 
 #include "svdrp.h"
@@ -361,6 +361,8 @@ const char *GetHelpPage(const char *Cmd, const char **p)
   return NULL;
 }
 
+char *cSVDRP::grabImageDir = NULL;
+
 cSVDRP::cSVDRP(int Port)
 :socket(Port)
 {
@@ -663,6 +665,7 @@ void cSVDRP::CmdGRAB(const char *Option)
      const char *delim = " \t";
      char *strtok_next;
      FileName = strtok_r(p, delim, &strtok_next);
+     // image type:
      char *Extension = strrchr(FileName, '.');
      if (Extension) {
         if (strcasecmp(Extension, ".jpg") == 0 || strcasecmp(Extension, ".jpeg") == 0)
@@ -682,6 +685,7 @@ void cSVDRP::CmdGRAB(const char *Option)
         Reply(501, "Missing filename extension in \"%s\"", FileName);
         return;
         }
+     // image quality (and obsolete type):
      if ((p = strtok_r(NULL, delim, &strtok_next)) != NULL) {
         if (strcasecmp(p, "JPEG") == 0 || strcasecmp(p, "PNM") == 0) {
            // tolerate for backward compatibility
@@ -696,6 +700,7 @@ void cSVDRP::CmdGRAB(const char *Option)
               }
            }
         }
+     // image size:
      if ((p = strtok_r(NULL, delim, &strtok_next)) != NULL) {
         if (isnumber(p))
            SizeX = atoi(p);
@@ -720,19 +725,52 @@ void cSVDRP::CmdGRAB(const char *Option)
         Reply(501, "Unexpected parameter \"%s\"", p);
         return;
         }
+     // canonicalize the file name:
+     char RealFileName[PATH_MAX];
+     if (FileName) {
+        if (grabImageDir) {
+           char *s;
+           asprintf(&s, "%s/%s", grabImageDir, FileName);
+           FileName = s;
+           char *slash = strrchr(FileName, '/'); // there definitely is one
+           *slash = 0;
+           char *r = realpath(FileName, RealFileName);
+           *slash = '/';
+           if (!r) {
+              LOG_ERROR_STR(FileName);
+              Reply(501, "Illegal file name \"%s\"", FileName);
+              free(s);
+              return;
+              }
+           strcat(RealFileName, slash);
+           FileName = RealFileName;
+           free(s);
+           if (strncmp(FileName, grabImageDir, strlen(grabImageDir)) != 0) {
+              Reply(501, "Illegal file name \"%s\"", FileName);
+              return;
+              }
+           }
+        else {
+           Reply(550, "Grabbing to file not allowed (use \"GRAB -\" instead)");
+           return;
+           }
+        }
+     // actual grabbing:
      int ImageSize;
      uchar *Image = cDevice::PrimaryDevice()->GrabImage(ImageSize, Jpeg, Quality, SizeX, SizeY);
      if (Image) {
         if (FileName) {
-           FILE *f = fopen(FileName, "wb");
-           if (f) {
-              if (fwrite(Image, ImageSize, 1, f) == 1)
+           int fd = open(FileName, O_WRONLY | O_CREAT | O_NOFOLLOW | O_TRUNC, DEFFILEMODE);
+           if (fd >= 0) {
+              if (safe_write(fd, Image, ImageSize) == ImageSize) {
+                 isyslog("grabbed image to %s", FileName);
                  Reply(250, "Grabbed image %s", Option);
+                 }
               else {
                  LOG_ERROR_STR(FileName);
                  Reply(451, "Can't write to '%s'", FileName);
                  }
-              fclose(f);
+              close(fd);
               }
            else {
               LOG_ERROR_STR(FileName);
@@ -1530,4 +1568,10 @@ bool cSVDRP::Process(void)
   return false;
 }
 
+void cSVDRP::SetGrabImageDir(const char *GrabImageDir)
+{
+  free(grabImageDir);
+  grabImageDir = GrabImageDir ? strdup(GrabImageDir) : NULL;
+}
+
 //TODO more than one connection???
diff --git a/svdrp.h b/svdrp.h
index 469c47a5..1229f8ef 100644
--- a/svdrp.h
+++ b/svdrp.h
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: svdrp.h 1.26 2005/11/27 15:26:42 kls Exp $
+ * $Id: svdrp.h 1.27 2005/12/30 14:46:38 kls Exp $
  */
 
 #ifndef __SVDRP_H
@@ -49,6 +49,7 @@ private:
   int length;
   char *cmdLine;
   time_t lastActivity;
+  static char *grabImageDir;
   void Close(bool Timeout = false);
   bool Send(const char *s, int length = -1);
   void Reply(int Code, const char *fmt, ...) __attribute__ ((format (printf, 3, 4)));
@@ -87,6 +88,7 @@ public:
   ~cSVDRP();
   bool HasConnection(void) { return file.IsOpen(); }
   bool Process(void);
+  static void SetGrabImageDir(const char *GrabImageDir);
   };
 
 #endif //__SVDRP_H
diff --git a/vdr.1 b/vdr.1
index f50db90d..54deb51d 100644
--- a/vdr.1
+++ b/vdr.1
@@ -8,7 +8,7 @@
 .\" License as specified in the file COPYING that comes with the
 .\" vdr distribution.
 .\"
-.\" $Id: vdr.1 1.15 2005/10/09 12:31:03 kls Exp $
+.\" $Id: vdr.1 1.16 2005/12/30 15:09:01 kls Exp $
 .\"
 .TH vdr 1 "19 Dec 2004" "1.3.18" "Video Disk Recorder"
 .SH NAME
@@ -59,6 +59,13 @@ Use \fB\-E\-\fR to disable this.
 If \fIfile\fR is a directory, the file \fIepg.data\fR
 will be created in that directory.
 .TP
+.BI \-g,\ \-\-grab= dir
+Write images from the SVDRP command GRAB into the
+given directory \fIdir\fR. \fIdir\fR must be the full path name of an
+existing directory, without any "..", double '/'
+or symlinks. By default, or if \fB\-g\-\fR is given,
+grabbing images to disk is disabled.
+.TP
 .B \-h, \-\-help
 Print a help message and exit.
 .TP
diff --git a/vdr.c b/vdr.c
index 61f8a54c..0d22f257 100644
--- a/vdr.c
+++ b/vdr.c
@@ -22,7 +22,7 @@
  *
  * The project's page is at http://www.cadsoft.de/vdr
  *
- * $Id: vdr.c 1.222 2005/12/18 14:38:30 kls Exp $
+ * $Id: vdr.c 1.223 2005/12/30 15:07:47 kls Exp $
  */
 
 #include <getopt.h>
@@ -144,6 +144,7 @@ int main(int argc, char *argv[])
       { "daemon",   no_argument,       NULL, 'd' },
       { "device",   required_argument, NULL, 'D' },
       { "epgfile",  required_argument, NULL, 'E' },
+      { "grab",     required_argument, NULL, 'g' },
       { "help",     no_argument,       NULL, 'h' },
       { "lib",      required_argument, NULL, 'L' },
       { "lirc",     optional_argument, NULL, 'l' | 0x100 },
@@ -164,7 +165,7 @@ int main(int argc, char *argv[])
     };
 
   int c;
-  while ((c = getopt_long(argc, argv, "a:c:dD:E:hl:L:mp:P:r:s:t:v:Vw:", long_options, NULL)) != -1) {
+  while ((c = getopt_long(argc, argv, "a:c:dD:E:g:hl:L:mp:P:r:s:t:v:Vw:", long_options, NULL)) != -1) {
         switch (c) {
           case 'a': AudioCommand = optarg;
                     break;
@@ -183,6 +184,8 @@ int main(int argc, char *argv[])
                     break;
           case 'E': EpgDataFileName = (*optarg != '-' ? optarg : NULL);
                     break;
+          case 'g': cSVDRP::SetGrabImageDir(*optarg != '-' ? optarg : NULL);
+                    break;
           case 'h': DisplayHelp = true;
                     break;
           case 'l': {
@@ -290,6 +293,10 @@ int main(int argc, char *argv[])
                "                           '-E-' disables this\n"
                "                           if FILE is a directory, the default EPG file will be\n"
                "                           created in that directory\n"
+               "  -g DIR    --grab=DIR     write images from the SVDRP command GRAB into the\n"
+               "                           given DIR; DIR must be the full path name of an\n"
+               "                           existing directory, without any \"..\", double '/'\n"
+               "                           or symlinks (default: none, same as -g-)\n"
                "  -h,       --help         print this help and exit\n"
                "  -l LEVEL, --log=LEVEL    set log level (default: 3)\n"
                "                           0 = no logging, 1 = errors only,\n"