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 aea70429d0bcc3bc86a776ba8b2dde8352483a37..ee015ebdf8261fd3acf12324deb4e736a065353a 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 285013e49175c8651055bb53e34e71e44115699c..af1ac5ec78358af3484eed0f91f3bdc8082b1940 100644 (file)
--- 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 d15607990db2412b8a14f281864f82f98b3cac7f..c7c29d45357a4d3c78692b7eda90109a07d2ecdc 100644 (file)
--- 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 2c1775f2e81df281a7c7769220acc26ade476cdf..f401c0311aba9ea4c2183498cc61a494839c8953 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 53e6af92f1db14c668f287724aed7b8b12c6f76a..4db65e23e53795d4cf562a38863fd814e27091db 100644 (file)
--- 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 8387a1e32446753f749887ba5655d23c2dfce462..4b2d1c4a93bf62ace9389787c5f1526c72c10850 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');