From c1120bbbe8fdea20cf64fa12247f4f4a4006c834 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Sat, 24 Dec 2016 15:03:51 +0000 Subject: Force CGI::FormBuilder->field to scalar context where necessary CGI::FormBuilder->field has behaviour similar to the CGI.pm misfeature we avoided in f4ec7b0. Force it into scalar context where it is used in an argument list. This prevents two (relatively minor) commit metadata forgery vulnerabilities: * In the comments plugin, an attacker who was able to post a comment could give it a user-specified author and author-URL even if the wiki configuration did not allow for that, by crafting multiple values to other fields. * In the editpage plugin, an attacker who was able to edit a page could potentially forge commit authorship by crafting multiple values for the rcsinfo field. The remaining plugins changed in this commit appear to have been protected by use of explicit scalar prototypes for the called functions, but have been changed anyway to make them more obviously correct. In particular, checkpassword() in passwordauth has a known prototype, so an attacker cannot trick it into treating multiple values of the name field as being the username, password and field to check for. OVE-20161226-0001 --- IkiWiki/Plugin/attachment.pm | 2 +- IkiWiki/Plugin/comments.pm | 11 ++++++----- IkiWiki/Plugin/editpage.pm | 2 +- IkiWiki/Plugin/notifyemail.pm | 2 +- IkiWiki/Plugin/passwordauth.pm | 4 ++-- IkiWiki/Plugin/po.pm | 2 +- IkiWiki/Plugin/rename.pm | 4 ++-- debian/changelog | 5 +++++ doc/security.mdwn | 22 ++++++++++++++++++++++ 9 files changed, 41 insertions(+), 13 deletions(-) diff --git a/IkiWiki/Plugin/attachment.pm b/IkiWiki/Plugin/attachment.pm index e8135a8fd..428b363b6 100644 --- a/IkiWiki/Plugin/attachment.pm +++ b/IkiWiki/Plugin/attachment.pm @@ -165,7 +165,7 @@ sub formbuilder (@) { # Generate the attachment list only after having added any new # attachments. - $form->tmpl_param("attachment_list" => [attachment_list($form->field('page'))]); + $form->tmpl_param("attachment_list" => [attachment_list(scalar $form->field('page'))]); } sub attachment_holding_location { diff --git a/IkiWiki/Plugin/comments.pm b/IkiWiki/Plugin/comments.pm index b47f965e7..0858f69f1 100644 --- a/IkiWiki/Plugin/comments.pm +++ b/IkiWiki/Plugin/comments.pm @@ -557,11 +557,12 @@ sub editcomment ($$) { } $postcomment=1; - my $ok=IkiWiki::check_content(content => $form->field('editcontent'), - subject => $form->field('subject'), + my $ok=IkiWiki::check_content( + content => scalar $form->field('editcontent'), + subject => scalar $form->field('subject'), $config{comments_allowauthor} ? ( - author => $form->field('author'), - url => $form->field('url'), + author => scalar $form->field('author'), + url => scalar $form->field('url'), ) : (), page => $location, cgi => $cgi, @@ -601,7 +602,7 @@ sub editcomment ($$) { length $form->field('subject')) { $message = sprintf( gettext("Added a comment: %s"), - $form->field('subject')); + scalar $form->field('subject')); } IkiWiki::rcs_add($file); diff --git a/IkiWiki/Plugin/editpage.pm b/IkiWiki/Plugin/editpage.pm index 6ca4b589f..99a142914 100644 --- a/IkiWiki/Plugin/editpage.pm +++ b/IkiWiki/Plugin/editpage.pm @@ -431,7 +431,7 @@ sub cgi_editpage ($$) { $conflict=rcs_commit( file => $file, message => $message, - token => $form->field("rcsinfo"), + token => scalar $form->field("rcsinfo"), session => $session, ); enable_commit_hook(); diff --git a/IkiWiki/Plugin/notifyemail.pm b/IkiWiki/Plugin/notifyemail.pm index b50a22a00..079bb10d4 100644 --- a/IkiWiki/Plugin/notifyemail.pm +++ b/IkiWiki/Plugin/notifyemail.pm @@ -34,7 +34,7 @@ sub formbuilder (@) { } elsif ($form->submitted eq "Save Preferences" && $form->validate && defined $form->field("subscriptions")) { - setsubscriptions($username, $form->field('subscriptions')); + setsubscriptions($username, scalar $form->field('subscriptions')); } } diff --git a/IkiWiki/Plugin/passwordauth.pm b/IkiWiki/Plugin/passwordauth.pm index 3bdd9de2e..c966087ce 100644 --- a/IkiWiki/Plugin/passwordauth.pm +++ b/IkiWiki/Plugin/passwordauth.pm @@ -231,7 +231,7 @@ sub formbuilder_setup (@) { $form->field( name => "password", validate => sub { - checkpassword($form->field("name"), shift); + checkpassword(scalar $form->field("name"), shift); }, ); } @@ -395,7 +395,7 @@ sub formbuilder (@) { if ($form->submitted eq "Save Preferences" && $form->validate) { my $user_name=$form->field('name'); if (defined $form->field("password") && length $form->field("password")) { - setpassword($user_name, $form->field('password')); + setpassword($user_name, scalar $form->field('password')); } } } diff --git a/IkiWiki/Plugin/po.pm b/IkiWiki/Plugin/po.pm index 6b55ee351..418e8e58a 100644 --- a/IkiWiki/Plugin/po.pm +++ b/IkiWiki/Plugin/po.pm @@ -548,7 +548,7 @@ sub formbuilder_setup (@) { # their buttons, which is why this hook must be run last. # The canrename/canremove hooks already ensure this is forbidden # at the backend level, so this is only UI sugar. - if (istranslation($form->field("page"))) { + if (istranslation(scalar $form->field("page"))) { map { for (my $i = 0; $i < @{$params{buttons}}; $i++) { if (@{$params{buttons}}[$i] eq $_) { diff --git a/IkiWiki/Plugin/rename.pm b/IkiWiki/Plugin/rename.pm index 4a86d5a09..56dfbd543 100644 --- a/IkiWiki/Plugin/rename.pm +++ b/IkiWiki/Plugin/rename.pm @@ -259,7 +259,7 @@ sub formbuilder (@) { my $session=$params{session}; if ($form->submitted eq "Rename" && $form->field("do") eq "edit") { - rename_start($q, $session, 0, $form->field("page")); + rename_start($q, $session, 0, scalar $form->field("page")); } elsif ($form->submitted eq "Rename Attachment") { my @selected=map { Encode::decode_utf8($_) } $q->param("attachment_select"); @@ -312,7 +312,7 @@ sub sessioncgi ($$) { # performed in check_canrename later. my $srcfile=IkiWiki::possibly_foolish_untaint($pagesources{$src}) if exists $pagesources{$src}; - my $dest=IkiWiki::possibly_foolish_untaint(titlepage($form->field("new_name"))); + my $dest=IkiWiki::possibly_foolish_untaint(titlepage(scalar $form->field("new_name"))); my $destfile=$dest; if (! $q->param("attachment")) { my $type=$q->param('type'); diff --git a/debian/changelog b/debian/changelog index 86d06bdc6..ccf830b27 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,5 +1,10 @@ ikiwiki (3.20161220) UNRELEASED; urgency=medium + * Security: force CGI::FormBuilder->field to scalar context where + necessary, avoiding unintended function argument injection + analogous to CVE-2014-1572. In ikiwiki this could be used to + forge commit metadata, but thankfully nothing more serious. + (OVE-20161226-0001) * Add CVE references for CVE-2016-10026 * Add missing ikiwiki.setup for the manual test for CVE-2016-10026 * git: don't issue a warning if the rcsinfo CGI parameter is undefined diff --git a/doc/security.mdwn b/doc/security.mdwn index 4f825deba..9818e0c94 100644 --- a/doc/security.mdwn +++ b/doc/security.mdwn @@ -563,3 +563,25 @@ which are both used in most ikiwiki installations. This bug was reported on 2016-12-17. The fixed version 3.20161219 was released on 2016-12-19. ([[!cve CVE-2016-10026]]) + +## Commit metadata forgery via CGI::FormBuilder context-dependent APIs + +When CGI::FormBuilder->field("foo") is called in list context (and +in particular in the arguments to a subroutine that takes named +arguments), it can return zero or more values for foo from the CGI +request, rather than the expected single value. This breaks the usual +Perl parsing convention for named arguments, similar to CVE-2014-1572 +in Bugzilla (which was caused by a similar API design issue in CGI.pm). + +In ikiwiki, this appears to have been exploitable in two places, both +of them relatively minor: + +* in the comments plugin, an attacker who was able to post a comment + could give it a user-specified author and author-URL even if the wiki + configuration did not allow for that, by crafting multiple values + for other fields +* in the editpage plugin, an attacker who was able to edit a page + could potentially forge commit authorship (attribute their edit to + someone else) by crafting multiple values for the rcsinfo field + +(OVE-20161226-0001) -- cgit v1.2.3