Item528: more analysis of validation and tainting issues, with many f…

…ixes git-svn-id: http://svn.foswiki.org/trunk@1490 0b4bb1d4-4e5a-0410-9cc4-b2b747904278
foswiki · Dec 21, 2008 · 21a9c71 · 21a9c71
1 parent bea86da
commit 21a9c71
Show file tree

Hide file tree

Showing 33 changed files with 345 additions and 260 deletions.
diff --git a/EditTablePlugin/lib/Foswiki/Plugins/EditTablePlugin.pm b/EditTablePlugin/lib/Foswiki/Plugins/EditTablePlugin.pm
@@ -76,14 +76,14 @@ sub initPlugin {
 }
 
 sub beforeCommonTagsHandler {
-    return unless $_[0] =~ /%EDIT(TABLE|CELL){(.*)}%/os;
+    return unless $_[0] =~ /%EDIT(TABLE|CELL){.*}%/s;
     require Foswiki::Plugins::EditTablePlugin::Core;
     Foswiki::Plugins::EditTablePlugin::Core::protectVariables(
         $_[0] );
 }
 
 sub commonTagsHandler {
-    return unless $_[0] =~ /%EDIT(TABLE|CELL){(.*)}%/os;
+    return unless $_[0] =~ /%EDIT(TABLE|CELL){.*}%/s;
 
     addViewModeHeadersToHead();
     require Foswiki::Plugins::EditTablePlugin::Core;

diff --git a/InterwikiPlugin/lib/Foswiki/Plugins/InterwikiPlugin.pm b/InterwikiPlugin/lib/Foswiki/Plugins/InterwikiPlugin.pm
@@ -50,10 +50,10 @@ use vars qw(
             %interSiteTable
     );
 
-# This should always be $Rev: 14913 (17 Sep 2007) $ so that TWiki can determine the checked-in
+# This should always be $Rev$ so that TWiki can determine the checked-in
 # status of the plugin. It is used by the build automation tools, so
 # you should leave it alone.
-$VERSION = '$Rev: 14913 (17 Sep 2007) $';
+$VERSION = '$Rev$';
 
 # This is a free-form string you can use to "name" your own plugin version.
 # It is *not* used by the build automation tools, but is reported as part
@@ -96,9 +96,6 @@ sub initPlugin {
           || 'InterWikis';
     ( $interWeb, $interTopic ) =
       Foswiki::Func::normalizeWebTopicName( $interWeb, $interTopic );
-    if( $interTopic =~ s/^(.*)\.// ) {
-        $interWeb = $1;
-    }
 
     my $text = Foswiki::Func::readTopicText( $interWeb, $interTopic, undef, 1 );
 

diff --git a/PreferencesPlugin/lib/Foswiki/Plugins/PreferencesPlugin.pm b/PreferencesPlugin/lib/Foswiki/Plugins/PreferencesPlugin.pm
@@ -123,6 +123,7 @@ sub beforeCommonTagsHandler {
     } elsif( $action eq 'save' ) {
 
         my( $meta, $text ) = Foswiki::Func::readTopic( $web, $topic );
+        # SMELL: unchecked implicit untaint of value?
         $text =~ s(^((?:\t|   )+\*\s(Set|Local)\s)(\w+)\s\=\s(.*)$)
           ($1._saveSet($query, $web, $topic, $3, $4, $formDef))mgeo;
         Foswiki::Func::saveTopic( $web, $topic, $meta, $text );

diff --git a/TopicUserMappingContrib/lib/Foswiki/Users/TopicUserMapping.pm b/TopicUserMappingContrib/lib/Foswiki/Users/TopicUserMapping.pm
@@ -378,6 +378,8 @@ sub addUser {
                 $web        = $1 || $Foswiki::cfg{UsersWebName};
                 $name       = $2;
                 $odate      = $3;
+                # Filter-in date format dd Mmm yyyy
+                $odate = '' unless $odate =~ /^\d+\s+[A-Za-z]+\s+\d+$/;
                 $insidelist = 1;
             }
             elsif ( $line =~ /^\s+\*\s([A-Z]) - / ) {
@@ -863,6 +865,7 @@ sub mapper_getEmails {
         # Now try the topic text
         foreach my $l ( split( /\r?\n/, $text ) ) {
             if ( $l =~ /^\s+\*\s+E-?mail:\s*(.*)$/mi ) {
+                # SMELL: implicit unvalidated untaint
                 push @addresses, split( /;/, $1 );
             }
         }

diff --git a/UnitTestContrib/test/unit/RcsTests.pm b/UnitTestContrib/test/unit/RcsTests.pm
@@ -214,8 +214,16 @@ sub verify_simple9 {
     $this->checkGetRevision([ "", "\n", "\n\n", "a", "a\n", "a\n\n", "\na","\n\na", "" ]);
 }
 
-sub verify_simple10 {
+# coMustCopy should only affect RcsWrap, but test them both anyway
+sub verify_simple10a {
     my $this = shift;
+    $Fowiki::cfg{RCS}{coMustCopy} = 0;
+    $this->checkGetRevision([ "a", "b", "a\n", "b", "a", "b\n","a\nb\n", "a\nc\n" ]);
+}
+
+sub verify_simple10b {
+    my $this = shift;
+    $Fowiki::cfg{RCS}{coMustCopy} = 1;
     $this->checkGetRevision([ "a", "b", "a\n", "b", "a", "b\n","a\nb\n", "a\nc\n" ]);
 }
 

diff --git a/WysiwygPlugin/lib/Foswiki/Plugins/WysiwygPlugin.pm b/WysiwygPlugin/lib/Foswiki/Plugins/WysiwygPlugin.pm
@@ -39,7 +39,7 @@ use vars qw( %TWikiCompatibility @refs );
 
 $SHORTDESCRIPTION = 'Translator framework for Wysiwyg editors';
 $NO_PREFS_IN_TOPIC = 1;
-$VERSION = '$Rev: 15843 $';
+$VERSION = '$Rev$';
 
 $RELEASE = '03 Aug 2008';
 
@@ -732,14 +732,29 @@ sub _restHTML2TML {
     return undef; # to prevent further processing
 }
 
+# SMELL: foswiki supports proper REST usage of the upload script,
+# so debatable if this is required any more
 sub _restUpload {
     my ($session, $plugin, $verb, $response) = @_;
     my $query = Foswiki::Func::getCgiQuery();
-    my $topic = $query->param('topic');
-    $topic =~ /^(.*)\.([^.]*)$/;
-    my $web = $1;
-    $topic = $2;
-
+    my ($web, $topic) = Foswiki::Func::normalizeWebTopicName( undef,
+        $query->param('topic'));
+    $web = Foswiki::Sandbox::untaint(
+        $web,
+        sub {
+            return $web if Foswiki::Func::isValidWebName($web, 1);
+            return undef;
+        });
+    $topic = Foswiki::Sandbox::untaint(
+        $topic,
+        sub {
+            return $topic if Foswiki::Func::isValidTopicName($topic, 1);
+            return undef;
+        });
+    unless (defined $web && defined $topic) {
+        returnRESTResult($response, 401, "Access denied");
+        return undef; # to prevent further processing
+    }
     my $hideFile = $query->param('hidefile') || '';
     my $fileComment = $query->param('filecomment') || '';
     my $createLink = $query->param('createlink') || '';
@@ -838,6 +853,18 @@ sub _restAttachments {
     my ($session, $plugin, $verb, $response) = @_;
     my ($web, $topic) = Foswiki::Func::normalizeWebTopicName(
         undef, Foswiki::Func::getCgiQuery()->param('topic'));
+    $web = Foswiki::Sandbox::untaint(
+        $web,
+        sub {
+            return $web if Foswiki::Func::isValidWebName($web, 1);
+            return undef;
+        });
+    $topic = Foswiki::Sandbox::untaint(
+        $topic,
+        sub {
+            return $topic if Foswiki::Func::isValidTopicName($topic, 1);
+            return undef;
+        });
     my ($meta, $text) = Foswiki::Func::readTopic($web, $topic);
     unless (Foswiki::Func::checkAccessPermission(
         'VIEW', Foswiki::Func::getWikiName(), $text, $topic, $web, $meta)) {

diff --git a/core/lib/Assert.pm b/core/lib/Assert.pm
@@ -10,7 +10,7 @@ require 5.006;
 #  1. ASSERT instead of assert
 #  2. has to be _explicitly enabled_ using the $ENV{ASSERT}
 #  3. should and shouldnt have been removed
-#  4. Added UNTAINTED
+#  4. Added UNTAINTED and TAINT
 #
 # Usage is as for Carp::Assert except that you have to explicitly
 # enable asserts using the environment variable ENV{FOSWIKI_ASSERTS}
@@ -19,12 +19,13 @@ require 5.006;
 
 use strict;
 
-use vars qw(@ISA $VERSION %EXPORT_TAGS);
+use vars qw(@ISA $VERSION %EXPORT_TAGS $DIRTY);
 
 BEGIN {
     $VERSION = '0.01';
+    $DIRTY = $ENV{PATH}; # Used in TAINT
 
-    $EXPORT_TAGS{NDEBUG} = [qw(ASSERT UNTAINTED DEBUG)];
+    $EXPORT_TAGS{NDEBUG} = [qw(ASSERT UNTAINTED TAINT DEBUG)];
     $EXPORT_TAGS{DEBUG}  = $EXPORT_TAGS{NDEBUG};
     Exporter::export_tags(qw(NDEBUG DEBUG));
 }
@@ -47,7 +48,7 @@ sub import {
     else {
         my $caller = caller;
         *{ $caller . '::ASSERT' }    = \&noop;
-        *{ $caller . '::UNTAINTED' } = \&ASSERTS_OFF;
+        *{ $caller . '::TAINT' }     = sub { $_[0] };
         *{ $caller . '::DEBUG' }     = \&ASSERTS_OFF;
     }
     use strict 'refs';
@@ -65,10 +66,16 @@ sub ASSERT ($;$) {
     return undef;
 }
 
+# Test if a value is untainted
 sub UNTAINTED($) {
     local ( @_, $@, $^W ) = @_;
     my $x;
     return eval { $x = $_[0], kill 0; 1 };
 }
 
+# Taint the datum passed and return the tainted value
+sub TAINT($) {
+    return substr($_[0].$DIRTY, 0, length($_[0]));
+}
+
 1;