Warning: this patch is incomplete and does not fix all known problems. See discussion below.
Version 1.0 of wiki.pl does not run cleanly when Perl's taint checking is enabled. Symptom: when saving a page or saving user preferences [or doing some administrative tasks], the script will throw a fatal "Insecure dependency in open while running with -T switch" error.
The cause is two routines, UserDataFilename and DoPost? [and some others, see below], which do not check possibly tainted Web input for correct format in a way that removes tainting.
First, enable taint checking (as it should have been from the start).
In UserDataFilename, remove tainting by checking whether the user id contains naught but digits.
In DoPost?, remove tainting by checking whether the page title matches $LinkPattern.
![[Home]](/wiki.gif)
*** wiki.pl Thu Oct 12 22:44:35 2006
--- wiki.pl.taintmode Fri Oct 27 17:15:47 2006
***************
*** 1,4 ****
! #!/usr/bin/perl
# UseModWiki version 1.0 (September 12, 2003)
# Copyright (C) 2000-2003 Clifford A. Adams <caadams@usemod.com>
# Copyright (C) 2002-2003 Sunir Shah <sunir@sunir.org>
--- 1,4 ----
! #!/usr/bin/perl -T
# UseModWiki version 1.0 (September 12, 2003)
# Copyright (C) 2000-2003 Clifford A. Adams <caadams@usemod.com>
# Copyright (C) 2002-2003 Sunir Shah <sunir@sunir.org>
***************
*** 2591,2596 ****
--- 2591,2601 ----
sub UserDataFilename {
my ($id) = @_;
+ if ($id =~ /(\d+)/) {
+ $id = $1; # remove tainting
+ } else {
+ die T("The userid must be a positive integer");
+ }
return "" if ($id < 1);
return $UserDir . "/" . ($id % 10) . "/$id.db";
}
***************
*** 3889,3894 ****
--- 3894,3904 ----
my ($editDiff, $old, $newAuthor, $pgtime, $oldrev, $preview, $user);
my $string = &GetParam("text", undef);
my $id = &GetParam("title", "");
+ if ($id =~ /($LinkPattern)/) {
+ $id = $1; # remove tainting
+ } else {
+ die T("Page name is not valid");
+ }
my $summary = &GetParam("summary", "");
my $oldtime = &GetParam("oldtime", "");
my $oldconflict = &GetParam("oldconflict", "");
The second part misses the case of FreeLinks. For the first one I'm a bit unsure if the check should be made late or more earlier when the UserId? is pulled from the parameters. I tend to make the checks as early as possible, which may be a bit more work. -- MarkusLude
The maintain action requires some untainting too. The page list is tainted. -- MarkusLude
The rename links action also needs attention. I'll look at all of these problems (inluding the FreeLinks fix) and try to create a comprehensive patch for version 1.0.4. I'll also examine the feasibility of moving UserId? untainting closer to where parameters are read. -- MarkIrons?
Thanks for mentioning the edit links stuff. AdminFeatures are seldom used and therefore overlooked easily. I'll look how the ValidId stuff and the untainting stuff could combined. -- MarkusLude
Markus, I've found that I've been adding this code in several places [OpenPage, DoOtherRequest, DoPageLock?]:
if ( ($FreeLinks && $id =~ /($FreeLinkPattern)/) || ($id =~ /($LinkPattern)/) ) {
$id = $1;
} else {
print Ts("%s is not a valid page name",$id);
}
Perhaps it could be rolled into a new ValidId that also untaints $id.
With a few other changes I've got everything (I think) working under taint mode. I'll update the patch above with the new code after I refactor the new code to remove duplication. Expect the update in a few days. -- MarkIrons?
The convert action too needs some untainting -- MarkusLude
What does the convert? do? Convert an older Usemod format to the latest format? It's not documented, and isn't mentioned on the Actions page. -- MarkIrons?
Yes, it converts usemod data files (user, page and keep files) from using "\xb3" as $FS (separator) to "\x1e\xff\xfe\x1e". The later is used with $NewFS set to 1.
Question: what shall we do with file names returned by a readdir() call that aren't valid ids? (E.g. a user put non-wiki files in a wiki dir.) Ignore the files? Throw an error and die? Other? Ignoring the files has some usability advantages, but the scenario of e.g. running convert? apparently successfully, only to find out later that some files weren't converted due to having invalid ids, is troubling. -- MarkIrons?
I think it's better to print out a warning message. The convert action could only be run by an admin. Sorry for the late reply. During last days I looked after some problems of the server and the primary problem with the wiki script for me is the utf-8 problem between perl 5.6 and 5.8 as mentioned on WikiBugs/Perl5.8BreaksUseModDataFiles -- MarkusLude
GenerateAllPagesList?(), where readdir() untainting happens, can also be called by regular users in a number of ways (including from DoRandom(), DoIndex?(), DoMaintain?(), SearchBody(), SearchTitleAndBody(), etc). Printing a warning probably will annoy users, since they can't fix the problem.
Right now my untainting code in GenerateAllPagesList?() silently discards invalid page names, but I've noted the problem in a comment. -- MarkIrons?
I've got code that passes these tests for pages and subpages with either WikiCase? and Free Link names:
It also passes:
diff below.
-- MarkIrons?
--- wiki.pl 2007-12-01 19:44:14.000000000 -0800
+++ wiki.pl.taintmode 2008-03-06 18:12:20.796875000 -0800
@@ -1,4 +1,4 @@
-#!perl
+#!perl -T
# UseModWiki version 1.0.4 (December 1, 2007)
# Copyright (C) 2000-2003 Clifford A. Adams <caadams@usemod.com>
# Copyright (C) 2002-2003 Sunir Shah <sunir@sunir.org>
@@ -448,6 +448,8 @@
%UserCookie = $q->cookie($CookieName);
$UserID = $UserCookie{'id'};
$UserID =~ s/\D//g; # Numeric only
+ $UserID =~ /(.*)/;
+ $UserID = $1;
if ($UserID < 200) {
$UserID = 111;
} else {
@@ -2452,6 +2454,12 @@
if ($Page{'version'} != 3) {
&UpdatePageVersion();
}
+ # QUESTION: should this be here, or at beginning of function?
+ if ( ($FreeLinks && $id =~ /($FreeLinkPattern)/) || ($id =~ /($LinkPattern)/) ) {
+ $id = $1;
+ } else {
+ print Ts('%s is not a valid page name',$id);
+ }
$OpenPageName = $id;
}
@@ -2654,6 +2662,11 @@
sub UserDataFilename {
my ($id) = @_;
+ if ($id =~ /(\d+)/) {
+ $id = $1; # remove tainting
+ } else {
+ die T("The userid must be a positive integer");
+ }
return "" if ($id < 1);
return $UserDir . "/" . ($id % 10) . "/$id.db";
}
@@ -2997,6 +3010,17 @@
}
}
}
+ my @validPages = ();
+ foreach (@pages) {
+ if (($FreeLinks && /($FreeLinkPattern)/) || /($LinkPattern)/) {
+ push(@validPages,$1);
+ }
+ else {
+ # QUESTION: what should we do with invalid page names? Print a warning?
+ # This would annoy most users, since they couldn't do anything to fix the problem.
+ }
+ }
+ @pages = @validPages;
return sort(@pages);
}
@@ -3244,6 +3268,13 @@
if ($FreeLinks) {
$id = &FreeToNormal($id); # Take care of users like Markus Lude :-)
}
+ # QUESTION: should untainting occur before the $FreeLinks test above?
+ if (($FreeLinks && ($id =~ /($FreeLinkPattern)/)) || ($id =~ /($LinkPattern)/)) {
+ $id = $1;
+ }
+ else {
+ &ReportError(T('Invalid page.'));
+ }
if (!&UserCanEdit($id, 1)) {
print &GetHeader('', T('Editing Denied'), '');
if (&UserIsBanned()) {
@@ -3971,6 +4002,12 @@
my ($editDiff, $old, $newAuthor, $pgtime, $oldrev, $preview, $user);
my $string = &GetParam("text", undef);
my $id = &GetParam("title", "");
+ if (($FreeLinks && ($id =~ /($FreeLinkPattern)/)) ||
+ ($id =~ /($LinkPattern)/)) {
+ $id = $1; # remove tainting
+ } else {
+ die Ts('Page name "$id" is not valid',$id);
+ }
my $summary = &GetParam("summary", "");
my $oldtime = &GetParam("oldtime", "");
my $oldconflict = &GetParam("oldconflict", "");
@@ -4475,6 +4512,12 @@
print '<p>', T('Missing page id to lock/unlock...');
return;
}
+ if (($FreeLinks && ($id =~ /($FreeLinkPattern)/)) || ($id =~ /($LinkPattern)/)) {
+ $id = $1;
+ }
+ else {
+ &ReportError(T('Invalid page name.'));
+ }
return if (!&ValidIdOrDie($id)); # Consider nicer error?
$fname = &GetLockedPageFile($id);
if (&GetParam("set", 1)) {
@@ -4947,6 +4990,12 @@
sub DoDeletePage {
my ($id) = @_;
+ if (($FreeLinks && ($id =~ /($FreeLinkPattern)/)) || ($id =~ /($LinkPattern)/)) {
+ $id = $1;
+ }
+ else {
+ &ReportError(T('Invalid page name.'));
+ }
return if (!&ValidIdOrDie($id));
print &GetHeader('', Ts('Delete %s', $id), '');
return if (!&UserIsAdminOrError());
@@ -5057,6 +5106,13 @@
closedir(DIRLIST);
foreach $file (@files) {
next if (($file eq '.') || ($file eq '..'));
+ if (($FreeLinks && ($file =~ /($FreeLinkPattern)/)) || ($file =~ /($LinkPattern)/)) {
+ $file = $1;
+ }
+ else {
+ print Ts('Ignoring invalid page name "%s".',"$file") . '<br>';
+ next;
+ }
$fname = "$topDir/$dir/$file";
if (-f $fname) {
# print $fname . '<br>'; # progress
@@ -5067,6 +5123,13 @@
closedir(DIRLIST);
foreach $subFile (@subFiles) {
next if (($subFile eq '.') || ($subFile eq '..'));
+ if (($FreeLinks && ($subFile =~ /($FreeLinkPattern)/)) || ($subFile =~ /($LinkPattern)/)) {
+ $subFile = $1;
+ }
+ else {
+ print Ts('Ignoring invalid subpage name "%s".',"$fname/$subFile") . '<br>';
+ next;
+ }
$subFname = "$fname/$subFile";
if (-f $subFname) {
# print $subFname . '<br>'; # progress