Modified 'libsi' to require callers to state the buffer sizes when getting strings in order to avoid buffer overflows1.3.10

9 files changed, 117 insertions, 94 deletions

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 4c2889b0..d6f34f1d 100644
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -1021,3 +1021,6 @@ Marco Schlüßler <marco@lordzodiac.de>
 Jürgen Schmitz <j.schmitz@web.de>
  for reporting a bug in displaying the current channel when switching via the SVDRP
  command CHAN
+
+Philip Lawatsch <philip@lawatsch.at>
+ for debugging a buffer overflow in eit.c
diff --git a/HISTORY b/HISTORY
index 2032e456..bd0605eb 100644
--- a/HISTORY
+++ b/HISTORY
@@ -2889,3 +2889,6 @@ Video Disk Recorder Revision History
 - Added a note about the default assignment of the color keys to MANUAL.
 - Added a note about NPTL ("Native Posix Thread Library") to the INSTALL file
   (apparently the "fix" in version 1.3.0 didn't really fix this).
+- Modified 'libsi' to require callers to state the buffer sizes when getting
+  strings in order to avoid buffer overflows (thanks to Philip Lawatsch for
+  debugging a buffer overflow in eit.c).
diff --git a/eit.c b/eit.c
index 97ba5230..3ac53fac 100644
--- a/eit.c
+++ b/eit.c
@@ -8,7 +8,7 @@
  * Robert Schneider <Robert.Schneider@web.de> and Rolf Hakenes <hakenes@hippomi.de>.
  * Adapted to 'libsi' for VDR 1.3.0 by Marcel Wiesweg <marcel.wiesweg@gmx.de>.
  *
- * $Id: eit.c 1.94 2004/03/20 10:53:23 kls Exp $
+ * $Id: eit.c 1.95 2004/06/06 14:49:45 kls Exp $
  */
 
 #include "eit.h"
@@ -193,12 +193,12 @@ cEIT::cEIT(cSchedules *Schedules, int Source, u_char Tid, const u_char *Data)
       if (!rEvent) {
          if (ShortEventDescriptor) {
             char buffer[256];
-            pEvent->SetTitle(ShortEventDescriptor->name.getText(buffer));
-            pEvent->SetShortText(ShortEventDescriptor->text.getText(buffer));
+            pEvent->SetTitle(ShortEventDescriptor->name.getText(buffer, sizeof(buffer)));
+            pEvent->SetShortText(ShortEventDescriptor->text.getText(buffer, sizeof(buffer)));
             }
          if (ExtendedEventDescriptors) {
-            char buffer[ExtendedEventDescriptors->getMaximumTextLength(": ")];
-            pEvent->SetDescription(ExtendedEventDescriptors->getText(buffer, ": "));
+            char buffer[ExtendedEventDescriptors->getMaximumTextLength(": ") + 1];
+            pEvent->SetDescription(ExtendedEventDescriptors->getText(buffer, sizeof(buffer), ": "));
             }
          }
       delete ExtendedEventDescriptors;
diff --git a/libsi/descriptor.c b/libsi/descriptor.c
index 685722e7..eb921c95 100644
--- a/libsi/descriptor.c
+++ b/libsi/descriptor.c
@@ -6,7 +6,7 @@
  *   the Free Software Foundation; either version 2 of the License, or     *
  *   (at your option) any later version.                                   *
  *                                                                         *
- *   $Id: descriptor.c 1.12 2004/03/26 15:25:28 kls Exp $
+ *   $Id: descriptor.c 1.13 2004/06/06 14:47:30 kls Exp $
  *                                                                         *
  ***************************************************************************/
 
@@ -84,53 +84,61 @@ int ExtendedEventDescriptors::getMaximumTextLength(const char *separation1, cons
 }
 
 char *ExtendedEventDescriptors::getText(const char *separation1, const char *separation2) {
-   char *text=new char[getMaximumTextLength(separation1, separation2)];
-   return getText(text, separation1, separation2);
+   int size = getMaximumTextLength(separation1, separation2);
+   char *text=new char[size];
+   return getText(text, size, separation1, separation2);
 }
 
-char *ExtendedEventDescriptors::getText(char *buffer, const char *separation1, const char *separation2) {
+char *ExtendedEventDescriptors::getText(char *buffer, int size, const char *separation1, const char *separation2) {
    int index=0, len;
-   char tempbuf[256];
    for (int i=0;i<length;i++) {
       ExtendedEventDescriptor *d=(ExtendedEventDescriptor *)array[i];
       if (!d)
          continue;
-      d->text.getText(tempbuf);
-      len=strlen(tempbuf);
-      if (len) {
-         memcpy(buffer+index, tempbuf, len);
-         index+=len;
-      }
+      d->text.getText(buffer+index, size);
+      len = strlen(buffer+index);
+      index += len;
+      size -= len;
    }
 
+   int sepLen1 = strlen(separation1);
+   int sepLen2 = strlen(separation2);
+   bool separated = false;
    for (int i=0;i<length;i++) {
       ExtendedEventDescriptor *d=(ExtendedEventDescriptor *)array[i];
       if (!d)
          continue;
 
-      strcpy(buffer+index, separation2); // let's have a separator between the long text and the items
-      index += strlen(separation2);
       ExtendedEventDescriptor::Item item;
       for (Loop::Iterator it; d->itemLoop.hasNext(it);   ) {
          item=d->itemLoop.getNext(it);
 
-         item.itemDescription.getText(tempbuf);
-         len=strlen(tempbuf);
-         if (len) {
-            memcpy(buffer+index, tempbuf, len);
-            index+=len;
+         if (!separated && size > sepLen2) {
+            strcpy(buffer+index, separation2); // let's have a separator between the long text and the items
+            index += sepLen2;
+            size -= sepLen2;
+            separated = true;
+         }
+
+         item.itemDescription.getText(buffer+index, size);
+         len = strlen(buffer+index);
+         index += len;
+         size -= len;
+         if (size > sepLen1) {
+            strcpy(buffer+index, separation1);
+            index += sepLen1;
+            size -= sepLen1;
          }
-         strcpy(buffer+index, separation1);
-         index += strlen(separation1);
-
-         item.item.getText(tempbuf);
-         len=strlen(tempbuf);
-         if (len) {
-            memcpy(buffer+index, tempbuf, len);
-            index+=len;
+
+         item.item.getText(buffer+index, size);
+         len = strlen(buffer+index);
+         index += len;
+         size -= len;
+         if (size > sepLen2) {
+            strcpy(buffer+index, separation2);
+            index += sepLen2;
+            size -= sepLen2;
          }
-         strcpy(buffer+index, separation2);
-         index += strlen(separation2);
       }
    }
 
@@ -150,23 +158,21 @@ int ExtendedEventDescriptors::getMaximumTextPlainLength() {
 }
 
 char *ExtendedEventDescriptors::getTextPlain() {
-   char *text=new char[getMaximumTextPlainLength()];
-   return getTextPlain(text);
+   int size = getMaximumTextPlainLength();
+   char *text=new char[size];
+   return getTextPlain(text, size);
 }
 
-char *ExtendedEventDescriptors::getTextPlain(char *buffer) {
+char *ExtendedEventDescriptors::getTextPlain(char *buffer, int size) {
    int index=0, len;
-   char tempbuf[256];
    for (int i=0;i<length;i++) {
       ExtendedEventDescriptor *d=(ExtendedEventDescriptor *)array[i];
       if (!d)
          continue;
-      d->text.getText(tempbuf);
-      len=strlen(tempbuf);
-      if (len) {
-         memcpy(buffer+index, tempbuf, len);
-         index+=len;
-      }
+      d->text.getText(buffer+index, size);
+      len = strlen(buffer+index);
+      index += len;
+      size -= len;
    }
    buffer[index]='\0';
    return buffer;
@@ -174,25 +180,27 @@ char *ExtendedEventDescriptors::getTextPlain(char *buffer) {
 
 int ExtendedEventDescriptors::getMaximumTextItemizedLength(const char *separation1, const char *separation2) {
    int ret=0;
-   int sepLength=strlen(separation1)+strlen(separation2)-2;
+   int sepLength=strlen(separation1)+strlen(separation2);
    for (int i=0;i<length;i++) {
       ExtendedEventDescriptor *d=(ExtendedEventDescriptor *)array[i];
       if (!d)
          continue;
-      //The length includes two 8-bit length fields which have already been subtracted from sepLength
+      //The length includes two 8-bit length fields which have already been subtracted from sepLength //XXX kls 2004-06-06: what does this mean???
       ret+=d->itemLoop.getLength()+sepLength;
    }
    return ret;
 }
 
 char *ExtendedEventDescriptors::getTextItemized(const char *separation1, const char *separation2) {
-   char *text=new char[getMaximumTextItemizedLength(separation1, separation2)];
-   return getTextItemized(text, separation1, separation2);
+   int size = getMaximumTextItemizedLength(separation1, separation2);
+   char *text=new char[size];
+   return getTextItemized(text, size, separation1, separation2);
 }
 
-char *ExtendedEventDescriptors::getTextItemized(char *buffer, const char *separation1, const char *separation2) {
+char *ExtendedEventDescriptors::getTextItemized(char *buffer, int size, const char *separation1, const char *separation2) {
    int index=0, len;
-   char tempbuf[256];
+   int sepLen1 = strlen(separation1);
+   int sepLen2 = strlen(separation2);
    for (int i=0;i<length;i++) {
       ExtendedEventDescriptor *d=(ExtendedEventDescriptor *)array[i];
       if (!d)
@@ -202,23 +210,25 @@ char *ExtendedEventDescriptors::getTextItemized(char *buffer, const char *separa
       for (Loop::Iterator it; d->itemLoop.hasNext(it);   ) {
          item=d->itemLoop.getNext(it);
 
-         item.itemDescription.getText(tempbuf);
-         len=strlen(tempbuf);
-         if (len) {
-            memcpy(buffer+index, tempbuf, len);
-            index+=len;
+         item.itemDescription.getText(buffer+index, size);
+         len = strlen(buffer+index);
+         index += len;
+         size -= len;
+         if (size > sepLen1) {
+            strcpy(buffer+index, separation1);
+            index += sepLen1;
+            size -= sepLen1;
          }
-         strcpy(buffer+index, separation1);
-         index += strlen(separation1);
-
-         item.item.getText(tempbuf);
-         len=strlen(tempbuf);
-         if (len) {
-            memcpy(buffer+index, tempbuf, len);
-            index+=len;
+
+         item.item.getText(buffer+index, size);
+         len = strlen(buffer+index);
+         index += len;
+         size -= len;
+         if (size > sepLen2) {
+            strcpy(buffer+index, separation2);
+            index += sepLen2;
+            size -= sepLen2;
          }
-         strcpy(buffer+index, separation2);
-         index += strlen(separation2);
       }
    }
    buffer[index]='\0';
@@ -227,7 +237,7 @@ char *ExtendedEventDescriptors::getTextItemized(char *buffer, const char *separa
 
 //returns the itemized text pair by pair. Maximum length for buffers is 256.
 //Return value is false if and only if the end of the list is reached.
-bool ExtendedEventDescriptors::getTextItemized(Loop::Iterator &it, bool &valid, char *itemDescription, char *itemText) {
+bool ExtendedEventDescriptors::getTextItemized(Loop::Iterator &it, bool &valid, char *itemDescription, char *itemText, int sizeItemDescription, int sizeItemText) {
    //The iterator has to store two values: The descriptor index (4bit)
    //and the item loop index (max overall length 256, min item length 16 => max number 128 => 7bit)
    valid=false;
@@ -244,8 +254,8 @@ bool ExtendedEventDescriptors::getTextItemized(Loop::Iterator &it, bool &valid,
       if (d->itemLoop.hasNext(it)) {
          item=d->itemLoop.getNext(it);
 
-         item.item.getText(itemDescription);
-         item.itemDescription.getText(itemText);
+         item.item.getText(itemDescription, sizeItemDescription);
+         item.itemDescription.getText(itemText, sizeItemText);
          valid=true;
          break;
       } else {
diff --git a/libsi/descriptor.h b/libsi/descriptor.h
index db3bba61..94959231 100644
--- a/libsi/descriptor.h
+++ b/libsi/descriptor.h
@@ -6,7 +6,7 @@
  *   the Free Software Foundation; either version 2 of the License, or     *
  *   (at your option) any later version.                                   *
  *                                                                         *
- *   $Id: descriptor.h 1.9 2004/03/26 15:26:03 kls Exp $
+ *   $Id: descriptor.h 1.10 2004/06/06 13:51:29 kls Exp $
  *                                                                         *
  ***************************************************************************/
 
@@ -55,12 +55,12 @@ public:
    //same semantics as with SI::String
    char *getText(const char *separation1="\t", const char *separation2="\n");
    //buffer must at least be getTextLength(), getMaximumTextLength() is a good choice
-   char *getText(char *buffer, const char *separation1="\t", const char *separation2="\n");
+   char *getText(char *buffer, int size, const char *separation1="\t", const char *separation2="\n");
 
    //these only return the non-itemized text fields in concatenated form
    int getMaximumTextPlainLength();
    char *getTextPlain();
-   char *getTextPlain(char *buffer);
+   char *getTextPlain(char *buffer, int size);
 
    //these only return the itemized text fields in concatenated form.
    //Between the description and the text the separation1 character is used,
@@ -68,11 +68,11 @@ public:
    //Director\tSteven Spielberg\nActor\tMichael Mendl\n
    int getMaximumTextItemizedLength(const char *separation1="\t", const char *separation2="\n");
    char *getTextItemized(const char *separation1="\t", const char *separation2="\n");
-   char *getTextItemized(char *buffer, const char *separation1="\t", const char *separation2="\n");
+   char *getTextItemized(char *buffer, int size, const char *separation1="\t", const char *separation2="\n");
    //returns the itemized text pair by pair. Maximum length for buffers is 256.
    //Return value is false if and only if the end of the list is reached.
    //The argument valid indicates whether the buffers contain valid content.
-   bool getTextItemized(Loop::Iterator &it, bool &valid, char *itemDescription, char *itemText);
+   bool getTextItemized(Loop::Iterator &it, bool &valid, char *itemDescription, char *itemText, int sizeItemDescription, int sizeItemText);
 };
 
 class TimeShiftedEventDescriptor : public Descriptor {
diff --git a/libsi/si.c b/libsi/si.c
index 2bc8339a..40f94539 100644
--- a/libsi/si.c
+++ b/libsi/si.c
@@ -6,7 +6,7 @@
  *   the Free Software Foundation; either version 2 of the License, or     *
  *   (at your option) any later version.                                   *
  *                                                                         *
- *   $Id: si.c 1.10 2004/05/29 17:06:23 kls Exp $
+ *   $Id: si.c 1.11 2004/06/06 14:43:56 kls Exp $
  *                                                                         *
  ***************************************************************************/
 
@@ -206,33 +206,36 @@ bool DescriptorGroup::isComplete() {
 
 char *String::getText() {
    if (getLength() < 0 || getLength() >4095)
-      return "text error";
+      return strdup("text error"); // caller will delete it!
    char *data=new char(getLength()+1);
-   decodeText(data);
+   decodeText(data, getLength()+1);
    return data;
 }
 
-char *String::getText(char *buffer) {
-   if (getLength() < 0 || getLength() >4095) {
-      strncpy(buffer, "text error", getLength()+1);
+char *String::getText(char *buffer, int size) {
+   if (getLength() < 0 || getLength() >= size) {
+      strncpy(buffer, "text error", size);
+      buffer[size-1] = 0;
       return buffer;
    }
-   decodeText(buffer);
+   decodeText(buffer, size);
    return buffer;
 }
 
 //taken from VDR, Copyright Klaus Schmidinger <kls@cadsoft.de>
-char *String::getText(char *buffer, char *shortVersion) {
-   if (getLength() < 0 || getLength() >4095) {
-      strncpy(buffer, "text error", getLength()+1);
+char *String::getText(char *buffer, char *shortVersion, int sizeBuffer, int sizeShortVersion) {
+   if (getLength() < 0 || getLength() >= sizeBuffer) {
+      strncpy(buffer, "text error", sizeBuffer);
+      buffer[sizeBuffer-1] = 0;
+      *shortVersion = 0;
       return buffer;
    }
-   decodeText(buffer, shortVersion);
+   decodeText(buffer, shortVersion, sizeBuffer, sizeShortVersion);
    return buffer;
 }
 
 //taken from libdtv, Copyright Rolf Hakenes <hakenes@hippomi.de>
-void String::decodeText(char *buffer) {
+void String::decodeText(char *buffer, int size) {
    const unsigned char *from=data.getData(0);
    char *to=buffer;
 
@@ -254,11 +257,13 @@ void String::decodeText(char *buffer) {
       else if (*from == 0x8A)
          *to++ = '\n';
       from++;
+      if (to - buffer >= size - 1)
+         break;
    }
    *to = '\0';
 }
 
-void String::decodeText(char *buffer, char *shortVersion) {
+void String::decodeText(char *buffer, char *shortVersion, int sizeBuffer, int sizeShortVersion) {
    const unsigned char *from=data.getData(0);
    char *to=buffer;
    char *toShort=shortVersion;
@@ -283,6 +288,8 @@ void String::decodeText(char *buffer, char *shortVersion) {
       else if (*from == 0x87)
          IsShortName--;
       from++;
+      if (to - buffer >= sizeBuffer - 1 || toShort - shortVersion >= sizeShortVersion - 1)
+         break;
    }
    *to = '\0';
    *toShort = '\0';
diff --git a/libsi/si.h b/libsi/si.h
index 195830d6..85d16ed4 100644
--- a/libsi/si.h
+++ b/libsi/si.h
@@ -6,7 +6,7 @@
  *   the Free Software Foundation; either version 2 of the License, or     *
  *   (at your option) any later version.                                   *
  *                                                                         *
- *   $Id: si.h 1.9 2004/03/07 10:09:49 kls Exp $
+ *   $Id: si.h 1.10 2004/06/06 13:35:21 kls Exp $
  *                                                                         *
  ***************************************************************************/
 
@@ -431,18 +431,18 @@ public:
    //so the maximum there is 256.
    //returns the given buffer for convenience.
    //The emphasis marks 0x86 and 0x87 are still available.
-   char *getText(char *buffer);
+   char *getText(char *buffer, int size);
    //The same semantics as for getText(char*) apply.
    //The short version of the text according to ETSI TR 101 211 (chapter 4.6)
    //will be written into the shortVersion buffer (which should, therefore, have the same
    //length as buffer). If no shortVersion is available, shortVersion will contain
    //an empty string.
    //The emphasis marks 0x86 and 0x87 are still available in buffer, but not in shortVersion.
-   char *getText(char *buffer, char *shortVersion);
+   char *getText(char *buffer, char *shortVersion, int sizeBuffer, int sizeShortVersion);
 protected:
    virtual void Parse() {}
-   void decodeText(char *buffer);
-   void decodeText(char *buffer, char *shortVersion);
+   void decodeText(char *buffer, int size);
+   void decodeText(char *buffer, char *shortVersion, int sizeBuffer, int sizeShortVersion);
 };
 
 } //end of namespace
diff --git a/nit.c b/nit.c
index cf3b674f..c515c240 100644
--- a/nit.c
+++ b/nit.c
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: nit.c 1.7 2004/05/22 15:46:21 kls Exp $
+ * $Id: nit.c 1.8 2004/06/06 14:24:49 kls Exp $
  */
 
 #include "nit.h"
@@ -71,7 +71,7 @@ void cNitFilter::Process(u_short Pid, u_char Tid, const u_char *Data, int Length
                switch (d->getDescriptorTag()) {
                  case SI::NetworkNameDescriptorTag: {
                       SI::NetworkNameDescriptor *nnd = (SI::NetworkNameDescriptor *)d;
-                      nnd->name.getText(nits[numNits].name);
+                      nnd->name.getText(nits[numNits].name, MAXNETWORKNAME);
                       }
                       break;
                  default: ;
diff --git a/sdt.c b/sdt.c
index 5ac6b419..247d2c9b 100644
--- a/sdt.c
+++ b/sdt.c
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: sdt.c 1.8 2004/03/07 10:46:08 kls Exp $
+ * $Id: sdt.c 1.9 2004/06/06 14:25:22 kls Exp $
  */
 
 #include "sdt.h"
@@ -59,7 +59,7 @@ void cSdtFilter::Process(u_short Pid, u_char Tid, const u_char *Data, int Length
                         {
                         char NameBuf[1024];
                         char ShortNameBuf[1024];
-                        sd->serviceName.getText(NameBuf, ShortNameBuf);
+                        sd->serviceName.getText(NameBuf, ShortNameBuf, sizeof(NameBuf), sizeof(ShortNameBuf));
                         char *pn = compactspace(NameBuf);
                         char *ps = compactspace(ShortNameBuf);
                         if (*NameBuf && *ShortNameBuf && strcmp(NameBuf, ShortNameBuf) != 0) {