examining debian's vdr patchset

  I was just looking at the patches included with debian's vdr package, and there
were two patches included that I had never seen before.  I did not write these
patches, but I just wanted to post them to see what everyone's
thoughts/reactions are about them (if they should be added to core vdr or not).

Best Regards,
C.

diff -ru vdr-1.3.27-eepg-orig/dvbdevice.c vdr-1.3.27-eepg/dvbdevice.c
--- vdr-1.3.27-orig/dvbdevice.c	2005-06-19 08:21:07.000000000 -0700
+++ vdr-1.3.27/dvbdevice.c	2005-06-29 15:56:57.000000000 -0700
@@ -699,7 +699,9 @@
               Quality = 100;
 
            isyslog("grabbing to %s (%s %d %d %d)", FileName, Jpeg ? "JPEG" : "PNM", Quality, vm.width, vm.height);
-           FILE *f = fopen(FileName, "wb");
+           int fd = open(FileName, O_CREAT | O_EXCL | O_TRUNC | O_RDWR, 00640);
+           if (fd > -1) {
+           FILE *f = fdopen(fd, "wb");
            if (f) {
               if (Jpeg) {
                  // write JPEG file:
@@ -735,6 +737,7 @@
                  }
               fclose(f);
               }
+           }
            else {
               LOG_ERROR_STR(FileName);
               result |= 1;
  

C.Y.M wrote:
> -    setMax(minsize[colorid].y2, yp + len - 1);
> +    setMax(minsize[colorid].y2, yp);

This looks like a minor performance bug fix, as the written area is a
horizontal line, not a box.

> +static bool OsdMatchesArea(cOsd *osd, tArea &area)

This (plus remainings of patch) re-allocates the osd if the new area
doesnt fit into the old osd. This may be a bug, if this actually occurs.

As far as I understand it, the dvbspu.c translates subpictures to osd
bitmaps, though I dont really know who actually uses this. Maybe DVD plugin?



The second patch is a security patch, described here:
http://www.debian.org/security/2005/dsa-656

> -           FILE *f = fopen(FileName, "wb");
> +           int fd = open(FileName, O_CREAT | O_EXCL | O_TRUNC | O_RDWR, 00640);
> +           if (fd > -1) {
> +           FILE *f = fdopen(fd, "wb");

This seems to force creating a new GRAB file with (00640 & ~umask)
access rights, while fopen always uses (00666 & ~umask).
Additionally, this version fails if the GRAB file already exists.
(vdradmin-am wont work with this, as the file is pre-allocated by
vdradmin-am. ;) )

I dont agree with this fix, because (1) insecure SVDRP access is IMHO a
security hole in any case, (2) if VDR runs properly as restricted user,
there shouldnt be any critical files with write access, and (3) though
the patched version cannot overwrite existing files, it still can create
new files anywhere, and thats IMHO not much better.

Cheers,

Udo
  

I demand that Udo Richter may or may not have written...

> C.Y.M wrote:
>> -    setMax(minsize[colorid].y2, yp + len - 1);
>> +    setMax(minsize[colorid].y2, yp);

> This looks like a minor performance bug fix, as the written area is a
> horizontal line, not a box.

> > +static bool OsdMatchesArea(cOsd *osd, tArea &area)

> This (plus remainings of patch) re-allocates the osd if the new area
> doesnt fit into the old osd. This may be a bug, if this actually occurs.

> As far as I understand it, the dvbspu.c translates subpictures to osd
> bitmaps, though I dont really know who actually uses this. Maybe DVD
> plugin?

Not sure, but it's one of Reinhard Nissl's patches.

> The second patch is a security patch, described here:
> http://www.debian.org/security/2005/dsa-656

>> -     FILE *f = fopen(FileName, "wb");
>> +     int fd = open(FileName, O_CREAT | O_EXCL | O_TRUNC | O_RDWR, 00640);
>> +     if (fd > -1) {
>> +     FILE *f = fdopen(fd, "wb");

> This seems to force creating a new GRAB file with (00640 & ~umask) access
> rights, while fopen always uses (00666 & ~umask). Additionally, this
> version fails if the GRAB file already exists. (vdradmin-am wont work with
> this, as the file is pre-allocated by vdradmin-am. ;) )

> I don't agree with this fix,

</AOL>.

> because (1) insecure SVDRP access is IMHO a security hole in any case,

True. (Klaus?)

> (2) if VDR runs properly as restricted user, there shouldn't be any
> critical files with write access,

True, *but* it's still possible to overwrite files which are, quite properly,
owned by vdr.

> and (3) though the patched version cannot overwrite existing files, it
> still can create new files anywhere, and thats IMHO not much better.

Agreed again. My VDR builds have used a similar patch (attached) which
restricts where these files can be written for some time now; vdradmin
shouldn't have a problem with it.

vdr-xine users will find a commented-out O_EXCL in xineLib.c - you should
uncomment this and replace it with O_NOFOLLOW. (My package already has this
patch; the official Debian package will too.)

(We still need a send-snap-as-base64 version. Both vdr and vdr-xine will
require modification for this; when I last looked at this, I came to the
conclusion that a file _handle_ needs to be passed to the snapshot-creation
code.)