Re: When to call CheckCopiedFile() ?

[Mark . Burgess]

Patch applied.
M

On 17 Dec, address@hidden wrote:
> 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;
>     }
>  
> 
> 
> 
> 
> _______________________________________________
> Bug-cfengine mailing list
> address@hidden
> http://mail.gnu.org/mailman/listinfo/bug-cfengine



~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Work: +47 22453272            Email:  address@hidden
Fax : +47 22453205            WWW  :  http://www.iu.hio.no/~mark
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~