From f91c2c6d77d0c4a54d84adcbf208be40bd324238 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 21 Jul 2008 18:13:03 -0400 Subject: response --- doc/todo/Make_example_setup_file_consistent.mdwn | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'doc') diff --git a/doc/todo/Make_example_setup_file_consistent.mdwn b/doc/todo/Make_example_setup_file_consistent.mdwn index c4157816e..1fdff7b0f 100644 --- a/doc/todo/Make_example_setup_file_consistent.mdwn +++ b/doc/todo/Make_example_setup_file_consistent.mdwn @@ -20,3 +20,10 @@ I think things could be improved if a clear decision was made here. Most of the svnpath => "trunk", #default What do others think? + +> I agree, and I'll take a patch. +> +> I may not work on it myself, since I have some +> [[interesting_ideas|online_configuration]] that would let ikiwiki +> generate a setup file for you, rather than having to keep maintain the +> current example. --[[Joey]] -- cgit v1.2.3 From fda61c9349f6420f331c8c424177f2b1f3c04100 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 21 Jul 2008 18:20:55 -0400 Subject: response --- doc/todo/cas_authentication.mdwn | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) (limited to 'doc') diff --git a/doc/todo/cas_authentication.mdwn b/doc/todo/cas_authentication.mdwn index ab523001c..a6b428207 100644 --- a/doc/todo/cas_authentication.mdwn +++ b/doc/todo/cas_authentication.mdwn @@ -14,6 +14,13 @@ follows) ? --[[/users/bbb]] +> Inline here is ok; git-am by mail is ok; a git repo I can pull from also +> ok. +> +> This looks pretty acceptable as-is, but you need to put a copyright and +> license statement at the top. I have a few questions that I'll insert +> inline with the patch below. --[[Joey]] + ------------------------------------------------------------------------------ diff --git a/IkiWiki/Plugin/cas.pm b/IkiWiki/Plugin/cas.pm new file mode 100644 @@ -29,17 +36,31 @@ follows) ? +use strict; +use IkiWiki 2.00; +use AuthCAS; # http://search.cpan.org/~osalaun/AuthCAS-1.3.1/ + +> In ikiwiki we generally deman-load perl modules only when they're used. +> This avoids loading expensive modules when the CGI isn't doing +> authentication. Can you do that with AuthCAS? Something like this before +> the use of it: `eval q{use AuthCAS}; error $@ if $@` + + +sub import { #{{{ + hook(type => "getopt", id => "cas", call => \&getopt); + hook(type => "auth", id => "cas", call => \&auth); + hook(type => "formbuilder_setup", id => "cas", call => \&formbuilder_setup); +} # }}} - + + +> Could you please use tabs for indentation of program flow? + +# FIXME: We should check_config to ensure that : +# * cas_url and ca_file are present + +> Please fix that.. + +# * no other auth plugin are present (at least passwordauth and openid) - + + +> Why would you want to make other auth plugins not work? Could a site not +> legitimatly chose to use this and another auth method? + +sub getopt () { #{{{ + eval q{use Getopt::Long}; + error($@) if $@; @@ -130,13 +151,20 @@ follows) ? +into the wiki. + +The plugin needs the [[!cpan AuthCAS-1.3.1]] perl module. + +> Does it really need that specific version? I think you should lose the +> version part. + + +This plugin has two mandatory configuration option. You **must** set `--cas_url` +to the url of a server offering CAS 2.0 authentication. You must also set the +`--ca_file` to an absolute path to the file containing CA certificates used by +the server (generally, aka under Debian, fixing that value to +`/etc/ssl/certs/ca-certificates.crt` is sufficient). - + + +> It would be good to add commented-out examples of these to +> [[ikiwiki.setup]] as well. + +This plugin is not enabled by default. It can not be used with other +authentication plugin, such as [[passwordauth]] or [[openid]]. -- cgit v1.2.3 From e630e7507ea253680750e670d7d213bc5ca3e57a Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 21 Jul 2008 18:26:14 -0400 Subject: Avoid troublesome abs_path calls in wrapper setup As documented in the forum post. --- IkiWiki/Wrapper.pm | 8 ++++---- debian/changelog | 1 + doc/forum/Allow_overriding_of_symlink_restriction.mdwn | 4 +--- 3 files changed, 6 insertions(+), 7 deletions(-) (limited to 'doc') diff --git a/IkiWiki/Wrapper.pm b/IkiWiki/Wrapper.pm index 79b9eb3e3..6dc25403e 100644 --- a/IkiWiki/Wrapper.pm +++ b/IkiWiki/Wrapper.pm @@ -4,14 +4,14 @@ package IkiWiki; use warnings; use strict; -use Cwd q{abs_path}; +use File::Spec; use Data::Dumper; use IkiWiki; sub gen_wrapper () { #{{{ - $config{srcdir}=abs_path($config{srcdir}); - $config{destdir}=abs_path($config{destdir}); - my $this=abs_path($0); + $config{srcdir}=File::Spec->rel2abs($config{srcdir}); + $config{destdir}=File::Spec->rel2abs($config{destdir}); + my $this=File::Spec->rel2abs($0); if (! -x $this) { error(sprintf(gettext("%s doesn't seem to be executable"), $this)); } diff --git a/debian/changelog b/debian/changelog index 86a770357..ca318e815 100644 --- a/debian/changelog +++ b/debian/changelog @@ -7,6 +7,7 @@ ikiwiki (2.55) UNRELEASED; urgency=low (Simon McVittie) * Really fix bug with links to pages with names containing colons. Previous fix mised a few cases. + * Avoid troublesome abs_path calls in wrapper setup. -- Joey Hess Mon, 21 Jul 2008 11:35:46 -0400 diff --git a/doc/forum/Allow_overriding_of_symlink_restriction.mdwn b/doc/forum/Allow_overriding_of_symlink_restriction.mdwn index bd94811df..069a18f30 100644 --- a/doc/forum/Allow_overriding_of_symlink_restriction.mdwn +++ b/doc/forum/Allow_overriding_of_symlink_restriction.mdwn @@ -132,6 +132,4 @@ like this being accepted before I bothered. >> hrm. I might see if File::Spec->rel2abs( $path ) ; will give absolute an path without expanding symlinks. >>> Patch using rel2abs() works well - it no longer expands symlinks. -> I suppose you could do the same thing with `$this`, but it does not sound -> like it has caused you problems anyway. -> --[[Joey]] +>>>> That patch is applied now. --[[Joey]] -- cgit v1.2.3 From c2a2f715087a4602876618fdec2fad073308a6d5 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 21 Jul 2008 18:33:09 -0400 Subject: Add allow_symlinks_before_srcdir config setting can be used to avoid a security check that is a good safe default, but problimatic overkill in some situations. I decided to underdocument this, because the option looks ugly, and I don't want people randomly turning it on because it looks like a good idea. So if you need it, you'll get an error message mentioning how to fix it. --- IkiWiki/Render.pm | 6 +- debian/changelog | 3 + .../Allow_overriding_of_symlink_restriction.mdwn | 139 +++++++++++++++++++++ .../Allow_overriding_of_symlink_restriction.mdwn | 135 -------------------- 4 files changed, 145 insertions(+), 138 deletions(-) create mode 100644 doc/bugs/Allow_overriding_of_symlink_restriction.mdwn delete mode 100644 doc/forum/Allow_overriding_of_symlink_restriction.mdwn (limited to 'doc') diff --git a/IkiWiki/Render.pm b/IkiWiki/Render.pm index fc1bc0c92..ab3ccd7ae 100644 --- a/IkiWiki/Render.pm +++ b/IkiWiki/Render.pm @@ -245,11 +245,11 @@ sub prune ($) { #{{{ } #}}} sub refresh () { #{{{ - # security check, avoid following symlinks in the srcdir path + # security check, avoid following symlinks in the srcdir path by default my $test=$config{srcdir}; while (length $test) { - if (-l $test) { - error("symlink found in srcdir path ($test)"); + if (-l $test && ! $config{allow_symlinks_before_srcdir}) { + error("symlink found in srcdir path ($test) -- set allow_symlinks_before_srcdir to allow this"); } unless ($test=~s/\/+$//) { $test=dirname($test); diff --git a/debian/changelog b/debian/changelog index ca318e815..7ab18a2c7 100644 --- a/debian/changelog +++ b/debian/changelog @@ -8,6 +8,9 @@ ikiwiki (2.55) UNRELEASED; urgency=low * Really fix bug with links to pages with names containing colons. Previous fix mised a few cases. * Avoid troublesome abs_path calls in wrapper setup. + * Add allow_symlinks_before_srcdir config setting that can be used to avoid + a security check that is a good safe default, but problimatic overkill in + some situations. -- Joey Hess Mon, 21 Jul 2008 11:35:46 -0400 diff --git a/doc/bugs/Allow_overriding_of_symlink_restriction.mdwn b/doc/bugs/Allow_overriding_of_symlink_restriction.mdwn new file mode 100644 index 000000000..69ea299e8 --- /dev/null +++ b/doc/bugs/Allow_overriding_of_symlink_restriction.mdwn @@ -0,0 +1,139 @@ +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. + +I think there are two issues here: + + - Symlinks with the source dir path itself, and + - Symlinks inside the source directory. + +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. + +> Correct. However, where does the revision controlled source directory end? Ikiwiki has no way +> of knowing. It cannot assume that `srcdir` is in revision control, and +> everything outside is not. For example, ikiwiki's own source tree has the +> doc wiki source inside `ikiwiki/doc`. So to fully close the source dir +> symlink issue, it's best to, by default, assume that the revision +> controlled directories could go down arbitrarily deep, down to the root of +> the filesystem. --[[Joey]] + +>> Fair point. + +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. + +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. + +> BTW, if you have a problem, please file it in [[todo]] or [[bugs]] in the +> future. Especially if you also have a patch. :-) --[[Joey]] + +>> 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 :) + +Is there a huge objection to this patch? + +>>> [[patch]] updated. + + diff --git a/IkiWiki/Render.pm b/IkiWiki/Render.pm + index 990fcaa..0fb78ba 100644 + --- a/IkiWiki/Render.pm + +++ b/IkiWiki/Render.pm + @@ -260,13 +260,15 @@ sub prune ($) { #{{{ + + sub refresh () { #{{{ + # security check, avoid following symlinks in the srcdir path + - my $test=$config{srcdir}; + - while (length $test) { + - if (-l $test) { + - error("symlink found in srcdir path ($test)"); + - } + - unless ($test=~s/\/+$//) { + - $test=dirname($test); + + if (! $config{allow_insecure_symlinks_in_path_to_srcdir}) { + + my $test=$config{srcdir}; + + while (length $test) { + + if (-l $test) { + + error("symlink found in srcdir path ($test)"); + + } + + unless ($test=~s/\/+$//) { + + $test=dirname($test); + + } + } + } + + diff --git a/doc/ikiwiki.setup b/doc/ikiwiki.setup + index 10cb3da..eb86e49 100644 + --- a/doc/ikiwiki.setup + +++ b/doc/ikiwiki.setup + @@ -203,4 +203,10 @@ use IkiWiki::Setup::Standard { + # For use with the attachment plugin, a program that returns + # nonzero if its standard input contains an virus. + #virus_checker => "clamdscan -", + + + + # The following setting allows symlinks in the path to your + + # srcdir. Symlinks are still not followed within srcdir. + + # Allowing symlinks to be followed, even in the path to srcdir, + + # will make some setups insecure. + + #allow_insecure_symlinks_in_path_to_srcdir => 0, + } + +> No, I don't have a big objection to such an option, as long as it's +> extremely well documented that it will make many setups insecure. +> It would be nice to come up with an option name that makes clear that +> it's allowing symlinks in the path to the `srcdir`, but still not inside +> the `srcdir`. +> --[[Joey]] + +>> Slightly modified version of patch applied. --[[Joey]] + +>> Ok, I'll try to get it cleaned up and documented. + +There is a second location where this can be an issue. That is in the +front of the wrapper. There the issue is that the path to the source dir +as seen on the cgi server and on the git server are different - each has +symlinks in place to support the other. The current wrapper gets the +absolute path to the source dir, and that breaks things for me. This is a +slightly different, albeit related, issue to the one above. The following +patch fixes things. Again, patch inline. Again, this patch could be +cleaned up :). I just wanted to see if there was any chance of a patch +like this being accepted before I bothered. + +>>> Patch updated: + + index 79b9eb3..ce1c395 100644 + --- a/IkiWiki/Wrapper.pm + +++ b/IkiWiki/Wrapper.pm + @@ -4,14 +4,14 @@ package IkiWiki; + + use warnings; + use strict; + -use Cwd q{abs_path}; + use Data::Dumper; + use IkiWiki; + +use File::Spec; + + sub gen_wrapper () { #{{{ + - $config{srcdir}=abs_path($config{srcdir}); + - $config{destdir}=abs_path($config{destdir}); + - my $this=abs_path($0); + + $config{srcdir}=File::Spec->rel2abs($config{srcdir}); + + $config{destdir}=File::Spec->rel2abs($config{destdir}); + + my $this=File::Spec->rel2abs($0); + if (! -x $this) { + error(sprintf(gettext("%s doesn't seem to be executable"), $this + } + +> ikiwiki uses absolute paths for `srcdir`, `destdir` and `this` because +> the wrapper could be run from any location, and if any of them happen to +> be a relative path, it would crash and burn. + +>> Which makes perfect sense. It is annoying that abs_path() is also +>> expanding symlinks. + +> I think the thing to do might be to make it check if `srcdir` and +> `destdir` look like an absolute path (ie, start with "/"). If so, it can +> skip running `abs_path` on them. + +>> I'll do that. I assume something like File::Spec->file_name_is_absolute( $path ); would have more cross-platformy goodness. +>> hrm. I might see if File::Spec->rel2abs( $path ) ; will give absolute an path without expanding symlinks. +>>> Patch using rel2abs() works well - it no longer expands symlinks. + +>>>> That patch is applied now. --[[Joey]] + +[[tag done]] diff --git a/doc/forum/Allow_overriding_of_symlink_restriction.mdwn b/doc/forum/Allow_overriding_of_symlink_restriction.mdwn deleted file mode 100644 index 069a18f30..000000000 --- a/doc/forum/Allow_overriding_of_symlink_restriction.mdwn +++ /dev/null @@ -1,135 +0,0 @@ -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. - -I think there are two issues here: - - - Symlinks with the source dir path itself, and - - Symlinks inside the source directory. - -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. - -> Correct. However, where does the revision controlled source directory end? Ikiwiki has no way -> of knowing. It cannot assume that `srcdir` is in revision control, and -> everything outside is not. For example, ikiwiki's own source tree has the -> doc wiki source inside `ikiwiki/doc`. So to fully close the source dir -> symlink issue, it's best to, by default, assume that the revision -> controlled directories could go down arbitrarily deep, down to the root of -> the filesystem. --[[Joey]] - ->> Fair point. - -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. - -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. - -> BTW, if you have a problem, please file it in [[todo]] or [[bugs]] in the -> future. Especially if you also have a patch. :-) --[[Joey]] - ->> 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 :) - -Is there a huge objection to this patch? - ->>> [[patch]] updated. - - diff --git a/IkiWiki/Render.pm b/IkiWiki/Render.pm - index 990fcaa..0fb78ba 100644 - --- a/IkiWiki/Render.pm - +++ b/IkiWiki/Render.pm - @@ -260,13 +260,15 @@ sub prune ($) { #{{{ - - sub refresh () { #{{{ - # security check, avoid following symlinks in the srcdir path - - my $test=$config{srcdir}; - - while (length $test) { - - if (-l $test) { - - error("symlink found in srcdir path ($test)"); - - } - - unless ($test=~s/\/+$//) { - - $test=dirname($test); - + if (! $config{allow_insecure_symlinks_in_path_to_srcdir}) { - + my $test=$config{srcdir}; - + while (length $test) { - + if (-l $test) { - + error("symlink found in srcdir path ($test)"); - + } - + unless ($test=~s/\/+$//) { - + $test=dirname($test); - + } - } - } - - diff --git a/doc/ikiwiki.setup b/doc/ikiwiki.setup - index 10cb3da..eb86e49 100644 - --- a/doc/ikiwiki.setup - +++ b/doc/ikiwiki.setup - @@ -203,4 +203,10 @@ use IkiWiki::Setup::Standard { - # For use with the attachment plugin, a program that returns - # nonzero if its standard input contains an virus. - #virus_checker => "clamdscan -", - + - + # The following setting allows symlinks in the path to your - + # srcdir. Symlinks are still not followed within srcdir. - + # Allowing symlinks to be followed, even in the path to srcdir, - + # will make some setups insecure. - + #allow_insecure_symlinks_in_path_to_srcdir => 0, - } - -> No, I don't have a big objection to such an option, as long as it's -> extremely well documented that it will make many setups insecure. -> It would be nice to come up with an option name that makes clear that -> it's allowing symlinks in the path to the `srcdir`, but still not inside -> the `srcdir`. -> --[[Joey]] - ->> Ok, I'll try to get it cleaned up and documented. - -There is a second location where this can be an issue. That is in the -front of the wrapper. There the issue is that the path to the source dir -as seen on the cgi server and on the git server are different - each has -symlinks in place to support the other. The current wrapper gets the -absolute path to the source dir, and that breaks things for me. This is a -slightly different, albeit related, issue to the one above. The following -patch fixes things. Again, patch inline. Again, this patch could be -cleaned up :). I just wanted to see if there was any chance of a patch -like this being accepted before I bothered. - ->>> Patch updated: - - index 79b9eb3..ce1c395 100644 - --- a/IkiWiki/Wrapper.pm - +++ b/IkiWiki/Wrapper.pm - @@ -4,14 +4,14 @@ package IkiWiki; - - use warnings; - use strict; - -use Cwd q{abs_path}; - use Data::Dumper; - use IkiWiki; - +use File::Spec; - - sub gen_wrapper () { #{{{ - - $config{srcdir}=abs_path($config{srcdir}); - - $config{destdir}=abs_path($config{destdir}); - - my $this=abs_path($0); - + $config{srcdir}=File::Spec->rel2abs($config{srcdir}); - + $config{destdir}=File::Spec->rel2abs($config{destdir}); - + my $this=File::Spec->rel2abs($0); - if (! -x $this) { - error(sprintf(gettext("%s doesn't seem to be executable"), $this - } - -> ikiwiki uses absolute paths for `srcdir`, `destdir` and `this` because -> the wrapper could be run from any location, and if any of them happen to -> be a relative path, it would crash and burn. - ->> Which makes perfect sense. It is annoying that abs_path() is also ->> expanding symlinks. - -> I think the thing to do might be to make it check if `srcdir` and -> `destdir` look like an absolute path (ie, start with "/"). If so, it can -> skip running `abs_path` on them. - ->> I'll do that. I assume something like File::Spec->file_name_is_absolute( $path ); would have more cross-platformy goodness. ->> hrm. I might see if File::Spec->rel2abs( $path ) ; will give absolute an path without expanding symlinks. ->>> Patch using rel2abs() works well - it no longer expands symlinks. - ->>>> That patch is applied now. --[[Joey]] -- cgit v1.2.3