Re: Possibly unsecure specfile code...

[Jamie C. <jca...@we...>]

jam...@te... wrote:

> Hi Jamie,
> 
> I ran accross the follow code snippet in the %pre section of the specfile:
> 
>      perl <<EOD;
>      # maketemp.pl
>      # Create the /tmp/.webmin directory if needed
> 
>      \$tmp_dir = "/tmp/.webmin";
> 
>      if (!-d \$tmp_dir) {
>              mkdir(\$tmp_dir, 0755) || exit 1;
>              chown(\$<, \$(, \$tmp_dir);
>              chmod(0755, \$tmp_dir);
>              }
>      @st = stat(\$tmp_dir);
>      if (@st && \$st[4] == \$< && \$st[5] == \$( && \$st[2] & 0x4000 &&
>          (\$st[2] & 0777) == 0755) {
>              exit 0;
>              }
>      else {
>              exit 1;
>              }
> 
> 
>      EOD
> 
> The main issue I saw was that it does not seem to deal with the possibility that the file
> exists and is a symbolic link to somewhere the admin would prefer it not point to.  Here is
> code I think handles this situation:
> 
> 
>      perl <<EOD;
>      # maketemp.pl
>      # Create the /tmp/.webmin directory if needed
> 
>      \$tmp_dir = "/tmp/.webmin";
> 
>      #
>      # Only create it if it does not exist
>      if (! -e \$tmp_dir) {
>              mkdir(\$tmp_dir, 0755) || exit 1;
>              chown(\$<, \$(, \$tmp_dir) || exit 1;
>              chmod(0755, \$tmp_dir) || exit 1;
>              }
>      else {
>           #
>           # It does exist...make sure its a directory
>           if ( -d \$tmp_dir ) {
>                #
>                # Now lets veirify it not a sybolic link:
>                exit(1) if( -l \$tmp_dir);
>                }
>           else {
>                #
>                # Its not a directory...we just need to exit
>                exit(1);
>                }
>           }
>      @st = stat(\$tmp_dir);
>      if (@st && \$st[4] == \$< && \$st[5] == \$( && \$st[2] & 0x4000 &&
>          (\$st[2] & 0777) == 0755) {
>              exit 0;
>              }
>      else {
>              exit 1;
>              }
> 
> 
>      EOD


Thanks .. you are correct that it doesn't handle nasty symlinks.

However, I found that just changing the stat() to an lstat() fixed

the problem.


> In my spec file (I am converting yours to suit our purposes more specifically) I simply used shell
> script to do the same logic, but I figured you were trying to be paranoid when double checking the
> perms after you had set them.  I am curious though, you tell it to verify the ownership against the
> real UID rather than the effective UID. Also, you could have compared it against root (UID 0)
> as RPMS are usually installed as root.   Any reason in particular you choose that path?

The only reason it doesn't compare ownership against root is that the same code is used in
setup.sh to setup the temp directory, and it is possible that someone might want to install
webmin as a non-root user. Since RPMs are always installed as root, $< will always be 0.

  - Jamie