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)

diff --git a/IkiWiki/Plugin/attachment.pm b/IkiWiki/Plugin/attachment.pm
index 9bac96fc6e0fe59363200b90bc49dd39eccd8dcf..04100e0fd018a5e60e2e302463398636df07135a 100644 (file)
--- 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 c5177833f2430d28860e26bdeca1356c6b7a03eb..aa9b49c8c3da621b2734cfee9b7bfb044b297cde 100644 (file)
--- a/IkiWiki/Plugin/comments.pm
+++ b/IkiWiki/Plugin/comments.pm
@@ -555,11 +555,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,
@@ -599,7 +600,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 78d0704c7fd3b699acbb65df391e475b29fae52b..fad7ecc5ac5f04a82aa368fc5755e2f105a2e212 100644 (file)
--- a/IkiWiki/Plugin/editpage.pm
+++ b/IkiWiki/Plugin/editpage.pm
@@ -430,7 +430,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 b50a22a00c2e0bc6179a21b22b987d8241282afb..079bb10d4fb7eda121916976ddd0945934a4557b 100644 (file)
--- 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 346515e23094e180f73b50cfd9d90c67f06c7a32..fe1da764ae9dc07a2431aaca78478cc9364b82e4 100644 (file)
--- 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 6107a4a2252c256e715a3b0bd210eb28d45371b3..1528f235ff82282022332af0f55c4664b06bf013 100644 (file)
--- 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 6d56340b896519e921e9b6c7d8a06ffd1d56fe2a..2456c22cb12af55f273ad32e820e728db6b2e7c7 100644 (file)
--- 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');