CGI, attachment, passwordauth: harden against repeated parameters

These instances of code similar to OVE-20170111-0001 are not believed
to be exploitable, because defined(), length(), setpassword(),
userinfo_set() and the binary "." operator all have prototypes that
force the relevant argument to be evaluated in scalar context. However,
using a safer idiom makes mistakes less likely.

diff --git a/IkiWiki/CGI.pm b/IkiWiki/CGI.pm
index b6923b54f89e55e0878977fdc906d849baa81e9f..878869ef1e43888183746f2e7db4b7eda53f538c 100644 (file)
--- a/IkiWiki/CGI.pm
+++ b/IkiWiki/CGI.pm
@@ -277,8 +277,9 @@ sub cgi_prefs ($$) {
                return;
        }
        elsif ($form->submitted eq 'Save Preferences' && $form->validate) {
                return;
        }
        elsif ($form->submitted eq 'Save Preferences' && $form->validate) {

-               if (defined $form->field('email')) {
-                       userinfo_set($user_name, 'email', $form->field('email')) ||
+               my $email = $form->field('email');
+               if (defined $email) {
+                       userinfo_set($user_name, 'email', $email) ||

                                error("failed to set email");
                }
 
                                error("failed to set email");
                }
 

diff --git a/IkiWiki/Plugin/attachment.pm b/IkiWiki/Plugin/attachment.pm
index ee015ebdf8261fd3acf12324deb4e736a065353a..ab1929e3618e39ead815bfe71639272e2a71d55b 100644 (file)
--- a/IkiWiki/Plugin/attachment.pm
+++ b/IkiWiki/Plugin/attachment.pm
@@ -156,8 +156,9 @@ sub formbuilder (@) {
                        }
                        $add.="\n";
                }
                        }
                        $add.="\n";
                }

+               my $content = $form->field('editcontent');

                $form->field(name => 'editcontent',
                $form->field(name => 'editcontent',

-                       value => $form->field('editcontent')."\n\n".$add,
+                       value => $content."\n\n".$add,

                        force => 1) if length $add;
        }
        
                        force => 1) if length $add;
        }
        


@@ -213,12 +214,12 @@ sub attachment_store {
        $filename=IkiWiki::basename($filename);
        $filename=~s/.*\\+(.+)/$1/; # hello, windows
        $filename=IkiWiki::possibly_foolish_untaint(linkpage($filename));
        $filename=IkiWiki::basename($filename);
        $filename=~s/.*\\+(.+)/$1/; # hello, windows
        $filename=IkiWiki::possibly_foolish_untaint(linkpage($filename));

-       my $dest=attachment_holding_location($form->field('page'));
+       my $dest=attachment_holding_location(scalar $form->field('page'));

        
        # Check that the user is allowed to edit the attachment.
        my $final_filename=
                linkpage(IkiWiki::possibly_foolish_untaint(
        
        # Check that the user is allowed to edit the attachment.
        my $final_filename=
                linkpage(IkiWiki::possibly_foolish_untaint(

-                       attachment_location($form->field('page')))).
+                       attachment_location(scalar $form->field('page')))).

                $filename;
        eval {
                if (IkiWiki::file_pruned($final_filename)) {
                $filename;
        eval {
                if (IkiWiki::file_pruned($final_filename)) {


@@ -270,13 +271,13 @@ sub attachments_save {
 
        # Move attachments out of holding directory.
        my @attachments;
 
        # Move attachments out of holding directory.
        my @attachments;

-       my $dir=attachment_holding_location($form->field('page'));
+       my $dir=attachment_holding_location(scalar $form->field('page'));

        foreach my $filename (glob("$dir/*")) {
                $filename=Encode::decode_utf8($filename);
                next unless -f $filename;
                my $destdir=$config{srcdir}."/".
                        linkpage(IkiWiki::possibly_foolish_untaint(
        foreach my $filename (glob("$dir/*")) {
                $filename=Encode::decode_utf8($filename);
                next unless -f $filename;
                my $destdir=$config{srcdir}."/".
                        linkpage(IkiWiki::possibly_foolish_untaint(

-                               attachment_location($form->field('page'))));
+                               attachment_location(scalar $form->field('page'))));

                my $destfile=IkiWiki::basename($filename);
                my $dest=$destdir.$destfile;
                unlink($dest);
                my $destfile=IkiWiki::basename($filename);
                my $dest=$destdir.$destfile;
                unlink($dest);

diff --git a/IkiWiki/Plugin/passwordauth.pm b/IkiWiki/Plugin/passwordauth.pm
index fe1da764ae9dc07a2431aaca78478cc9364b82e4..84961c51fd2be865b9b02447cdf839d8c6b8f9f8 100644 (file)
--- a/IkiWiki/Plugin/passwordauth.pm
+++ b/IkiWiki/Plugin/passwordauth.pm
@@ -327,10 +327,12 @@ sub formbuilder (@) {
                        }
                        elsif ($form->submitted eq 'Create Account') {
                                my $email = $form->field('email');
                        }
                        elsif ($form->submitted eq 'Create Account') {
                                my $email = $form->field('email');

+                               my $password = $form->field('password');
+

                                if (IkiWiki::userinfo_setall($user_name, {
                                        'email' => $email,
                                        'regdate' => time})) {
                                if (IkiWiki::userinfo_setall($user_name, {
                                        'email' => $email,
                                        'regdate' => time})) {

-                                       setpassword($user_name, $form->field('password'));
+                                       setpassword($user_name, $password);

                                        $form->field(name => "confirm_password", type => "hidden");
                                        $form->field(name => "email", type => "hidden");
                                        $form->text(gettext("Account creation successful. Now you can Login."));
                                        $form->field(name => "confirm_password", type => "hidden");
                                        $form->field(name => "email", type => "hidden");
                                        $form->text(gettext("Account creation successful. Now you can Login."));


@@ -389,8 +391,9 @@ sub formbuilder (@) {
        elsif ($form->title eq "preferences") {
                if ($form->submitted eq "Save Preferences" && $form->validate) {
                        my $user_name=$form->field('name');
        elsif ($form->title eq "preferences") {
                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, scalar $form->field('password'));
+                       my $password=$form->field('password');
+                       if (defined $password && length $password) {
+                               setpassword($user_name, $password);

                        }
                }
        }
                        }
                }
        }