Re: When to call CheckCopiedFile() ?

[Sergio . Gelato]

Earlier I wrote:

> I propose to call CheckCopiedFile() from within CopyReg(), just before
> the rename(new,dest) call.

I since noticed that CopyReg() is also called, without a subsequent
CheckCopiedFile(), from Repository(). I suppose one could simply set
dummy.plus, dummy.minus, dummy.uid and dummy.gid in Repository() to
reasonable values and go ahead, but since this bug is not a show-stopper
I'll just sit and think a while longer.

> 1. potential buffer overflows in forming the name of the backup file,
>    particularly when IMAGEBACKUP == 's'. Could be regarded as a
>    documentation issue ("Don't use very long destination file names"---
>    "Yes, but just how long is too long?"). But why doesn't cfengine
>    use strlcat() (or at least strncat()) throughout?
> 2. rename(backup,dest) is attempted unconditionally, even when
>    IMAGEBACKUP == 'n' (in which case the backup variable is undefined).

A patch for these two issues follows.

> 3. Wouldn't it be better to perform the sanity checks (size, checksum)
>    on the new file first, and only back up/remove the old file if they
>    succeed ?

I'll postpone action on that one.

--- orig/src/image.c
+++ mod/src/image.c
@@ -1456,7 +1456,7 @@
 
 { char backup[bufsize];
   char new[bufsize], *linkable;
-  int remote = false, silent, backupisdir=false;
+  int remote = false, silent, backupisdir=false, backupok=false;
   struct stat s;
 #ifdef HAVE_UTIME_H
   struct utimbuf timebuf;
@@ -1496,6 +1496,11 @@
    remote = true;
    }
 
+if (BufferOverflow(dest,CF_NEW))
+   {
+   printf(" culprit: CopyReg\n");
+   return false;
+   }
 strcpy(new,dest);
 strcat(new,CF_NEW);
 
@@ -1538,6 +1543,11 @@
    
    sprintf(stamp, "_%d_%s", CFSTARTTIME, CanonifyName(ctime(&STAMPNOW)));
 
+   if (BufferOverflow(dest,stamp))
+      {
+      printf(" culprit: CopyReg\n");
+      return false;
+      }
    strcpy(backup,dest);
 
    if (IMAGEBACKUP == 's')
@@ -1545,6 +1555,7 @@
       strcat(backup,stamp);
       }
 
+   /* rely on prior BufferOverflow() and on strlen(CF_SAVED) < buffer_margin */
    strcat(backup,CF_SAVED);
 
    if (IsItemIn(VREPOSLIST,backup))
@@ -1570,6 +1581,7 @@
       {
       /* ignore */
       }
+   backupok = (lstat(backup,&s) != -1);        /* Did the rename() succeed? 
NFS-safe */
    }
 else
    {
@@ -1596,9 +1608,9 @@
    {
    snprintf(OUTPUT,bufsize*2,"WARNING: new file %s seems to have been 
corrupted in transit (sizes %d and %d), aborting!\n",new, (int) dstat.st_size, 
(int) sstat.st_size);
    CfLog(cfverbose,OUTPUT,"");
-   if (rename(backup,dest) == -1)
+   if (backupok)
       {
-      /* ignore */
+      rename(backup,dest);     /* ignore failure */
       }
    return false;
    }
@@ -1610,8 +1622,9 @@
       {
       snprintf(OUTPUT,bufsize*2,"WARNING: new file %s seems to have been 
corrupted in transit, aborting!\n",new);
       CfLog(cfverbose,OUTPUT,"");
-      if (rename(backup,dest) == -1)
+      if (backupok)
         {
+        rename(backup,dest);   /* ignore failure */
         }
       return false;
       }
@@ -1621,7 +1634,10 @@
    {
    snprintf(OUTPUT,bufsize*2,"Problem: could not install copy file as %s, 
directory in the way?\n",dest);
    CfLog(cferror,OUTPUT,"rename");
-   rename(backup,dest);
+   if (backupok)
+      {
+      rename(backup,dest);     /* ignore failure */
+      }
    return false;
    }