From 3f21bf20c52599cc6dbe252488dc3dda36402f3c Mon Sep 17 00:00:00 2001 From: Klaus Schmidinger Date: Fri, 30 Dec 2005 15:11:16 +0100 Subject: [PATCH] New option '-g'; fixed security hole CAN-2005-0071 when grabbing to file --- CONTRIBUTORS | 6 ++++++ HISTORY | 16 +++++++++++++++- device.c | 10 +++++----- svdrp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++----- svdrp.h | 4 +++- vdr.1 | 9 ++++++++- vdr.c | 11 +++++++++-- 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 '--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 for translating OSD texts to the Italian language @@ -1571,3 +1573,7 @@ Bob Withers 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 + 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 @@ -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"