From 93c590e9e8ae2d9ba1705937d11632f8f7e3a786 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Sat, 24 Dec 2016 15:03:51 +0000 Subject: [PATCH] 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 (cherry picked from commit c1120bbbe8fdea20cf64fa12247f4f4a4006c834) --- 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 ++-- 7 files changed, 14 insertions(+), 13 deletions(-) diff --git a/IkiWiki/Plugin/attachment.pm b/IkiWiki/Plugin/attachment.pm index aea70429d..ee015ebdf 100644 --- a/IkiWiki/Plugin/attachment.pm +++ b/IkiWiki/Plugin/attachment.pm @@ -163,7 +163,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 285013e49..af1ac5ec7 100644 --- a/IkiWiki/Plugin/comments.pm +++ b/IkiWiki/Plugin/comments.pm @@ -524,11 +524,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, @@ -568,7 +569,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 d15607990..c7c29d453 100644 --- a/IkiWiki/Plugin/editpage.pm +++ b/IkiWiki/Plugin/editpage.pm @@ -428,7 +428,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 2c1775f2e..f401c0311 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 346515e23..fe1da764a 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); }, ); } @@ -390,7 +390,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 53e6af92f..4db65e23e 100644 --- a/IkiWiki/Plugin/po.pm +++ b/IkiWiki/Plugin/po.pm @@ -542,7 +542,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 8387a1e32..4b2d1c4a9 100644 --- a/IkiWiki/Plugin/rename.pm +++ b/IkiWiki/Plugin/rename.pm @@ -258,7 +258,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"); @@ -311,7 +311,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'); -- 2.39.5