Tainting is moot because the whole purpose of the script is to run arbitrary user-provided code (in patch form). Still validate the patch filename but use the standard IsValidFileName() function (instead of the much stricter regular expression used so far) so patches accepted by Submit.pl don't get rejected at this late stage. However carefully quote the filename when building our shell command so it does not fail if the filename contains a space (for instance). Also note that in practice (so far) the patch filename is always called patch.diff as this is hardcoded in WineRunBuild.pl. So this is all a bit academic.
Signed-off-by: Francois Gouget fgouget@codeweavers.com --- testbot/bin/build/Build.pl | 25 ++++++++++++++----------- testbot/lib/WineTestBot/Utils.pm | 26 +++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 12 deletions(-)
diff --git a/testbot/bin/build/Build.pl b/testbot/bin/build/Build.pl index 8318af12a..e97995559 100755 --- a/testbot/bin/build/Build.pl +++ b/testbot/bin/build/Build.pl @@ -1,10 +1,13 @@ -#!/usr/bin/perl -Tw +#!/usr/bin/perl # -*- Mode: Perl; perl-indent-level: 2; indent-tabs-mode: nil -*- # # Performs the 'build' task in the build machine. Specifically this applies a # conformance test patch, rebuilds the impacted test and retrieves the # resulting 32 and 64 bit binaries. # +# This script does not use tainting (-T) because its whole purpose is to run +# arbitrary user-provided code anyway (in patch form). +# # Copyright 2009 Ge van Geldorp # Copyright 2012-2014, 2017-2018 Francois Gouget # @@ -22,6 +25,7 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+use warnings; use strict;
sub BEGIN @@ -42,6 +46,8 @@ sub BEGIN
use WineTestBot::Config; use WineTestBot::PatchUtils; +use WineTestBot::Utils; +
my $LogFileName = "$LogDir/Build.log";
@@ -85,7 +91,7 @@ sub ApplyPatch($)
InfoMsg "Applying patch\n"; system("( cd $DataDir/wine && set -x && " . - " git apply --verbose $PatchFile && " . + " git apply --verbose ". ShQuote($PatchFile) ." && ". " git add -A " . ") >>$LogFileName 2>&1"); if ($? != 0) @@ -199,18 +205,15 @@ if (! $PatchFile || !$BitIndicators) FatalError "Usage: Build.pl <patchfile> <bits>\n"; }
-# Untaint parameters -if ($PatchFile =~ m/^([\w_.-]+)$/) +# Verify parameters +if (!IsValidFileName($PatchFile)) { - $PatchFile = "$DataDir/staging/$1"; - if (! -r $PatchFile) - { - FatalError "Patch file $PatchFile not readable\n"; - } + FatalError "The patch filename '$PatchFile' contains invalid characters\n"; } -else +$PatchFile = "$DataDir/staging/$PatchFile"; +if (!-r $PatchFile) { - FatalError "Invalid patch file $PatchFile\n"; + FatalError "Patch file '$PatchFile' is not readable\n"; }
my ($Run32, $Run64); diff --git a/testbot/lib/WineTestBot/Utils.pm b/testbot/lib/WineTestBot/Utils.pm index 962e6ff67..111a56589 100644 --- a/testbot/lib/WineTestBot/Utils.pm +++ b/testbot/lib/WineTestBot/Utils.pm @@ -28,7 +28,8 @@ WineTestBot::Utils - Utility functions use Exporter 'import'; our @EXPORT = qw(MakeSecureURL SecureConnection GenerateRandomString OpenNewFile CreateNewFile CreateNewLink CreateNewDir - DurationToString BuildEMailRecipient IsValidFileName); + DurationToString BuildEMailRecipient IsValidFileName + ShQuote);
use Fcntl;
@@ -196,4 +197,27 @@ sub IsValidFileName($) return $FileName !~ m~[<>:"/\|?*]~; }
+=pod +=over 12 + +=item C<ShQuote()> + +Quotes strings so they can be used in shell commands. + +Note that this implies escaping '$'s and '`'s which may not be appropriate +in another context. + +=back +=cut + +sub ShQuote($) +{ + my ($Str)=@_; + $Str =~ s%\%\\%g; + $Str =~ s%$%\$%g; + $Str =~ s%"%\"%g; + $Str =~ s%`%\`%g; + return ""$Str""; +} + 1;