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

diff --git a/IkiWiki/Plugin/attachment.pm b/IkiWiki/Plugin/attachment.pm
index e8135a8fd8be8abc25e05a1bacb0e142e9108a71..428b363b61abe5d13379ed8cb9187c879077c21c 100644 (file)
--- 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.
        
        # 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 {
 }
 
 sub attachment_holding_location {

diff --git a/IkiWiki/Plugin/comments.pm b/IkiWiki/Plugin/comments.pm
index b47f965e7b76359d391a5a63db8fbfc2c215a46a..0858f69f1def801d8f12f03be0c3dfd8c9e7b881 100644 (file)
--- a/IkiWiki/Plugin/comments.pm
+++ b/IkiWiki/Plugin/comments.pm
@@ -557,11 +557,12 @@ sub editcomment ($$) {
                }
                
                $postcomment=1;
                }
                
                $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} ? (
                        $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,
                        ) : (),
                        page => $location,
                        cgi => $cgi,


@@ -601,7 +602,7 @@ sub editcomment ($$) {
                                length $form->field('subject')) {
                                $message = sprintf(
                                        gettext("Added a comment: %s"),
                                length $form->field('subject')) {
                                $message = sprintf(
                                        gettext("Added a comment: %s"),

-                                       $form->field('subject'));
+                                       scalar $form->field('subject'));

                        }
 
                        IkiWiki::rcs_add($file);
                        }
 
                        IkiWiki::rcs_add($file);

diff --git a/IkiWiki/Plugin/editpage.pm b/IkiWiki/Plugin/editpage.pm
index 6ca4b589f507ae595d36bb802a97ee9640f0b9bd..99a1429143eee57db87269dd759a9d28111a9e73 100644 (file)
--- a/IkiWiki/Plugin/editpage.pm
+++ b/IkiWiki/Plugin/editpage.pm
@@ -431,7 +431,7 @@ sub cgi_editpage ($$) {
                        $conflict=rcs_commit(
                                file => $file,
                                message => $message,
                        $conflict=rcs_commit(
                                file => $file,
                                message => $message,

-                               token => $form->field("rcsinfo"),
+                               token => scalar $form->field("rcsinfo"),

                                session => $session,
                        );
                        enable_commit_hook();
                                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")) {
        }
        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 3bdd9de2ed01919ab2fa6559ba6658977bac7628..c966087ce75039baa9f1492a81976de4502936fd 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 {
                                $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")) {
                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 6b55ee35133e66a33f24710248b2afa666618189..418e8e58a2f1a705e8498ad9e22b7afaf8d493ec 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.
                # 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 $_) {
                        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 4a86d5a09b11f1e5097b053c6624cf0080f13796..56dfbd54356ebc167622f739aa31b42b7c01baeb 100644 (file)
--- 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") {
                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");
                }
                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};
                        # 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');
                        my $destfile=$dest;
                        if (! $q->param("attachment")) {
                                my $type=$q->param('type');

diff --git a/debian/changelog b/debian/changelog
index 86d06bdc664beca79a80261e875392e3d54eceb9..ccf830b2768a5c88add904666f79f1546fb59c87 100644 (file)
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,5 +1,10 @@
 ikiwiki (3.20161220) UNRELEASED; urgency=medium
 
 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
   * 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 4f825debaab65794ed1ff9e595f9ff83154f97ff..9818e0c9429de9f2e50111ef79c467f110dc2e02 100644 (file)
--- 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]])
 
 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)