From: Crawford C. <cra...@cr...> - 2004-11-29 11:37:24
|
Nice one, Florian! ;-) I have applied most of your changes onto the DEVELOP codebase. We had touched the same code in a few places, so a number of your changes were already in the DEVELOP codebase :-). I moved the important functions out of TWiki.pm and into TWiki/Sandbox.pm, and converted the test to use Test::Unit which is how our other unit tests are done. All unit tests pass in my 5.8 environment, and I'm just checking the integration tests now. Some questions: 1. I note you reverted the change to the search string filter in Search.pm back to what was originally posted as the fix. Why? 2. I can obviously recode readFromProcess* to use the backtick operator, for environments that don't support safe pipes. (5.6, no POSIX, Windows). What caveats should we highlight in this case? Regards, C. On Sunday 28 November 2004 17:20, Florian Weimer wrote: > Here's my backtick operator rewrite. It is based on the Subversion > trunk (r3248) and has been tested on RHEL 3. For more details, see > the included README.robustness file. Instructions how to use the new > functions can be found at: > > <http://www.enyo.de/fw/security/notes/twiki-robustness.html> > > I'm going to use this page to publish updates to the patch. Because > of the lack of Windows support, I doubt this patch can be applied to > the TWiki trunk in the near future. > > ------------------------------------------------------------------------ > r18 | f2u | 2004-11-28 14:11:10 +0100 (Sun, 28 Nov 2004) | 3 lines > > * README.robustness: > Update. > > ------------------------------------------------------------------------ > r17 | f2u | 2004-11-28 13:10:31 +0100 (Sun, 28 Nov 2004) | 12 lines > > Add some safeguards against directory traversal attacks and > normalize file names using TWiki::normalizeFileName. > > * lib/TWiki/Store/RcsLite.pm (_process, _writeme): > Normalize file names. > > * lib/TWiki/Store.pm (saveFile, readTopmeta, readFile, readFileHead): > Likewise. > > * lib/TWiki/UI/Upload.pm (_imgsize): > Likewise. > > ------------------------------------------------------------------------ > r15 | f2u | 2004-11-27 22:32:26 +0100 (Sat, 27 Nov 2004) | 9 lines > > * lib/TWiki.pm (normalizeFileName): > Be more conservative about directory traversal attempts using '..'. > By default, normalizeFileName now dies if such a component is > detected. The old behavior can be activated again by passing a true > value as second argument. > > * t/20robustness.t: > More test cases for normalizeFileName. > > ------------------------------------------------------------------------ > r12 | f2u | 2004-11-27 20:18:12 +0100 (Sat, 27 Nov 2004) | 3 lines > > * bin/manage: > Disable relockrcs because it might be unsafe. > > ------------------------------------------------------------------------ > r9 | f2u | 2004-11-27 17:44:45 +0100 (Sat, 27 Nov 2004) | 3 lines > > * lib/TWiki/Store/RcsWrap.pm (getRevision): > Invoke TWiki::untaintUnchecked (instead of direct untainting). > > ------------------------------------------------------------------------ > r8 | f2u | 2004-11-27 17:35:59 +0100 (Sat, 27 Nov 2004) | 18 lines > > Remove unused code around TWiki::Store::RcsWrap::setTopicRevisionTag. > This resolves another FIXME. > > See the discussion at: > <http://sourceforge.net/mailarchive/message.php?msg_id=10126397> > > * lib/TWiki/Store/RcsWrap.pm (setTopicRevisionTag): > Remove. > > * lib/TWiki/Store/RcsWrap.pm (setTopicRevisionTag): > Remove. > > * lib/TWiki/Func.pm (setTopicRevisionTag): > Remove. > > * lib/TWiki/Store.pm (setTopicRevisionTag): > Remove. > > ------------------------------------------------------------------------ > r7 | f2u | 2004-11-27 17:26:09 +0100 (Sat, 27 Nov 2004) | 21 lines > > Implement standard error to output redirection in > TWiki::readFromProcess. This should mean that the new > TWiki::Store::RcsWrap implementation is equivalent to the old one. > > * lib/TWiki.pm > (readFromProcess): Stop using the Perl 5.8 safe pipe to subprocess > construct. Use fork-through-open, so that we can redirect stderr to > stdout. This introduces a dependency on the POSIX module. > (normalizeFileName): Untaint components explicitly. This is very > likely a Perl bug, but we currently cannot write test cases for taint > checks, so we are forced to defer a full analysis. (Apparently, the > Perl 5.8 pipe performs very little taint checking, hiding the bug. This > needs confirmation, too.) > > * lib/TWiki/Store/RcsWrap.pm (_ci): > Mostly fix the FIXME, based on input from Crawford Currie: > <http://sourceforge.net/mailarchive/message.php?msg_id=10123583> > > * t/20robustness.t > Add test case for output redirection. > > ------------------------------------------------------------------------ > r6 | f2u | 2004-11-20 11:38:27 +0100 (Sat, 20 Nov 2004) | 22 lines > > * README.robustness: > Note that the backtick removal is mostly done. > > * t/20robustness.t: > New test cases. > > * lib/TWiki.pm > (buildCommandLine): Add new flags N, S, D. Allow more than one > replacement in each argument. > (readFromProcess): New subroutine. > > * lib/TWiki/Store/RcsWrap.pm > (whole file): Replace backtick operators with calls to > TWiki::readFromProcess. > (_ci): FIXME, the same $cmd is invoked twice. > (getRevision): FIXME to flag potentially unnecessary untaint > operations. > > * lib/TWiki.cfg: > Update RCS configuration to include flags. Temporarily disable RCS > output redirection. > > ------------------------------------------------------------------------ > r5 | f2u | 2004-11-19 20:31:44 +0100 (Fri, 19 Nov 2004) | 14 lines > > * README.robustness > Mention test cases. > > * lib/TWiki.pm > (untaintUnchecked, normalizeFileName, buildCommandLine, > readFromProcessArray): Add new subprograms. > > * lib/TWiki/Search.pm > Removed backtick operator from, replacing it with > readFromProcessArray. (The new code even is somewhat shorter.) > > * t/20robustness.t > Test cases. > > ------------------------------------------------------------------------ > r4 | fw | 2004-11-17 10:24:55 +0100 (Wed, 17 Nov 2004) | 17 lines > > Patch from Andrew Moise <ch...@de...>: > > "If you set one of the ALLOW preferences (e.g. ALLOWWEBVIEW) to a group > that doesn't exist, or has no members, then twiki treats that as if > there were no ALLOWWEBVIEW property -- i.e. all users are allowed > access. To me this seems surprising and wrong; I would expect all > access to the topic to be denied for all users in this case. > > "I believe that the attached patch makes twiki handle this case > correctly, by treating an empty @allowList as cause for failure if it > was the result of a non-empty ALLOW preference." > > <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=281624> > > * lib/TWiki/Access.pm (checkAccessPermission): > Change as described. > > === README.robustness > ================================================================== > --- README.robustness (/twiki/trunk) (revision 281) > +++ README.robustness (/twiki/branches/robustness) (revision 281) > @@ -0,0 +1,28 @@ > +This is the unofficial robustness branch of TWiki. The goal of this > +branch is to remove questionable code from the TWiki code base, to > +improve its security against attackers which can change TWiki topics > +and try to gain shell command access to the web server. (Attacks > +against the TWiki application itself and its access control mechanisms > +are not a focus of this work.) > + > +The following steps have been performed: > + > + - The Perl backtick operator has been replaced with a central > + subprocess invocation mechanism. The shell is no longer used > + to parse command lines, and shell meta-characters are no longer > + a problem. > + > + - In some places, file names are checked for directory traversal > + attempts. These checks might not be completely sufficient, > + though. > + > + - Some untaint operations have been removed. Others have been > + replaced with a call to TWiki::untaintUnchecked, which can be found > + easily using grep in future audits. > + > +Test cases for new helper routines are contained in t/20robustness.t. > + > +Note that these changes break native Windows support, and require > +Perl 5.8. It is not clear how to safely support native Windows because > +command line handling is completely different from UNIX (one large string > +which is parsed by the called program vs. one string for each argument). > === t/20robustness.t > ================================================================== > --- t/20robustness.t (/twiki/trunk) (revision 281) > +++ t/20robustness.t (/twiki/branches/robustness) (revision 281) > @@ -0,0 +1,77 @@ > +#!/usr/bin/perl > +use strict; > +use warnings; > +use Test::More tests => 54; > + > +require 'bin/setlib.cfg'; > +require_ok ('TWiki'); > +require_ok ('TWiki::Store::RcsWrap'); > + > +is (TWiki::untaintUnchecked (''), ''); > +is (TWiki::untaintUnchecked ('abc'), 'abc'); > +is (TWiki::untaintUnchecked (undef), undef); > + > +is (TWiki::normalizeFileName ('abc'), 'abc'); > +is (TWiki::normalizeFileName ('./abc'), 'abc'); > +is (TWiki::normalizeFileName ('abc/.'), 'abc'); > +is (TWiki::normalizeFileName ('-abc'), './-abc'); > +is (TWiki::normalizeFileName ('-'), './-'); > +is (TWiki::normalizeFileName ('--abc'), './--abc'); > +is (TWiki::normalizeFileName ('--'), './--'); > +is (TWiki::normalizeFileName ('/abc'), '/abc'); > +is (TWiki::normalizeFileName ('//abc'), '/abc'); > +is (TWiki::normalizeFileName ('/a/bc'), '/a/bc'); > +is (TWiki::normalizeFileName ('//a/bc'), '/a/bc'); > +is (TWiki::normalizeFileName ('/a/b/c'), '/a/b/c'); > +is (TWiki::normalizeFileName ('//a/b/c'), '/a/b/c'); > +is (TWiki::normalizeFileName ('/a/b/c/'), '/a/b/c'); > +is (TWiki::normalizeFileName ('//a/b/c/'), '/a/b/c'); > +is (TWiki::normalizeFileName ('/a/b/c/..', 1), '/a/b'); > +is (TWiki::normalizeFileName ('//a/b/c/..', 1), '/a/b'); > +is (TWiki::normalizeFileName ('/a/b/c/../..', 1), '/a'); > +is (TWiki::normalizeFileName ('//a/b/c/../..', 1), '/a'); > +is (TWiki::normalizeFileName ('/a/b/c/../../..', 1), '/'); > +is (TWiki::normalizeFileName ('//a/b/c/../../..', 1), '/'); > +is (TWiki::normalizeFileName ('a/b/c/..', 1), 'a/b'); > +is (TWiki::normalizeFileName ('a/b/c/../..', 1), 'a'); > +eval { TWiki::normalizeFileName ('a/b/c/../../..', 1) }; > +isnt ($@, ''); > +eval { TWiki::normalizeFileName ('a/..', 1) }; > +isnt ($@, ''); > +eval { TWiki::normalizeFileName ('-/..', 1) }; > +isnt ($@, ''); > +eval { TWiki::normalizeFileName ('') }; > +isnt ($@, ''); > +eval { TWiki::normalizeFileName ('', 1) }; > +isnt ($@, ''); > +eval { TWiki::normalizeFileName (undef) }; > +isnt ($@, ''); > +eval { TWiki::normalizeFileName (undef, 1) }; > +isnt ($@, ''); > +eval { TWiki::normalizeFileName ('a/b/../c') }; > +isnt ($@, ''); > + > +is_deeply ([TWiki::buildCommandLine ('a b c', ())], ['a', 'b', 'c']); > +is_deeply ([TWiki::buildCommandLine (' a b c ', ())], ['a', 'b', 'c']); > +is_deeply ([TWiki::buildCommandLine (' %A% %B% %C% ', (A => 1, B => 2, C > => 3))], [1, 2, 3]); +is_deeply ([TWiki::buildCommandLine (' %A|U% %B|F% > %C|F% ', (A => 1, B => "-..", C => "a/b"))], [1, "./-..", 'a/b']); > +is_deeply ([TWiki::buildCommandLine (' %A% %B%:%C% ', (A => 1, B => 2, C > => 3))], [1, "2:3"]); +is_deeply ([TWiki::buildCommandLine (' %A% > -n%B%:%C% ', (A => 1, B => 2, C => 3))], [1, "-n2:3"]); +is_deeply > ([TWiki::buildCommandLine (' %A% -r%B%:HEAD %C% ', (A => 1, B => 2, C => > 3))], [1, "-r2:HEAD", 3]); +is_deeply ([TWiki::buildCommandLine (' %A|F% > ', (A => ["a", "b", "/c"]))], ['a', 'b', '/c']); +is_deeply > ([TWiki::buildCommandLine (' %A|N% %B|S% %C|S%', (A => [1, 2.3, 4], B => > 'string', C => ''))], ['1', '2.3', '4', 'string', '']); +is_deeply > ([TWiki::buildCommandLine ('%A|D%', A => TWiki::formatTime (1100944661, > '$rcs', 'gmtime'))], ['2004/11/20 09:57:41']); +eval { > TWiki::buildCommandLine ('%A|%') }; > +isnt ($@, ''); > +eval { TWiki::buildCommandLine ('%A|X%') }; > +isnt ($@, ''); > +eval { TWiki::buildCommandLine (' %A|N% ', A => '2/3') }; > +isnt ($@, ''); > +eval { TWiki::buildCommandLine (' %A|S% ', A => '2/3') }; > +isnt ($@, ''); > + > +is_deeply ([TWiki::readFromProcessArray ('echo', ' %A% %B% ', (A => " 1", > B => "2 "))], [" 1 2 "]); + > +is_deeply ([TWiki::readFromProcess ('sh -c %A%', A => 'exit 7')], ['', > 7]); +is_deeply ([TWiki::readFromProcess ('sh -c %A%', A => 'echo 1; echo > 2')], ["1\n2\n", 0]); +is_deeply ([TWiki::readFromProcess ('sh -c %A%', A > => 'echo 1; echo 2 1>&2')], ["1\n2\n", 0]); === lib/TWiki.pm > ================================================================== > --- lib/TWiki.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki.pm (/twiki/branches/robustness) (revision 281) > @@ -3212,6 +3212,236 @@ > return $text; > } > > +=pod > +---++untaintUnchecked ( $string ) > +Return value: $untainted > + > +Untaints $string without any checks (dangerous). If $string is > +undefined, return undef. > + > +The intent is to use this routine to be able to find all untainting > +places using grep. > + > +=cut > + > +sub untaintUnchecked ($) { > + my $string = shift; > + return undef unless defined $string; > + if ($string =~ /^(.*)$/) { > + return $1; > + } > + return $string; # Can't happen. > +} > + > +=pod > +---++normalizeFileName ( $string [, $dotdot] ) > +Return value: $filename > + > +Errors out if $string contains whitespace characters. If $dotdot is > +present and true, allow ".." in path names. > + > +The returned string is not tainted, but it may contain shell > +metacharacters and even control characters. > + > +=cut > + > +sub normalizeFileName ($;$) { > + my ($string, $dotdot) = @_; > + my $absolute = $string =~ /^\//; > + my @result; > + for my $component (split /\//, $string) { > + next unless $component; > + next if $component eq '.'; > + if ($component eq '..') { > + if ($dotdot && @result > 0) { > + pop @result; > + } else { > + die 'directory traversal attempt'; > + } > + } elsif ($component =~ /^(\S+)$/) { > + # We need to untaint the string explicitly. > + # FIXME: This might be a Perl bug. > + push @result, untaintUnchecked $1; > + } else { > + die 'whitespace in file name component'; > + } > + } > + if (@result) { > + if ($absolute) { > + $result[0] = "/$result[0]"; > + } elsif ($result[0] =~ /^-/) { > + $result[0] = "./$result[0]"; > + } > + return join '/', @result; > + } else { > + return '/' if $absolute; > + die 'empty file name'; > + } > +} > + > +=pod > +---++buildCommandLine ( $template, %params ) > +Return value: @arguments > + > +$template is split at whitespace, and '%VAR%' strings contained in it > +are replaced with $params{VAR}. %params may consist of scalars and > +array references as values. Array references are dereferenced and the > +array elements are inserted into the command line at the indicated > +point. > + > +'%VAR%' can optionally take the form '%VAR|FLAG%', where FLAG is a > +single character flag. Permitted flags are U (untaint without further > +checks -- dangerous), F (normalize as file name), N (generalized > +number), and S (simple, short string). > + > +=cut > + > +sub buildCommandLine ($%) { > + my ($template, %params) = @_; > + my @arguments; > + > + for my $tmplarg (split /\s+/, $template) { > + next if $tmplarg eq ''; # ignore leading/trailing whitespace > + > + # Split single argument into its parts. It may contain > + # multiple substitutions. > + > + my @tmplarg = $tmplarg =~ /([^%]+|%[^%]+%)/g; > + my @targs; > + for my $t (@tmplarg) { > + if ($t =~ /%(.*?)(|\|[A-Z])%/) { > + my ($p, $flag) = ($1, $2); > + if (! exists $params{$p}) { > + die "unknown parameter name $p"; > + } > + my $type = ref $params{$p}; > + my @params; > + if ($type eq '') { > + @params = ($params{$p}); > + } elsif ($type eq 'ARRAY') { > + @params = @{$params{$p}}; > + } else { > + die "$type reference passed in $p"; > + } > + > + for my $param (@params) { > + if ($flag) { > + if ($flag =~ /U/) { > + push @targs, untaintUnchecked $param; > + } elsif ($flag =~ /F/) { > + push @targs, normalizeFileName $param; > + } elsif ($flag =~ /N/) { > + # Generalized number. > + if ($param =~ /^([0-9A-Fa-f.x+\-]{0,30})$/) { > + push @targs, $1; > + } else { > + die "invalid number argument"; > + } > + } elsif ($flag =~ /S/) { > + # Harmless string. > + if ($param =~ /^([0-9A-Za-z.+_\-]{0,30})$/) { > + push @targs, $1; > + } else { > + die "invalid string argument"; > + } > + } elsif ($flag =~ /D/) { > + # RCS date. > + if ($param =~ m!^(\d\d\d\d/\d\d/\d\d \d\d:\d\d:\d\d)$!) { > + push @targs, $1; > + } else { > + die "invalid date argument"; > + } > + } else { > + die "illegal flag in $t"; > + } > + } else { > + push @targs, $param; > + } > + } > + } else { > + push @targs, $t; > + } > + } > + > + # Recombine the argument if the template argument contained > + # multiple parts. > + > + if (@tmplarg == 1) { > + push @arguments, @targs; > + } else { > + push @arguments, join ('', @targs); > + } > + } > + > + return @arguments; > +} > + > +=pod > + > +---++readFromProcessArray ( $path, $template, @params ) > +Return value: @outputLines > + > +Invokes the program $path with the arguments described by $template > +and @params, and returns the output of the program as an array of > +lines. If $path is not absolute, $ENV{PATH} is searched. > + > +$template is interpreted by buildCommandLine. > + > +The caller has to ensure that the invoked program does not react in a > +harmful way to the passed arguments. readFromProcessArray merely > +ensures that the shell does not interpret any of the passed arguments. > + > +=cut > + > +sub readFromProcessArray ($$@) { > + my ($path, $template, %params) = @_; > + > + my @args = buildCommandLine $template, %params; > + my $process; > + open $process, '-|', $path, @args > + or die "open failed: $!"; > + my @data = map { chomp $_; $_ } <$process>; # remove newline > characters. + close $process; > + return @data; > +} > + > +=pod > + > +---++readFromProcess ( $template, @params ) > +Return value: ($output, $status) > + > +Like readFromProcessArray, but returns the process output as a single > +string, together with the exit status. Furthermore, the program to > +execute is taken from the first argument in $template, and standard > +error is redirected to standard input. > + > +=cut > + > +use POSIX; > + > +sub readFromProcess ($@) { > + my ($template, %params) = @_; > + > + my @args = buildCommandLine $template, %params; > + > + # The code follows the safe pipe construct found in perlipc(1). > + my $pipe; > + my $pid = open $pipe, '-|'; > + if ($pid) { # parent > + local $/; # read everything in one operation > + my $data = <$pipe>; > + close $pipe; > + return ($data, $? >> 8); > + } else { > + # Redirect standard error to standard output. > + POSIX::close 2; > + POSIX::dup 1; > + exec { $args[0] } @args; > + # Usually not reached. > + exit 127; > + } > +} > + > =end twiki > > =cut > === lib/TWiki.cfg > ================================================================== > --- lib/TWiki.cfg (/twiki/trunk) (revision 281) > +++ lib/TWiki.cfg (/twiki/branches/robustness) (revision 281) > @@ -192,7 +192,8 @@ > $useRcsDir = "0"; > # This should enable gathering of extra error information on most OSes. > However, won't work on NT4 unless unix like shell is used $endRcsCmd > = ""; > -$endRcsCmd = " 2>&1" if( $OS eq "UNIX" ); > +# Disable verbose error output because safe pipes do not support them yet. > +# $endRcsCmd = " 2>&1" if( $OS eq "UNIX" ); > # Command quote ' for unix, \" for Windows > $cmdQuote = "'"; > $cmdQuote = "\"" if( $OS eq "WINDOWS" ); > @@ -304,19 +305,20 @@ > useRcsDir => $TWiki::useRcsDir, > > # Rcs only > - initBinaryCmd => "$rcsDir/rcs $rcsArg -q -i -t-none -kb %FILENAME% > $endRcsCmd", - tmpBinaryCmd => "$rcsDir/rcs $rcsArg -q -kb %FILENAME% > $endRcsCmd", - ciCmd => "$rcsDir/ci $rcsArg -q -l > -m$cmdQuote%COMMENT%$cmdQuote -t-none -w$cmdQuote%USERNAME%$cmdQuote > %FILENAME% $endRcsCmd", - coCmd => "$rcsDir/co $rcsArg -q > -p%REVISION% $keywordMode %FILENAME% $endRcsCmd", - histCmd => > "$rcsDir/rlog $rcsArg -h %FILENAME% $endRcsCmd", - infoCmd => > "$rcsDir/rlog $rcsArg -r%REVISION% %FILENAME% $endRcsCmd", - diffCmd > => "$rcsDir/rcsdiff $rcsArg -q -w -B -r%REVISION1% -r%REVISION2% > $keywordMode --unified=%CONTEXT% %FILENAME% $endRcsCmd", - breakLockCmd > => "$rcsDir/rcs $rcsArg -q -l -M %FILENAME% $endRcsCmd", - ciDateCmd > => "$rcsDir/ci -l $rcsArg -q -mnone -t-none -d$cmdQuote%DATE%$cmdQuote > -w$cmdQuote%USERNAME%$cmdQuote %FILENAME% $endRcsCmd", - delRevCmd > => "$rcsDir/rcs $rcsArg -q -o%REVISION% %FILENAME% $endRcsCmd", - > unlockCmd => "$rcsDir/rcs $rcsArg -q -u %FILENAME% $endRcsCmd", - > lockCmd => "$rcsDir/rcs $rcsArg -q -l %FILENAME% $endRcsCmd", - > tagCmd => "$rcsDir/rcs $rcsArg -N%TAG%:%REVISION% %FILENAME% > $endRcsCmd", + # This settings are security-relevant. > + initBinaryCmd => "$rcsDir/rcs $rcsArg -q -i -t-none -kb %FILENAME|F% > $endRcsCmd", + tmpBinaryCmd => "$rcsDir/rcs $rcsArg -q -kb %FILENAME|F% > $endRcsCmd", + ciCmd => "$rcsDir/ci $rcsArg -q -l -m%COMMENT|U% > -t-none -w%USERNAME|S% %FILENAME|F% $endRcsCmd", + coCmd => > "$rcsDir/co $rcsArg -q -p%REVISION|N% $keywordMode %FILENAME|F% > $endRcsCmd", + histCmd => "$rcsDir/rlog $rcsArg -h %FILENAME|F% > $endRcsCmd", + infoCmd => "$rcsDir/rlog $rcsArg -r%REVISION|N% > %FILENAME|F% $endRcsCmd", + diffCmd => "$rcsDir/rcsdiff $rcsArg -q > -w -B -r%REVISION1|N% -r%REVISION2|N% $keywordMode --unified=%CONTEXT|N% > %FILENAME|F% $endRcsCmd", + breakLockCmd => "$rcsDir/rcs $rcsArg -q -l > -M %FILENAME|F% $endRcsCmd", + ciDateCmd => "$rcsDir/ci -l $rcsArg > -q -mnone -t-none -d%DATE|D% -w%USERNAME|S% %FILENAME|F% $endRcsCmd", + > delRevCmd => "$rcsDir/rcs $rcsArg -q -o%REVISION|N% %FILENAME|F% > $endRcsCmd", + unlockCmd => "$rcsDir/rcs $rcsArg -q -u %FILENAME|F% > $endRcsCmd", + lockCmd => "$rcsDir/rcs $rcsArg -q -l %FILENAME|F% > $endRcsCmd", + tagCmd => "$rcsDir/rcs $rcsArg > -N%TAG|S%:%REVISION|N% %FILENAME|F% $endRcsCmd", ); > > # Regex security filter for web name, topic name, user > name : === lib/TWiki/Search.pm > ================================================================== > --- lib/TWiki/Search.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki/Search.pm (/twiki/branches/robustness) (revision 281) > @@ -193,16 +193,17 @@ > unless( $theScope eq "topic" ) { > # Construct command line with 'grep'. I18N: 'grep' must use > locales if needed, # for case-insensitive searching. See > TWiki::setupLocale. - my $cmd = ""; > + my $program; > if( $theType eq "regex" ) { > - $cmd .= $TWiki::egrepCmd; > + $program = $TWiki::egrepCmd; > } else { > - $cmd .= $TWiki::fgrepCmd; > + $program = $TWiki::fgrepCmd; > } > - $cmd .= " -i" unless( $caseSensitive ); > - $cmd .= " -l -- $TWiki::cmdQuote%TOKEN%$TWiki::cmdQuote > %FILES%"; > > - my $result = ""; > + my $template = ''; > + $template .= ' -i' unless( $caseSensitive ); > + $template .= ' -l -- %TOKEN|U% %FILES|F%'; > + > if( $sDir ) { > chdir( "$sDir" ); > _traceExec( "chdir to $sDir", "" ); > @@ -215,15 +216,10 @@ > my @set = splice( @take, 0, $maxTopicsInSet ); > while( @set ) { > @set = map { "$_.txt" } @set; # add > ".txt" extension to topic names - my $acmd = $cmd; > - $acmd =~ s/%TOKEN%/$token/o; > - $acmd =~ s/%FILES%/@set/o; > - $acmd =~ /(.*)/; > - $acmd = "$1"; # > untaint variable (FIXME: Needs a better check!) - $result = > `$acmd`; > - _traceExec( $acmd, $result ); > - @set = split( /\n/, $result ); > - @set = map { /(.*)\.txt$/; $_ = $1; } @set; # cut > ".txt" extension + @set = TWiki::readFromProcessArray > ($program, $template, + TOKEN => $token, > + FILES => \@set); > + @set = map { $_ =~ s/\.txt$//; $_ } @set; # > cut ".txt" extension my %seen = (); > foreach my $topic ( @set ) { > $seen{$topic}++; # make > topics unique @@ -445,9 +441,9 @@ > > # fix for Codev.SecurityAlertExecuteCommandsWithSearch > # vulnerability, search: "test_vulnerability '; ls -la'" > - $theSearchVal =~ s/(^|[^\\])([\'\`])/\\$2/g; # Escape ' and ` > - $theSearchVal =~ s/[\@\$]\(/$1\\\(/g; # Defuse @( ... ) and > $( ... ) - $theSearchVal = substr($theSearchVal, 0, 1500); # Limit > string length + $theSearchVal =~ s/[\'\`]//g; # Filter > ' and ` + $theSearchVal =~ s/\@\(/\@\\\(/g; # Defuse @( ... > ) + $theSearchVal = substr($theSearchVal, 0, 200); # Limit string length > > my $originalSearch = $theSearchVal; > my $renameTopic; > === lib/TWiki/Store/RcsLite.pm > ================================================================== > --- lib/TWiki/Store/RcsLite.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki/Store/RcsLite.pm (/twiki/branches/robustness) (revision > 281) @@ -234,7 +234,7 @@ > sub _process > { > my( $self ) = @_; > - my $rcsFile = $self->rcsFile(); > + my $rcsFile = TWiki::normalizeFileName( $self->rcsFile() ); > if( ! -e $rcsFile ) { > $self->{where} = "nofile"; > return; > @@ -624,7 +624,7 @@ > my $out = new FileHandle; > > chmod( 0644, $self->rcsFile() ); # FIXME move permission to config or > similar - if( ! $out->open( "> " . $self->rcsFile() ) ) { > + if( ! $out->open( "> " . TWiki::normalizeFileName( $self->rcsFile() ) > ) ) { $dataError = "Problem opening " . $self->rcsFile() . " for writing"; > } else { > binmode( $out ); > @@ -730,32 +730,6 @@ > } > > > -=pod > - > ----+++ setTopicRevisionTag( $web, $topic, $rev, $tag ) ==> $success > - > -| Description: | sets a names tag on the specified revision | > -| Parameter: =$web= | webname | > -| Parameter: =$topic= | topic name | > -| Parameter: =$rev= | the revision we are taging | > -| Parameter: =$tag= | the string to tag with | > -| Return: =$success= | | > -| TODO: | we _need_ an error mechanism! | > -| Since: | TWiki:: (20 April 2004) | > - > -=cut > - > -sub setTopicRevisionTag > -{ > -# my ( $self, $web, $topic, $rev, $tag ) = @_; > - > - TWiki::writeDebug("setTopicRevisionTag - not implemented in RCSLite"); > -#TODO: implement me :) > - > - return ""; > -} > - > - > # ====================== > =pod > > === lib/TWiki/Store/RcsWrap.pm > ================================================================== > --- lib/TWiki/Store/RcsWrap.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki/Store/RcsWrap.pm (/twiki/branches/robustness) (revision > 281) @@ -159,11 +159,7 @@ > # Can only do something when changing to binary > my $cmd = $self->{"initBinaryCmd"}; > my $file = $self->file(); > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote/go; > - $cmd =~ /(.*)/; > - $cmd = "$1"; # safe, so untaint variable > - my $rcsOutput = `$cmd`; > - my $exit = $? >> 8; > + my ($rcsOutput, $exit) = TWiki::readFromProcess ($cmd, FILENAME => > $file); _traceExec( $cmd, $rcsOutput, $exit ); > if( $exit && $rcsOutput ) { > $rcsOutput = "$cmd\n$rcsOutput"; > @@ -216,7 +212,6 @@ > my $file = $self->{file}; > my $rcsFile = $self->{rcsFile}; > my $cmd; > - my $rcsOut; > > # update repository with same userName and date > if( $rev == 1 ) { > @@ -228,15 +223,11 @@ > $self->_saveFile( $self->file(), $text ); > $cmd = $self->{ciDateCmd}; > $date = TWiki::formatTime( $date , "\$rcs", "gmtime"); > - $cmd =~ s/%DATE%/$date/; > - $cmd =~ s/%USERNAME%/$user/; > - $file =~ s/$TWiki::securityFilter//go; > - $rcsFile =~ s/$TWiki::securityFilter//go; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote > $cmdQuote$rcsFile$cmdQuote/; - $cmd =~ /(.*)/; > - $cmd = $1; # safe, so untaint variable > - $rcsOut = `$cmd`; > - my $exit = $? >> 8; > + my ($rcsOut, $exit) = TWiki::readFromProcess > + ($cmd, > + DATE => $date, > + USERNAME => $user, > + FILENAME => [$file, $rcsFile]); > _traceExec( $cmd, $rcsOut, $exit ); > #$rcsOut =~ s/^Warning\: missing newline.*//os; # forget warning > if( $exit ) { > @@ -281,11 +272,8 @@ > my $file = $self->{file}; > my $rcsFile = $self->{rcsFile}; > my $cmd= $self->{unlockCmd}; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote > $cmdQuote$rcsFile$cmdQuote/go; - $cmd =~ /(.*)/; > - $cmd = $1; # safe, so untaint > - my $rcsOut = `$cmd`; # capture stderr > - my $exit = $? >> 8; > + my ($rcsOut, $exit) = TWiki::readFromProcess > + ($cmd, FILENAME => [$file, $rcsFile]); > _traceExec( $cmd, $rcsOut, $exit ); > #$rcsOut =~ s/^Warning\: missing newline.*//os; # forget warning > if( $exit ) { > @@ -293,12 +281,10 @@ > return $rcsOut; > } > $cmd= $self->{delRevCmd}; > - $cmd =~ s/%REVISION%/1.$rev/go; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote > $cmdQuote$rcsFile$cmdQuote/go; - $cmd =~ /(.*)/; > - $cmd = $1; # safe, so untaint variable > - $rcsOut = `$cmd`; > - $exit = $? >> 8; > + ($rcsOut, $exit) = TWiki::readFromProcess > + ($cmd, > + REVISION => "1.$rev", > + FILENAME => [$file, $rcsFile]); > _traceExec( $cmd, $rcsOut, $exit ); > #$rcsOut =~ s/^Warning\: missing newline.*//os; # forget warning > if( $exit ) { > @@ -306,11 +292,10 @@ > return $rcsOut; > } > $cmd= $self->{lockCmd}; > - $cmd =~ s/%REVISION%/$rev/go; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote > $cmdQuote$rcsFile$cmdQuote/go; - $cmd =~ /(.*)/; > - $cmd = $1; # safe, so untaint variable > - $rcsOut = `$cmd`; > + ($rcsOut, $exit) = TWiki::readFromProcess > + ($cmd, > + REVISION => "$rev", > + FILENAME => [$file, $rcsFile]); > _traceExec( $cmd, $rcsOut, $exit ); > #$rcsOut =~ s/^Warning\: missing newline.*//os; # forget warning > if( $exit ) { > @@ -344,26 +329,21 @@ > $tmpRevFile = "$tmpfile,v"; > copy( $self->rcsFile(), $tmpRevFile ); > my $cmd1 = $self->{tmpBinaryCmd}; > - $cmd1 =~ s/%FILENAME%/$cmdQuote$tmpRevFile$cmdQuote/; > - $cmd1 =~ /(.*)/; > - $cmd1 = "$1"; > - my $tmp = `$cmd1`; > + my ($tmp) = TWiki::readFromProcess ($cmd1, FILENAME => > $tmpRevFile); _traceExec( $cmd1, $tmp ); > $file = $tmpfile; > $cmd =~ s/-p%REVISION%/-r%REVISION%/; > } > - $cmd =~ s/%REVISION%/1.$version/; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote/; > - $cmd =~ /(.*)/; > - $cmd = "$1"; # untaint > - my $text = `$cmd`; > + my ($text) = TWiki::readFromProcess > + ($cmd, > + REVISION => "1.$version", > + FILENAME => $file); > if( $tmpfile ) { > $text = $self->_readFile( $tmpfile ); > - $tmpfile =~ /(.*)/; > - $tmpfile = "$1"; # untaint > + # FIXME: Is untainting really necessary here? > + $tmpfile = TWiki::untaintUnchecked $tmpfile; > unlink $tmpfile; > - $tmpRevFile =~ /(.*)/; > - $tmpRevFile = "$1"; # untaint > + $tmpRevFile = TWiki::untaintUnchecked $tmpRevFile; > unlink $tmpRevFile; > } > _traceExec( $cmd, $text ); > @@ -388,10 +368,7 @@ > return ""; > } > > - $cmd =~ s/%FILENAME%/$cmdQuote$rcsFile$cmdQuote/; > - $cmd =~ /(.*)/; > - $cmd = $1; # now safe, so untaint variable > - my $rcsOutput = `$cmd`; > + my ($rcsOutput) = TWiki::readFromProcess ($cmd, FILENAME => $rcsFile); > _traceExec( $cmd, $rcsOutput ); > if( $rcsOutput =~ /head:\s+\d+\.(\d+)\n/ ) { > return $1; > @@ -433,11 +410,10 @@ > my( $dummy, $rev, $date, $user, $comment ); > if ( -e $rcsFile ) { > my $cmd= $self->{infoCmd}; > - $cmd =~ s/%REVISION%/$version/; > - $cmd =~ s/%FILENAME%/$cmdQuote$rcsFile$cmdQuote/; > - $cmd =~ /(.*)/; $cmd = $1; # Untaint > - my $rcsOut = `$cmd`; > - my $exit = $? >> 8; > + my ($rcsOut, $exit) = TWiki::readFromProcess > + ($cmd, > + REVISION => $version, > + FILENAME => $rcsFile); > _traceExec( $cmd, $cmd, $exit ); > $rcsError = "Error with $cmd, output: $rcsOut" if( $exit ); > if( ! $rcsError ) { > @@ -477,6 +453,7 @@ > my $error = ""; > > my $tmp= ""; > + my $exit; > if ( $rev1 eq "1" && $rev2 eq "1" ) { > my $text = $self->getRevision(1); > $tmp = "1a1\n"; > @@ -484,20 +461,16 @@ > $tmp = "$tmp> $_\n"; > } > } else { > - $tmp= $self->{"diffCmd"}; > - $tmp =~ s/%REVISION1%/1.$rev1/; > - $tmp =~ s/%REVISION2%/1.$rev2/; > + my $cmd = $self->{"diffCmd"}; > my $rcsFile = $self->rcsFile(); > - $rcsFile =~ s/$TWiki::securityFilter//go; > - $tmp =~ s/%FILENAME%/$cmdQuote$rcsFile$cmdQuote/; > - $tmp =~ s/%CONTEXT%/$contextLines/; > - $tmp =~ /(.*)/; > - my $cmd = $1; # now safe, so untaint variable > - $tmp = `$cmd`; > - my $exit = $? >> 8; > + ($tmp, $exit) = TWiki::readFromProcess > + ($cmd, > + REVISION1 => "1.$rev1", > + REVISION2 => "1.$rev2", > + FILENAME => $rcsFile, > + CONTEXT => $contextLines); > $error = "Error $exit when runing $cmd"; > _traceExec( $cmd, $tmp, $exit ); > - _trace( "and now $tmp" ); > # Avoid showing change in revision number! > # I'm not too happy with this implementation, I think it may be > better to filter before sending to diff command, # possibly using > Algorithm::Diff from CPAN. > @@ -593,32 +566,27 @@ > return "$file is not writable" unless ( -w $file ); > > my $cmd = $self->{"ciCmd"}; > - my $rcsOutput = ""; > - $cmd =~ s/%USERNAME%/$userName/; > - $file =~ s/$TWiki::securityFilter//go; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote/; > - $comment = "none" unless( $comment ); > - $comment =~ s/[\"\'\`\;]//go; # security, > Codev.NoShellCharacterEscapingInFileAttachComment, MikeSmith - $cmd =~ > s/%COMMENT%/$comment/; > - $cmd =~ /(.*)/; > - $cmd = $1; # safe, so untaint variable > - $rcsOutput = `$cmd`; # capture stderr (S.Knutson) > - my $exit = $? >> 8; > + my ($rcsOutput, $exit) = TWiki::readFromProcess > + ($cmd, > + USERNAME => $userName, > + FILENAME => $file, > + COMMENT => $comment); > _traceExec( $cmd, $rcsOutput ); > if( $exit && $rcsOutput =~ /no lock set by/ ) { > - # Try and break lock, setting new one and doing ci again > - my $cmd = $self->{"breakLockCmd"}; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote/go; > - $cmd =~ /(.*)/; > - my $out = `$cmd`; > - _traceExec( $cmd, $out ); > - # Assume it worked, as not sure how to trap failure > - $rcsOutput = `$cmd`; # capture stderr (S.Knutson) > - $exit = $? >> 8; > - _traceExec( $cmd, $rcsOutput ); > - if( ! $exit ) { > - $rcsOutput = ""; > - } > + # Try and break lock, setting new lock and doing ci again > + # Assume it worked, as not sure how to trap failure > + TWiki::readFromProcess ($self->{"breakLockCmd"}, FILENAME => $file); > + > + # re-do the ci command > + ($rcsOutput, $exit) = TWiki::readFromProcess > + ($cmd, > + USERNAME => $userName, > + FILENAME => $file, > + COMMENT => $comment); > + # FIXME: Is this really correct? It suppresses the error return > below. + if( ! $exit ) { > + $rcsOutput = ""; > + } > } > if( $exit && $rcsOutput ) { # oops, stderr was not empty, return error > $rcsOutput = "$cmd\n$rcsOutput"; > @@ -626,45 +594,4 @@ > return $rcsOutput; > } > > -=pod > - > ----+++ setTopicRevisionTag( $web, $topic, $rev, $tag ) ==> $success > - > -| Description: | sets a names tag on the specified revision | > -| Parameter: =$web= | webname | > -| Parameter: =$topic= | topic name | > -| Parameter: =$rev= | the revision we are taging | > -| Parameter: =$tag= | the string to tag with | > -| Return: =$success= | | > -| TODO: | we _need_ an error mechanism! | > -| TODO: | NEED to check if the version exists (rcs does not) | > -| Since: | TWiki:: (20 April 2004) | > - > -=cut > - > -sub setTopicRevisionTag > -{ > - my ( $self, $web, $topic, $rev, $tag ) = @_; > - > - my $file = $self->{file}; > - if ( -e $file ) { > - my $cmd= $self->{tagCmd}; > - $cmd =~ s/%REVISION%/$rev/; > - $cmd =~ s/%FILENAME%/$cmdQuote$file$cmdQuote/; > - $cmd =~ s/%TAG%/$tag/; > - $cmd = $cmd." 2>> $TWiki::warningFilename"; > - $cmd =~ /(.*)/; $cmd = $1; # Untaint > - my $rcsOut = `$cmd`; > - my $exit = $? >> 8; > - _traceExec( $cmd, $cmd, $exit ); > - if( $exit && $rcsOut ) { # oops, stderr was not empty, return error > - $rcsOut = "$cmd\n$$rcsOut"; > - TWiki:writeDebug("RCSWrap::setTopicRevisionTag error - $rcsOut"); > - return; > - } > - } > - > - return 1;#success > -} > - > 1; > === lib/TWiki/Func.pm > ================================================================== > --- lib/TWiki/Func.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki/Func.pm (/twiki/branches/robustness) (revision 281) > @@ -848,29 +848,6 @@ > } > > # ========================= > -# (undocumented feature of Cairo since RcsLite is not implemented yet) > -#=pod > -# > -#---+++ setTopicRevisionTag( $web, $topic, $rev, $tag ) ==> $success > -# > -#| Description: | Sets a names tag on the specified revision | > -#| Parameter: =$web= | Web name | > -#| Parameter: =$topic= | Topic name | > -#| Parameter: =$rev= | The revision to tag | > -#| Parameter: =$tag= | The string to tag with | > -#| Return: =$success= | (CODE_SMELL: Other functions return error string, > or empty if OK) | -#| TODO: | we _need_ an error mechanism! | > -#| Since: | TWiki::Plugins::VERSION 1.022 (20 April 2004) | > -# > -#=cut > - > -sub setTopicRevisionTag > -{ > -# my ( $web, $topic, $rev, $tag ) = @_; > - > - return TWiki::Store::setTopicRevisionTag( @_ ); > -} > - > =pod > > ---++ Functions: Rendering > === lib/TWiki/Store.pm > ================================================================== > --- lib/TWiki/Store.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki/Store.pm (/twiki/branches/robustness) (revision 281) > @@ -981,6 +981,7 @@ > sub saveFile > { > my( $name, $text ) = @_; > + $name = TWiki::normalizeFileName( $name ); > > umask( 002 ); > unless ( open( FILE, ">$name" ) ) { > @@ -1236,7 +1237,7 @@ > my( $theWeb, $theTopic ) = @_; > > my $topicHandler = _getTopicHandler( $theWeb, $theTopic ); > - my $filename = getFileName( $theWeb, $theTopic ); > + my $filename = TWiki::normalizeFileName( getFileName( $theWeb, > $theTopic ) ); > > my $data = ""; > my $line; > @@ -1579,15 +1580,14 @@ > Return value: $fileContents > > Returns the entire contents of the given file, which can be specified in > any -format acceptable to the Perl open() function. SECURITY NOTE: make > sure -any $filename coming from a user is stripped of special characters > that might -change Perl's open() semantics. > +format acceptable to the Perl open() function. > > =cut > > sub readFile > { > my( $name ) = @_; > + $name = TWiki::normalizeFileName( $name ); > my $data = ""; > undef $/; # set to read to EOF > open( IN_FILE, "<$name" ) || return ""; > @@ -1610,6 +1610,7 @@ > sub readFileHead > { > my( $name, $maxLines ) = @_; > + $name = TWiki::normalizeFileName( $name ); > my $data = ""; > my $line; > my $l = 0; > @@ -1727,34 +1728,6 @@ > } > #/AS > > -=pod > - > ----+++ setTopicRevisionTag( $web, $topic, $rev, $tag ) ==> $success > - > -| Description: | sets a names tag on the specified revision | > -| Parameter: =$web= | webname | > -| Parameter: =$topic= | topic name | > -| Parameter: =$rev= | the revision we are taging | > -| Parameter: =$tag= | the string to tag with | > -| Return: =$success= | | > -| TODO: | we _need_ an error mechanism! | > -| Since: | TWiki:: (20 April 2004) | > - > -=cut > - > -sub setTopicRevisionTag > -{ > - my ( $web, $topic, $rev, $tag ) = @_; > - > - my $topicHandler = _getTopicHandler( $web, $topic ); > -# TWiki::writeDebug("Store - setTopicRevisionTag ( $web, $topic, $rev, > $tag )"); - return $topicHandler->setTopicRevisionTag( $web, $topic, > $rev, $tag ); -} > - > - > - > -# ========================= > - > 1; > > # EOF > === lib/TWiki/Access.pm > ================================================================== > --- lib/TWiki/Access.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki/Access.pm (/twiki/branches/robustness) (revision 281) > @@ -140,6 +140,10 @@ > @denyList = @tmpList; > } else { > @allowList = @tmpList; > + if ($2 =~ /\S/ && !@allowList) { > + ##&TWiki::writeDebug( " return 0, empty ALLOWTOPIC" ); > + return 0; > + } > } > } > } > @@ -159,6 +163,10 @@ > prvGetUserList( $tmpVal ); > ##my $tmp = join( ', ', @allowList ); > ##&TWiki::writeDebug( " Prefs ALLOWWEB$theAccessType: {$tmp}" ); > + if ($tmpVal && $tmpVal =~ /\S/ && !@allowList) { > + ##&TWiki::writeDebug( " return 0, empty ALLOWWEB" ); > + return 0; > + } > } > > # access permission logic > === lib/TWiki/UI/Upload.pm > ================================================================== > --- lib/TWiki/UI/Upload.pm (/twiki/trunk) (revision 281) > +++ lib/TWiki/UI/Upload.pm (/twiki/branches/robustness) (revision 281) > @@ -119,6 +119,7 @@ > my( $file ) = shift @_; > my( $x, $y) = ( 0, 0 ); > > + $file = TWiki::normalizeFileName( $file ); > if( defined( $file ) && open( STRM, "<$file" ) ) { > binmode( STRM ); # for crappy MS OSes - Win/Dos/NT use is NOT > SUPPORTED if( $file =~ /\.jpg$/i || $file =~ /\.jpeg$/i ) { > === bin/manage > ================================================================== > --- bin/manage (/twiki/trunk) (revision 281) > +++ bin/manage (/twiki/branches/robustness) (revision 281) > @@ -87,8 +87,10 @@ > > TWiki::UI::Manage::removeUser($webName, $topic, $wikiName, $query); > > -} elsif( $action eq "relockrcs" ) { > - TWiki::UI::Manage::relockRcsFiles(); > +# FIXME: Contains backticks, needs refactoring, upstream has been > informed. +# } elsif( $action eq "relockrcs" ) { > +# TWiki::UI::Manage::relockRcsFiles(); > + > } elsif( $action ) { > > my( $topic, $webName, $dummy, $userName ) = > > > > ------------------------------------------------------- > SF email is sponsored by - The IT Product Guide > Read honest & candid reviews on hundreds of IT Products from real users. > Discover which products truly live up to the hype. Start reading now. > http://productguide.itmanagersjournal.com/ > _______________________________________________ > TWiki-Dev mailing list > TWi...@li... > https://lists.sourceforge.net/lists/listinfo/twiki-dev -- C-dot Groupware Consultants http://www.c-dot.co.uk |