Francois Gouget : testbot/Build: Don' t use tainting and improve handling of the patch filename.

[Alexandre Julliard]

Module: tools
Branch: master
Commit: 62342aa288bfaacafb0c5a52f125fae0089e9e1e
URL:    https://source.winehq.org/git/tools.git/?a=commit;h=62342aa288bfaacafb0c5a52...

Author: Francois Gouget 
Date:   Wed Jun 20 02:43:59 2018 +0200

testbot/Build: Don't use tainting and improve handling of the patch filename.

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 
Signed-off-by: Alexandre Julliard

---

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 8318af1..e979955 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 962e6ff..111a565 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;