git.oblomov.eu Git - ikiwiki/blob - doc/bugs/Allow_overriding_of_symlink_restriction.mdwn

   1 There is currently a restriction in ikiwiki that there cannot be any symlinks in the source path.  This is to deal with a security issue discussed [[here|security#index29h2]].  The issue, as I understand it, is that someone might change a symlink and so cause things on the server to be published when the server admin doesn't want them to be.
   2
   3 I think there are two issues here:
   4
   5  - Symlinks with the source dir path itself, and
   6  - Symlinks inside the source directory.
   7
   8 The first appears to me to be less of a security issue.  If there is a way for a malicious person to change where that path points, then you have problems this check isn't going to solve.  The second is quite clearly a security issue - if someone were to commit a symlink into the source dir they could cause lots of stuff to be published that shouldn't be.
   9
  10 > Correct. However, where does the revision controlled source directory end? Ikiwiki has no way
  11 > of knowing. It cannot assume that `srcdir` is in revision control, and
  12 > everything outside is not. For example, ikiwiki's own source tree has the
  13 > doc wiki source inside `ikiwiki/doc`. So to fully close the source dir
  14 > symlink issue, it's best to, by default, assume that the revision
  15 > controlled directories could go down arbitrarily deep, down to the root of
  16 > the filesystem. --[[Joey]]
  17
  18 >> Fair point.
  19
  20 The current code seems to check this constraint at the top of IkiWiki/Render.pm at the start of refresh().  It seems to only check the source dir itself, not the subdirs.  Then it uses File::Find to recuse which doesn't follow symlinks.
  21
  22 Now my problem:  I have a hosted server where I cannot avoid having a symlink in the source path.  I've made a patch to optionally turn off the symlink checking in the source path itself.  The patch would still not follow symlinks inside the source dir.  This would seem to be ok security-wise for me as I know that path is ok and it isn't going to change on me.
  23
  24 > BTW, if you have a problem, please file it in [[todo]] or [[bugs]] in the
  25 > future. Especially if you also have a patch. :-) --[[Joey]]
  26
  27 >> Well, I was unsure I wasn't missing something.  I wanted to discuss the concept of the patch as much as submit the patch.  But, ok :)
  28
  29 Is there a huge objection to this patch?
  30
  31 >>> [[patch]] updated.
  32
  33     diff --git a/IkiWiki/Render.pm b/IkiWiki/Render.pm
  34     index 990fcaa..0fb78ba 100644
  35     --- a/IkiWiki/Render.pm
  36     +++ b/IkiWiki/Render.pm
  37     @@ -260,13 +260,15 @@ sub prune ($) {
  38
  39      sub refresh () {
  40         # security check, avoid following symlinks in the srcdir path
  41     -   my $test=$config{srcdir};
  42     -   while (length $test) {
  43     -           if (-l $test) {
  44     -                   error("symlink found in srcdir path ($test)");
  45     -           }
  46     -           unless ($test=~s/\/+$//) {
  47     -                   $test=dirname($test);
  48     +   if (! $config{allow_insecure_symlinks_in_path_to_srcdir}) {
  49     +           my $test=$config{srcdir};
  50     +           while (length $test) {
  51     +                   if (-l $test) {
  52     +                           error("symlink found in srcdir path ($test)");
  53     +                   }
  54     +                   unless ($test=~s/\/+$//) {
  55     +                           $test=dirname($test);
  56     +                   }
  57                 }
  58         }
  59
  60     diff --git a/doc/ikiwiki.setup b/doc/ikiwiki.setup
  61     index 10cb3da..eb86e49 100644
  62     --- a/doc/ikiwiki.setup
  63     +++ b/doc/ikiwiki.setup
  64     @@ -203,4 +203,10 @@ use IkiWiki::Setup::Standard {
  65         # For use with the attachment plugin, a program that returns
  66         # nonzero if its standard input contains an virus.
  67         #virus_checker => "clamdscan -",
  68     +
  69     +   # The following setting allows symlinks in the path to your
  70     +   # srcdir.  Symlinks are still not followed within srcdir.
  71     +   # Allowing symlinks to be followed, even in the path to srcdir,
  72     +   # will make some setups insecure.
  73     +   #allow_insecure_symlinks_in_path_to_srcdir => 0,
  74      }
  75
  76 > No, I don't have a big objection to such an option, as long as it's
  77 > extremely well documented that it will make many setups insecure.
  78 > It would be nice to come up with an option name that makes clear that
  79 > it's allowing symlinks in the path to the `srcdir`, but still not inside
  80 > the `srcdir`.
  81 > --[[Joey]]
  82
  83 >> Slightly modified version of patch applied. --[[Joey]]
  84
  85 >> Ok, I'll try to get it cleaned up and documented.
  86
  87 There is a second location where this can be an issue.  That is in the
  88 front of the wrapper.  There the issue is that the path to the source dir
  89 as seen on the cgi server and on the git server are different - each has
  90 symlinks in place to support the other.  The current wrapper gets the
  91 absolute path to the source dir, and that breaks things for me.  This is a
  92 slightly different, albeit related, issue to the one above.  The following
  93 patch fixes things.  Again, patch inline.  Again, this patch could be
  94 cleaned up :).  I just wanted to see if there was any chance of a patch
  95 like this being accepted before I bothered.
  96
  97 >>> Patch updated:
  98
  99     index 79b9eb3..ce1c395 100644
 100     --- a/IkiWiki/Wrapper.pm
 101     +++ b/IkiWiki/Wrapper.pm
 102     @@ -4,14 +4,14 @@ package IkiWiki;
 103
 104      use warnings;
 105      use strict;
 106     -use Cwd q{abs_path};
 107      use Data::Dumper;
 108      use IkiWiki;
 109     +use File::Spec;
 110
 111      sub gen_wrapper () {
 112     -       $config{srcdir}=abs_path($config{srcdir});
 113     -       $config{destdir}=abs_path($config{destdir});
 114     -       my $this=abs_path($0);
 115     +       $config{srcdir}=File::Spec->rel2abs($config{srcdir});
 116     +       $config{destdir}=File::Spec->rel2abs($config{destdir});
 117     +       my $this=File::Spec->rel2abs($0);
 118             if (! -x $this) {
 119                     error(sprintf(gettext("%s doesn't seem to be executable"), $this
 120             }
 121
 122 > ikiwiki uses absolute paths for `srcdir`, `destdir` and `this` because
 123 > the wrapper could be run from any location, and if any of them happen to
 124 > be a relative path, it would crash and burn.
 125
 126 >> Which makes perfect sense.  It is annoying that abs_path() is also
 127 >> expanding symlinks.
 128
 129 > I think the thing to do might be to make it check if `srcdir` and
 130 > `destdir` look like an absolute path (ie, start with "/"). If so, it can
 131 > skip running `abs_path` on them.
 132
 133 >> I'll do that.  I assume something like <code> File::Spec->file_name_is_absolute( $path ); </code> would have more cross-platformy goodness.
 134 >> hrm.  I might see if <code> File::Spec->rel2abs( $path ) ; </code> will give absolute an path without expanding symlinks.
 135 >>> Patch using rel2abs() works well - it no longer expands symlinks.
 136
 137 >>>> That patch is applied now. --[[Joey]]
 138
 139 [[!tag done]]