Fix CSRF attacks against the preferences and edit forms. Closes: #475445

The fix involved embedding the session id in the forms, and not allowing the
forms to be submitted if the embedded id does not match the session id.

In the case of the preferences form, if the session id is not embedded,
then the CGI parameters are cleared. This avoids a secondary attack where the
link to the preferences form prefills password or other fields, and
the user hits "submit" without noticing these prefilled values.

In the case of the editpage form, the anonok plugin can allow anyone to edit,
and so I chose not to guard against CSRF attacks against users who are not
logged in. Otherwise, it also embeds the session id and checks it.

For page editing, I assume that the user will notice if content or commit
message is changed because of CGI parameters, and won't blndly hit save page.
So I didn't block those CGI paramters. (It's even possible to use those CGI
parameters, for good, not for evil, I guess..)

The only other CSRF attack I can think of in ikiwiki involves the poll plugin.
It's certianly possible to set up a link that causes the user to unknowingly
vote in a poll. However, the poll plugin is not intended to be used for things
that people would want to attack, since anyone can after all edit the poll page
and fill in any values they like. So this "attack" is ignorable.

diff --git a/IkiWiki/CGI.pm b/IkiWiki/CGI.pm
index 4706770884ab5a1fb89a171fab2200670334339d..65136a26947a2cae9ee5eeecc52ed092e33b197b 100644 (file)
--- a/IkiWiki/CGI.pm
+++ b/IkiWiki/CGI.pm
@@ -161,8 +161,18 @@ sub cgi_prefs ($$) { #{{{
        my $session=shift;
 
        needsignin($q, $session);
-
        decode_cgi_utf8($q);
+       
+       # The session id is stored on the form and checked to
+       # guard against CSRF.
+       my $sid=$q->param('sid');
+       if (! defined $sid) {
+               $q->delete_all;
+       }
+       elsif ($sid ne $session->id) {
+               error(gettext("Your login session has expired."));
+       }
+
        eval q{use CGI::FormBuilder};
        error($@) if $@;
        my $form = CGI::FormBuilder->new(
@@ -193,7 +203,10 @@ sub cgi_prefs ($$) { #{{{
                        buttons => $buttons);
        });
        
-       $form->field(name => "do", type => "hidden");
+       $form->field(name => "do", type => "hidden", value => "prefs",
+               force => 1);
+       $form->field(name => "sid", type => "hidden", value => $session->id,
+               force => 1);
        $form->field(name => "email", size => 50, fieldset => "preferences");
        $form->field(name => "banned_users", size => 50,
                fieldset => "admin");
@@ -241,11 +254,11 @@ sub cgi_prefs ($$) { #{{{
 sub cgi_editpage ($$) { #{{{
        my $q=shift;
        my $session=shift;
-
-       my @fields=qw(do rcsinfo subpage from page type editcontent comments);
-       my @buttons=("Save Page", "Preview", "Cancel");
        
        decode_cgi_utf8($q);
+       
+       my @fields=qw(do rcsinfo subpage from page type editcontent comments);
+       my @buttons=("Save Page", "Preview", "Cancel");
        eval q{use CGI::FormBuilder};
        error($@) if $@;
        my $form = CGI::FormBuilder->new(
@@ -316,6 +329,8 @@ sub cgi_editpage ($$) { #{{{
        }
 
        $form->field(name => "do", type => 'hidden');
+       $form->field(name => "sid", type => "hidden", value => $session->id,
+               force => 1);
        $form->field(name => "from", type => 'hidden');
        $form->field(name => "rcsinfo", type => 'hidden');
        $form->field(name => "subpage", type => 'hidden');
@@ -474,6 +489,16 @@ sub cgi_editpage ($$) { #{{{
        else {
                # save page
                check_canedit($page, $q, $session);
+       
+               # The session id is stored on the form and checked to
+               # guard against CSRF. But only if the user is logged in,
+               # as anonok can allow anonymous edits.
+               if (defined $session->param("name")) {
+                       my $sid=$q->param('sid');
+                       if (! defined $sid || $sid ne $session->id) {
+                               error(gettext("Your login session has expired."));
+                       }
+               }
 
                my $exists=-e "$config{srcdir}/$file";
 

diff --git a/debian/changelog b/debian/changelog
index 613640f60bcebee54166fec552071440cfceafb8..9085d97cb6c0167186faaf220b9949ea5b857f73 100644 (file)
--- a/debian/changelog
+++ b/debian/changelog
@@ -3,6 +3,9 @@ ikiwiki (2.42) UNRELEASED; urgency=low
   * aggregate: Correct a mistake in the code that dummy up a guid for feeds
     lacking one.
   * inline: Correct handling of urls relative to baseurl in feeds.
+  * Fix CSRF attacks against the preferences and edit forms. The fix involved
+    embedding the session id in the forms, and not allowing the forms to be
+    submitted if the embedded id does not match the session id. Closes: #475445
 
  -- Joey Hess <joeyh@debian.org>  Thu, 03 Apr 2008 02:35:39 -0400
 

diff --git a/doc/security.mdwn b/doc/security.mdwn
index 373f64557508c8b1e6e9e736d409981cdf64c8b5..29ae7d4b3bc30c81e7c70d5b82e8660318c44952 100644 (file)
--- a/doc/security.mdwn
+++ b/doc/security.mdwn
@@ -366,3 +366,14 @@ with the release of ikiwiki 2.31.1. (And a few subsequent versions..)
 A fix was also backported to Debian etch, as version 1.33.4. I recommend
 upgrading to one of these versions if your wiki can be edited by third
 parties.
+
+## Cross Site Request Forging
+
+Cross Site Request Forging could be used to constuct a link that would
+change a logged-in user's password or other preferences if they clicked on
+the link. It could also be used to construct a link that would cause a wiki
+page to be modified by a logged-in user.
+
+These holes were discovered on 10 April 2008 and fixed the same day with
+the release of ikiwiki 2.42. A fix was also backported to Debian etch, as
+version 1.33.4. I recommend upgrading to one of these versions.

diff --git a/po/ikiwiki.pot b/po/ikiwiki.pot
index a3f7cafcbe198ef532f8d7303ce430d79aa8ad47..5e7e4b4d41b686b61e642627008ce9fa985cf267 100644 (file)
--- a/po/ikiwiki.pot
+++ b/po/ikiwiki.pot
@@ -8,7 +8,7 @@ msgid ""
 msgstr ""
 "Project-Id-Version: PACKAGE VERSION\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2008-03-29 21:01-0400\n"
+"POT-Creation-Date: 2008-04-10 16:18-0400\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
@@ -24,46 +24,50 @@ msgstr ""
 msgid "login failed, perhaps you need to turn on cookies?"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:184
+#: ../IkiWiki/CGI.pm:173 ../IkiWiki/CGI.pm:499
+msgid "Your login session has expired."
+msgstr ""
+
+#: ../IkiWiki/CGI.pm:194
 msgid "Login"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:185
+#: ../IkiWiki/CGI.pm:195
 msgid "Preferences"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:186
+#: ../IkiWiki/CGI.pm:196
 msgid "Admin"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:235
+#: ../IkiWiki/CGI.pm:248
 msgid "Preferences saved."
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:293
+#: ../IkiWiki/CGI.pm:306
 #, perl-format
 msgid "%s is not an editable page"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:395 ../IkiWiki/Plugin/brokenlinks.pm:24
+#: ../IkiWiki/CGI.pm:410 ../IkiWiki/Plugin/brokenlinks.pm:24
 #: ../IkiWiki/Plugin/inline.pm:265 ../IkiWiki/Plugin/opendiscussion.pm:17
 #: ../IkiWiki/Plugin/orphans.pm:28 ../IkiWiki/Render.pm:95
 #: ../IkiWiki/Render.pm:172
 msgid "discussion"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:451
+#: ../IkiWiki/CGI.pm:466
 #, perl-format
 msgid "creating %s"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:469 ../IkiWiki/CGI.pm:487 ../IkiWiki/CGI.pm:497
-#: ../IkiWiki/CGI.pm:531 ../IkiWiki/CGI.pm:576
+#: ../IkiWiki/CGI.pm:484 ../IkiWiki/CGI.pm:512 ../IkiWiki/CGI.pm:522
+#: ../IkiWiki/CGI.pm:556 ../IkiWiki/CGI.pm:601
 #, perl-format
 msgid "editing %s"
 msgstr ""
 
-#: ../IkiWiki/CGI.pm:666
+#: ../IkiWiki/CGI.pm:691
 msgid "You are banned."
 msgstr ""
 
@@ -222,7 +226,7 @@ msgstr ""
 msgid "Discussion"
 msgstr ""
 
-#: ../IkiWiki/Plugin/inline.pm:491
+#: ../IkiWiki/Plugin/inline.pm:500
 msgid "RPC::XML::Client not found, not pinging"
 msgstr ""
 

diff --git a/templates/editpage.tmpl b/templates/editpage.tmpl
index cf4950ead9210fe42cdf126b4299cba1f55dccd4..b0bb0ecb99ceb52b4b7214f214aa5660a64dae53 100644 (file)
--- a/templates/editpage.tmpl
+++ b/templates/editpage.tmpl
@@ -41,6 +41,7 @@ together before saving.
 </TMPL_IF>
 <TMPL_VAR FORM-START>
 <TMPL_VAR FIELD-DO>
+<TMPL_VAR FIELD-SID>
 <TMPL_VAR FIELD-FROM>
 <TMPL_VAR FIELD-RCSINFO>
 <TMPL_VAR FIELD-NEWFILE>