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 a6c0c271258b2f9dca1692c89ae3ce6b5d20631e..9c80c05c14ad16c91ab22f9ed9afb20989117a09 100644 (file)
--- a/IkiWiki/CGI.pm
+++ b/IkiWiki/CGI.pm
@@ -307,8 +307,9 @@ sub cgi_prefs ($$) {
                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");
                }
 

diff --git a/IkiWiki/Plugin/attachment.pm b/IkiWiki/Plugin/attachment.pm
index 04100e0fd018a5e60e2e302463398636df07135a..0d6f81c4f941f59f6532bf401f85fb79ade2bab9 100644 (file)
--- a/IkiWiki/Plugin/attachment.pm
+++ b/IkiWiki/Plugin/attachment.pm
@@ -156,8 +156,9 @@ sub formbuilder (@) {
                        }
                        $add.="\n";
                }
+               my $content = $form->field('editcontent');
                $form->field(name => 'editcontent',
-                       value => $form->field('editcontent')."\n\n".$add,
+                       value => $content."\n\n".$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));
-       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(
-                       attachment_location($form->field('page')))).
+                       attachment_location(scalar $form->field('page')))).
                $filename;
        eval {
                if (IkiWiki::file_pruned($final_filename)) {
@@ -272,12 +273,12 @@ sub attachments_save {
 
        # 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=linkpage(IkiWiki::possibly_foolish_untaint(
-                       attachment_location($form->field('page'))));
+                       attachment_location(scalar $form->field('page'))));
                my $absdestdir=$config{srcdir}."/".$destdir;
                my $destfile=IkiWiki::basename($filename);
                my $dest=$absdestdir.$destfile;

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');
+                               my $password = $form->field('password');
+
                                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."));
@@ -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');
-                       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);
                        }
                }
        }